Skip to contentSkip to conversation

seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()

PATCH Superseded Kees Cook 3 participants 7 messages Oct 26, 2023 – Oct 27, 2023

Subsystems (2)
Library code Tracing

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_cstr()

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_cstr():

	do_soemthing(seq_buf_cstr(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]>
---
 include/linux/seq_buf.h | 19 +++++++++++++++----
 kernel/trace/trace.c    | 11 +----------
 lib/seq_buf.c           |  4 +---
 3 files changed, 17 insertions(+), 17 deletions(-)
include/linux/seq_buf.h +15 -4
index 8483e4b2d0d2..8896b830eb3d 100644
@@ -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_cstr - 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_cstr(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;
 }
 
 /**
kernel/trace/trace.c +1 -10
index d629065c2383..d83f36dc4bf8 100644
@@ -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_cstr(&iter->seq.seq))) {
 			int ret;
 
 			/* Try to safely read the string */
lib/seq_buf.c +1 -3
index b7477aefff53..165caed5a74e 100644
@@ -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_cstr(s);
 	while ((lf = strchr(start, '\n'))) {
 		int len = lf - start + 1;
Signature · 1 line
2.34.1
maintainer
#1 lore raw
Quoted 1 line · On Thu, 26 Oct 2023 10:07:28 -0700 Kees Cook <[email protected]> wrote:

Solve two ergonomic issues with struct seq_buf:

"ergonomic"? Does it cause carpal tunnel? ;-)

Quoted 17 lines

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):

Quoted 7 lines
	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_cstr():

	do_soemthing(seq_buf_cstr(s));

Do we really need to call it _cstr? Why not just have seq_buf_str() ?

I mean, this is C, do we need to state that in the name too?

BTW, I'm perfectly fine with this change, just the naming I have issues with.

-- Steve

author
#2 lore raw
Quoted 27 lines · Steven Rostedt
Quoted 1 line · On Thu, 26 Oct 2023 10:07:28 -0700 Kees Cook <[email protected]> wrote:

Solve two ergonomic issues with struct seq_buf:

"ergonomic"? Does it cause carpal tunnel? ;-)

Quoted 20 lines

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);
Quoted 8 lines
Quoted 4 lines

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

	do_soemthing(seq_buf_cstr(s));

Do we really need to call it _cstr? Why not just have seq_buf_str() ?

I mean, this is C, do we need to state that in the name too?

I'm fine either way. I did that just to make the distinction between our length-managed string of characters interface (seq_buf), and the %NUL-terminated string of characters (traditionally called "C String" in other languages). And it was still shorter than "seq_buf_terminate(s); s->buffer" ;)

Quoted 2 lines

BTW, I'm perfectly fine with this change, just the naming I have issues with.

Cool; thanks for looking at it!

Signature · 1 line
Kees Cook
maintainer
#3 lore raw
Quoted 9 lines · On Thu, 26 Oct 2023 10:54:26 -0700 Kees Cook <[email protected]> wrote:
Quoted 3 lines

Do we really need to call it _cstr? Why not just have seq_buf_str() ?

I mean, this is C, do we need to state that in the name too?

I'm fine either way. I did that just to make the distinction between our length-managed string of characters interface (seq_buf), and the %NUL-terminated string of characters (traditionally called "C String" in other languages). And it was still shorter than "seq_buf_terminate(s); s->buffer" ;)

Do you believe that people might get confused with it as seq_buf_str()?

Can you envision that we would want a seq_buf_str() and seq_buf_cstr() that do something different?

-- Steve

author
#4 lore raw
Quoted 13 lines · Steven Rostedt
Quoted 9 lines · On Thu, 26 Oct 2023 10:54:26 -0700 Kees Cook <[email protected]> wrote:
Quoted 3 lines

Do we really need to call it _cstr? Why not just have seq_buf_str() ?

I mean, this is C, do we need to state that in the name too?

I'm fine either way. I did that just to make the distinction between our length-managed string of characters interface (seq_buf), and the %NUL-terminated string of characters (traditionally called "C String" in other languages). And it was still shorter than "seq_buf_terminate(s); s->buffer" ;)

Quoted 4 lines

Do you believe that people might get confused with it as seq_buf_str()?

Can you envision that we would want a seq_buf_str() and seq_buf_cstr() that do something different?

No, I see your point. Like I said, I don't care either way. I was just explaining why I did it that way. "string" means a lot of things to different people. "C String" is unambiguous, and I try to be unambiguous whenever possible. :)

I'll send a v2 as seq_buf_str()...

Signature · 1 line
Kees Cook
#6 lore raw
In reply to Kees Cook
Quoted 11 lines · Kees Cook
Quoted 5 lines
Quoted 1 line
	do_soemthing(seq_buf_cstr(s));

Do we really need to call it _cstr? Why not just have seq_buf_str() ?

I mean, this is C, do we need to state that in the name too?

I'm fine either way. I did that just to make the distinction between our length-managed string of characters interface (seq_buf), and the %NUL-terminated string of characters (traditionally called "C String" in other languages). And it was still shorter than "seq_buf_terminate(s); s->buffer" ;)

'cstr' might be short for 'counted string' ...