[v2] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str()

  Solve two ergonomic issues with struct seq_buf;

1) Too much boilerplate is required to initialize:

	struct seq_buf s;
	char buf[32];

	seq_buf_init(s, buf, sizeof(buf));

Instead, we can build this directly on the stack. Provide
DECLARE_SEQ_BUF() macro to do this:

	DECLARE_SEQ_BUF(s, 32);

2) %NUL termination is fragile and requires 2 steps to get a valid
   C String (and is a layering violation exposing the "internals" of
   seq_buf):

	seq_buf_terminate(s);
	do_something(s->buffer);

Instead, we can just return s->buffer direction after terminating it
in refactored seq_buf_terminate(), now known as seq_buf_str():

	do_soemthing(seq_buf_str(s));

Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: "Matthew Wilcox (Oracle)" <willy@infradead.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Justin Stitt <justinstitt@google.com>
Cc: Kent Overstreet <kent.overstreet@linux.dev>
Cc: Petr Mladek <pmladek@suse.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc: Sergey Senozhatsky <senozhatsky@chromium.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Jonathan Corbet <corbet@lwn.net>
Cc: Yun Zhou <yun.zhou@windriver.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>
Cc: Zhen Lei <thunder.leizhen@huawei.com>
Cc: linux-trace-kernel@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v2 - rename seq_buf_cstr() to seq_buf_str() (rostedt)
v1 - https://lore.kernel.org/all/20231026170722.work.638-kees@kernel.org/
---
 include/linux/seq_buf.h | 19 +++++++++++++++----
 kernel/trace/trace.c    | 11 +----------
 lib/seq_buf.c           |  4 +---
 3 files changed, 17 insertions(+), 17 deletions(-)
  

On Thu, 26 Oct 2023 12:40:37 -0700
Kees Cook <keescook@chromium.org> wrote:

> @@ -81,16 +88,20 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
>   *
>   * After this function is called, s->buffer is safe to use
>   * in string operations.
> + *
> + * Returns @s->buf after making sure it is terminated.
>   */
> -static inline void seq_buf_terminate(struct seq_buf *s)
> +static inline char *seq_buf_str(struct seq_buf *s)

Looking at show_buffer() (below), I wonder if this should be:

static inline const char *seq_buf_str() ?

I mean, it can be modified, but do we want to allow that?

-- Steve


>  {
>  	if (WARN_ON(s->size == 0))
> -		return;
> +		return "";
>  
>  	if (seq_buf_buffer_left(s))
>  		s->buffer[s->len] = 0;
>  	else
>  		s->buffer[s->size - 1] = 0;
> +
> +	return s->buffer;
>  }
>  
>  /**
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index d629065c2383..2539cfc20a97 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -3828,15 +3828,6 @@ static bool trace_safe_str(struct trace_iterator *iter, const char *str,
>  	return false;
>  }
>  
> -static const char *show_buffer(struct trace_seq *s)
> -{
> -	struct seq_buf *seq = &s->seq;
> -
> -	seq_buf_terminate(seq);
> -
> -	return seq->buffer;
> -}
> -
>  static DEFINE_STATIC_KEY_FALSE(trace_no_verify);
>
  

On Thu, Oct 26, 2023 at 12:40:37PM -0700, Kees Cook wrote:
> Solve two ergonomic issues with struct seq_buf;
> 
> 1) Too much boilerplate is required to initialize:
> 
> 	struct seq_buf s;
> 	char buf[32];
> 
> 	seq_buf_init(s, buf, sizeof(buf));
> 
> Instead, we can build this directly on the stack. Provide
> DECLARE_SEQ_BUF() macro to do this:
> 
> 	DECLARE_SEQ_BUF(s, 32);
> 
> 2) %NUL termination is fragile and requires 2 steps to get a valid
>    C String (and is a layering violation exposing the "internals" of
>    seq_buf):
> 
> 	seq_buf_terminate(s);
> 	do_something(s->buffer);
> 
> Instead, we can just return s->buffer direction after terminating it
> in refactored seq_buf_terminate(), now known as seq_buf_str():
> 
> 	do_soemthing(seq_buf_str(s));

...

> +#define DECLARE_SEQ_BUF(NAME, SIZE)					\
> +	char __ ## NAME ## _buffer[SIZE] = "";				\
> +	struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer,	\
> +				.size = SIZE }

Hmm... Wouldn't be more readable to have it as

#define DECLARE_SEQ_BUF(NAME, SIZE)			\
	char __ ## NAME ## _buffer[SIZE] = "";		\
	struct seq_buf NAME = {				\
		.buffer = &__ ## NAME ## _buffer,	\
		.size = SIZE,				\
	}

?

...

> +static inline char *seq_buf_str(struct seq_buf *s)
>  {
>  	if (WARN_ON(s->size == 0))
> -		return;
> +		return "";

I'm wondering why it's a problem to have an empty string?

>  	if (seq_buf_buffer_left(s))
>  		s->buffer[s->len] = 0;
>  	else
>  		s->buffer[s->size - 1] = 0;
> +
> +	return s->buffer;
>  }
  

On Thu, 26 Oct 2023 23:20:15 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

> > +#define DECLARE_SEQ_BUF(NAME, SIZE)					\
> > +	char __ ## NAME ## _buffer[SIZE] = "";				\
> > +	struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer,	\
> > +				.size = SIZE }  
> 
> Hmm... Wouldn't be more readable to have it as
> 
> #define DECLARE_SEQ_BUF(NAME, SIZE)			\
> 	char __ ## NAME ## _buffer[SIZE] = "";		\
> 	struct seq_buf NAME = {				\
> 		.buffer = &__ ## NAME ## _buffer,	\
> 		.size = SIZE,				\
> 	}
> 
> ?

I agree with the above.

> 
> ...
> 
> > +static inline char *seq_buf_str(struct seq_buf *s)
> >  {
> >  	if (WARN_ON(s->size == 0))
> > -		return;
> > +		return "";  
> 
> I'm wondering why it's a problem to have an empty string?

Not sure what you mean? With s->size = 0, s->buffer may not have been
assigned. That shouldn't be the case, but it does make it more robust.

-- Steve


> 
> >  	if (seq_buf_buffer_left(s))
> >  		s->buffer[s->len] = 0;
> >  	else
> >  		s->buffer[s->size - 1] = 0;
> > +
> > +	return s->buffer;
> >  }  
>
  

On Thu, Oct 26, 2023 at 12:40:37PM -0700, Kees Cook wrote:
> Solve two ergonomic issues with struct seq_buf;
> 
> 1) Too much boilerplate is required to initialize:
> 
> 	struct seq_buf s;
> 	char buf[32];
> 
> 	seq_buf_init(s, buf, sizeof(buf));
> 
> Instead, we can build this directly on the stack. Provide
> DECLARE_SEQ_BUF() macro to do this:
> 
> 	DECLARE_SEQ_BUF(s, 32);

DECLARE_SEQ_BUF_ONSTACK maybe?  But otherwise this looks like a good
concept.

> Instead, we can just return s->buffer direction after terminating it
> in refactored seq_buf_terminate(), now known as seq_buf_str():
> 
> 	do_soemthing(seq_buf_str(s));

Looks good.  Btw, one typical do_something would be printing it,
so adding a format specifier that's using this helper would also
probably be very useful.
  

On Fri, Oct 27, 2023 at 06:54:51AM +0200, Christoph Hellwig wrote:
> > Instead, we can just return s->buffer direction after terminating it
> > in refactored seq_buf_terminate(), now known as seq_buf_str():
> > 
> > 	do_soemthing(seq_buf_str(s));
> 
> Looks good.  Btw, one typical do_something would be printing it,
> so adding a format specifier that's using this helper would also
> probably be very useful.

my hope is to get vsprintf.c completely refactored to use seq_buf
internally, and then printsb(&sb) would actually be a primitive we'd
have insted of printk("%pSB", &sb);

this would btw let us get rid of the entire %pFOO infrastructure.
and make dump_page() far less crap.
  

On Thu, Oct 26, 2023 at 03:44:59PM -0400, Steven Rostedt wrote:
> On Thu, 26 Oct 2023 12:40:37 -0700
> Kees Cook <keescook@chromium.org> wrote:
> 
> > @@ -81,16 +88,20 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
> >   *
> >   * After this function is called, s->buffer is safe to use
> >   * in string operations.
> > + *
> > + * Returns @s->buf after making sure it is terminated.
> >   */
> > -static inline void seq_buf_terminate(struct seq_buf *s)
> > +static inline char *seq_buf_str(struct seq_buf *s)
> 
> Looking at show_buffer() (below), I wonder if this should be:
> 
> static inline const char *seq_buf_str() ?
> 
> I mean, it can be modified, but do we want to allow that?

Yeah, good idea. I've updated this for v3.
  

On Thu, Oct 26, 2023 at 11:20:15PM +0300, Andy Shevchenko wrote:
> On Thu, Oct 26, 2023 at 12:40:37PM -0700, Kees Cook wrote:
> > Solve two ergonomic issues with struct seq_buf;
> > 
> > 1) Too much boilerplate is required to initialize:
> > 
> > 	struct seq_buf s;
> > 	char buf[32];
> > 
> > 	seq_buf_init(s, buf, sizeof(buf));
> > 
> > Instead, we can build this directly on the stack. Provide
> > DECLARE_SEQ_BUF() macro to do this:
> > 
> > 	DECLARE_SEQ_BUF(s, 32);
> > 
> > 2) %NUL termination is fragile and requires 2 steps to get a valid
> >    C String (and is a layering violation exposing the "internals" of
> >    seq_buf):
> > 
> > 	seq_buf_terminate(s);
> > 	do_something(s->buffer);
> > 
> > Instead, we can just return s->buffer direction after terminating it
> > in refactored seq_buf_terminate(), now known as seq_buf_str():
> > 
> > 	do_soemthing(seq_buf_str(s));
> 
> ...
> 
> > +#define DECLARE_SEQ_BUF(NAME, SIZE)					\
> > +	char __ ## NAME ## _buffer[SIZE] = "";				\
> > +	struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer,	\
> > +				.size = SIZE }
> 
> Hmm... Wouldn't be more readable to have it as
> 
> #define DECLARE_SEQ_BUF(NAME, SIZE)			\
> 	char __ ## NAME ## _buffer[SIZE] = "";		\
> 	struct seq_buf NAME = {				\
> 		.buffer = &__ ## NAME ## _buffer,	\
> 		.size = SIZE,				\
> 	}
> 
> ?

Yes, I don't know why I did it the smooshed way. Fixed for v3.

> > +static inline char *seq_buf_str(struct seq_buf *s)
> >  {
> >  	if (WARN_ON(s->size == 0))
> > -		return;
> > +		return "";
> 
> I'm wondering why it's a problem to have an empty string?

Well, it's a pathological case where "size" is 0 -- it shouldn't happen
(hence the warn), but it's more robust to return an empty .data string
pointer than a NULL s->buffer or an s->buffer that isn't intended to be
used (i.e. the size == 0).
  

On Fri, Oct 27, 2023 at 06:54:51AM +0200, Christoph Hellwig wrote:
> On Thu, Oct 26, 2023 at 12:40:37PM -0700, Kees Cook wrote:
> > Solve two ergonomic issues with struct seq_buf;
> > 
> > 1) Too much boilerplate is required to initialize:
> > 
> > 	struct seq_buf s;
> > 	char buf[32];
> > 
> > 	seq_buf_init(s, buf, sizeof(buf));
> > 
> > Instead, we can build this directly on the stack. Provide
> > DECLARE_SEQ_BUF() macro to do this:
> > 
> > 	DECLARE_SEQ_BUF(s, 32);
> 
> DECLARE_SEQ_BUF_ONSTACK maybe?  But otherwise this looks like a good
> concept.

It's usable for globals too... also it's a shorter name as-is. :)