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 <[email protected]>
Cc: "Matthew Wilcox (Oracle)" <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Justin Stitt <[email protected]>
Cc: Kent Overstreet <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Rasmus Villemoes <[email protected]>
Cc: Sergey Senozhatsky <[email protected]>
Cc: Masami Hiramatsu <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Jonathan Corbet <[email protected]>
Cc: Yun Zhou <[email protected]>
Cc: Jacob Keller <[email protected]>
Cc: Zhen Lei <[email protected]>
Cc: [email protected]
Signed-off-by: Kees Cook <[email protected]>
---
v2 - rename seq_buf_cstr() to seq_buf_str() (rostedt)
v1 - https://lore.kernel.org/all/[email protected]/
---
include/linux/seq_buf.h | 19 +++++++++++++++----
kernel/trace/trace.c | 11 +----------
lib/seq_buf.c | 4 +---
3 files changed, 17 insertions(+), 17 deletions(-)
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;
--
2.34.1
On Thu, 26 Oct 2023 12:40:37 -0700
Kees Cook <[email protected]> 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;
> }
--
With Best Regards,
Andy Shevchenko
On Thu, 26 Oct 2023 23:20:15 +0300
Andy Shevchenko <[email protected]> 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 <[email protected]> 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.
--
Kees Cook
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).
--
Kees Cook
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. :)
--
Kees Cook