| Message ID | 20231026194033.it.702-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str() | expand |
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. :)
diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h index 8483e4b2d0d2..71eb4d624308 100644 --- a/include/linux/seq_buf.h +++ b/include/linux/seq_buf.h @@ -21,9 +21,16 @@ struct seq_buf { size_t len; }; +#define DECLARE_SEQ_BUF(NAME, SIZE) \ + char __ ## NAME ## _buffer[SIZE] = ""; \ + struct seq_buf NAME = { .buffer = &__ ## NAME ## _buffer, \ + .size = SIZE } + static inline void seq_buf_clear(struct seq_buf *s) { s->len = 0; + if (s->size) + s->buffer[0] = '\0'; } static inline void @@ -69,8 +76,8 @@ static inline unsigned int seq_buf_used(struct seq_buf *s) } /** - * seq_buf_terminate - Make sure buffer is nul terminated - * @s: the seq_buf descriptor to terminate. + * seq_buf_str - get %NUL-terminated C string from seq_buf + * @s: the seq_buf handle * * This makes sure that the buffer in @s is nul terminated and * safe to read as a string. @@ -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) { 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); static int test_can_verify_check(const char *fmt, ...) @@ -3976,7 +3967,7 @@ void trace_check_vprintf(struct trace_iterator *iter, const char *fmt, */ if (WARN_ONCE(!trace_safe_str(iter, str, star, len), "fmt: '%s' current_buffer: '%s'", - fmt, show_buffer(&iter->seq))) { + fmt, seq_buf_str(&iter->seq.seq))) { int ret; /* Try to safely read the string */ diff --git a/lib/seq_buf.c b/lib/seq_buf.c index b7477aefff53..23518f77ea9c 100644 --- a/lib/seq_buf.c +++ b/lib/seq_buf.c @@ -109,9 +109,7 @@ void seq_buf_do_printk(struct seq_buf *s, const char *lvl) if (s->size == 0 || s->len == 0) return; - seq_buf_terminate(s); - - start = s->buffer; + start = seq_buf_str(s); while ((lf = strchr(start, '\n'))) { int len = lf - start + 1;
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(-)