seq_buf: Introduce DECLARE_SEQ_BUF and seq_buf_str()

include/linux/seq_buf.h +3 -1

 {
 	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 +0 -9

index d629065c2383..2539cfc20a97 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);
 

include/linux/seq_buf.h static inline unsigned int seq_buf_used(struct seq_buf *s) · line 94

Quoted 9 lines · On Thu, 26 Oct 2023 12:40:37 -0700 Kees Cook <[email protected]> wrote:

include/linux/seq_buf.h +3 -1

@@ -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?

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

include/linux/seq_buf.h +2 -0

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

include/linux/seq_buf.h struct seq_buf { · line 27

Quoted 4 lines

include/linux/seq_buf.h +4 -0

+#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,				\
	}

?

...

include/linux/seq_buf.h static inline unsigned int seq_buf_used(struct seq_buf *s) · line 97

Quoted 5 lines

include/linux/seq_buf.h +2 -1

+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?

Quoted 8 lines

include/linux/seq_buf.h +2 -0

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

Quoted 25 lines · Kees Cook

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

...

include/linux/seq_buf.h struct seq_buf { · line 27

Quoted 4 lines

include/linux/seq_buf.h +4 -0

+#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,				\
	}

?

include/linux/seq_buf.h static inline unsigned int seq_buf_used(struct seq_buf *s) · line 97

Quoted 5 lines

include/linux/seq_buf.h +2 -1

+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?

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

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

Quoted 4 lines

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.

Quoted 9 lines · Kees Cook

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

Quoted 4 lines

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.