seq_buf: Add seq_buf_strlen()

Hi,

While working on seq_buf conversions[1], we found there was a need for seq_buf_strlen() (since calling strlen(seq_buf_str()) would be a waste of time: seq_buf already knows the length), seq_buf_init_append() (to start a seq_buf from an existing string buffer), and seq_buf_terminate() (as there are many callers using seq_buf_str() just for its termination side-effect). And while implementing that, we found a bunch of other related things that needed fixing. This is that ever-growing series, with tests for each fix.

v4:
- add trailers from v3
- 03/11, 04/11: export seq_buf_putmem() and seq_buf_path(), which the
tests call, so they also build as a module
- 08/11: start out overflowed when the buffer has no NUL within its
size, as strlcat() treats it as already truncated (Andy)
- 09/11: use seq_buf_strlen() for flags_show()'s check whether any
flag was printed, too (Madhavan)
- 01/11, 05/11: drop doubled blank lines and re-wrap a declaration in
the tests (Andy)
- v3..v4 diff: https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/diff/?id=dev/v7.3-rc2/seq_buf/v4&id2=dev/v7.3-rc2/seq_buf/v3
v3: https://lore.kernel.org/all/[email protected]/
v2: https://lore.kernel.org/all/[email protected]/
v1: https://lore.kernel.org/all/[email protected]/

Thanks!

-Kees

[1] https://lore.kernel.org/all/[email protected]/

Bill Wendling (1):
seq_buf: Add seq_buf_init_append()

Kees Cook (10):
seq_buf: Do not print an empty line from an overflowed

    seq_buf_do_printk()

seq_buf: Do not pop from an overflowed seq_buf
seq_buf: Copy what fits when seq_buf_puts() and seq_buf_putmem()
overflow
seq_buf: Clear what a writer did not claim when a seq_buf overflows
seq_buf: Add seq_buf_strlen()
seq_buf: Add seq_buf_terminate()
bpf: Remove dead newline stripping from format_disasm_line() powerpc/papr_scm: Return the string length from the sysfs show
functions
nvdimm: ndtest: Return the string length from flags_show()
docs: core-api: Document the seq_buf API

 Documentation/core-api/kernel-api.rst     |   9 +
 include/linux/seq_buf.h                   | 139 +++++-
 include/linux/trace_seq.h                 |   5 +-
 arch/powerpc/platforms/pseries/papr_scm.c |   6 +-
 kernel/bpf/diagnostics.c                  |   8 +-
 kernel/trace/trace_events.c               |   4 +-
 kernel/trace/trace_events_hist.c          |   6 +-
 lib/seq_buf.c                             |  37 +-
 lib/tests/seq_buf_kunit.c                 | 528 +++++++++++++++++++++-
 tools/testing/nvdimm/test/ndtest.c        |   2 +-
 10 files changed, 708 insertions(+), 36 deletions(-)

2.55.0

seq_buf_do_printk() prints a buffer line by line, then prints whatever follows the last newline. When a string has overflowed at exactly a newline, an empty line is printed since the pointer hasn't reached the overflow mark of the seq_buf. Switch to just check if the string is already empty and only print if not.

The only caller is the memory cgroup OOM report, so this could only ever add a blank line to a report whose statistics already did not fit.

Add a test that registers a console to count the records that seq_buf_do_printk() emits. It counts the records carrying the test's marker, and the records holding nothing but a line feed that arrive after one, so that unrelated kernel messages do not disturb it. Both states that reach the flaw are covered: exactly full, and overflowed.

Tests passed under qemu on ARCH=x86_64 with GCC 16.2.0 and CONFIG_KASAN=y, and on big-endian ARCH=s390 with GCC s390x-linux-gnu 16.2.0.

---
 lib/seq_buf.c             |   2 +-
 lib/tests/seq_buf_kunit.c | 132 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+), 1 deletion(-)

index a92093f346da..35a5964370b4 100644
@@ -128,7 +128,7 @@ void seq_buf_do_printk(struct seq_buf *s, const char *lvl)
 	}
 
 	/* No trailing LF */
-	if (start < s->buffer + s->len)
+	if (*start)
 		printk("%s%s\n", lvl, start);
 }
 EXPORT_SYMBOL_GPL(seq_buf_do_printk);

2.55.0

That's a pointless state to handle: KUnit reports its own results at KERN_INFO, so with a console that hides KERN_INFO, or with printk compiled out, all of KUnit is unusable, not just this test.

-Kees

Kees Cook

When a seq_buf has overflowed, its len is size + 1, so seq_buf_pop() decrements len to size and reads buffer[size], one byte past the end of the buffer. It also leaves len equal to size, which no longer counts as overflowed, so a truncated seq_buf then looks like a complete, full one.

An overflowed seq_buf logically has no last character to pop: the length of what was written has been lost, and the last byte of the buffer may be the NUL written by vsnprintf() or bytes that were never committed. Return -1 for an overflowed seq_buf, as for an empty one, and leave it overflowed, as the rest of the seq_buf API does until seq_buf_clear() or seq_buf_init().

The current callers do not reach this, e.g. trace_syscalls only calls trace_seq_pop() when the trace_seq it pops from has not overflowed, and kernel/bpf/diagnostics.c sets the length from strnlen() before popping.

Add tests for the pop corner cases.

Tests passed under qemu on ARCH=x86_64 with GCC 16.2.0 and CONFIG_KASAN=y, and on big-endian ARCH=s390 with GCC s390x-linux-gnu 16.1.0.

---
 include/linux/seq_buf.h   |  4 ++--
 include/linux/trace_seq.h |  5 ++++-
 lib/tests/seq_buf_kunit.c | 42 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 3 deletions(-)

index 9f2839e73f8a..f5a350347bc5 100644
@@ -155,11 +155,11 @@ static inline void seq_buf_commit(struct seq_buf *s, int num)
  *
  * Removes the last written character to the seq_buf @s.
  *
- * Returns the last character or -1 if it is empty.
+ * Returns the last character, or -1 if @s is empty or has overflowed.
  */
 static inline int seq_buf_pop(struct seq_buf *s)
 {
-	if (!s->len)
+	if (!s->len || seq_buf_has_overflowed(s))
 		return -1;
 
 	s->len--;

index 697d619aafdc..7174ebf3f015 100644
@@ -86,7 +86,10 @@ static inline bool trace_seq_has_overflowed(struct trace_seq *s)
  *
  * Removes the last written character to the trace_seq @s.
  *
- * Returns the last character or -1 if it is empty.
+ * Returns the last character, or -1 if the underlying seq_buf is empty or
+ * has overflowed. Note that only that buffer is consulted: a @s marked
+ * full by a write that did not fit, which trace_seq_has_overflowed()
+ * reports as overflowed, still pops the last character written.
  */
 static inline int trace_seq_pop(struct trace_seq *s)
 {

2.55.0

When seq_buf_puts() or seq_buf_putmem() is given more than fits, it copies nothing and only marks the seq_buf as overflowed. If seq_buf_str() is used, it will terminate the buffer in its last byte, so every byte between the end of the data and the end of the buffer becomes part of the string, though the seq_buf never wrote them.

seq_buf_printf() does not have this problem, because vsnprintf() writes as much of the output as fits, followed by a NUL. Repeat this behavior in seq_buf_puts(), using strscpy(), and in seq_buf_putmem(), which also covers seq_buf_putmem_hex(). seq_buf_putc() needs no change, as it can only overflow when the buffer is already full.

Each writer now records the buffer as full once it has copied what fits, so that what it wrote can be told apart from bytes nothing touched.

Update seq_buf_putmem_hex_overflow_test, which expected a hex group that did not fit whole to be left out entirely, and add tests that overflow seq_buf_puts(), seq_buf_putmem() and seq_buf_putmem_hex() with stale bytes in the buffer.

The three partial-overflow tests check the buffer itself rather than seq_buf_str(). Neither writer leaves the last byte of the buffer alone:
seq_buf_putmem() copies raw bytes and writes no NUL, and seq_buf_puts() relies on strscpy() to write one. That byte is exactly where seq_buf_str() writes its terminator, so asserting only on the string would pass whether the copy stopped a byte early or dropped the NUL entirely.

seq_buf_putmem() was never exported, unlike the other writers, so the test failed to link as a module. Export it.

Tests passed under qemu on ARCH=x86_64 with GCC 16.2.0 and CONFIG_KASAN=y, and on big-endian ARCH=s390 with GCC s390x-linux-gnu 16.2.0, and the test builds as a module (CONFIG_SEQ_BUF_KUNIT_TEST=m).

---
 include/linux/seq_buf.h   |  6 +++
 lib/seq_buf.c             | 17 +++++++-
 lib/tests/seq_buf_kunit.c | 85 ++++++++++++++++++++++++++++++++++++++-
 3 files changed, 104 insertions(+), 4 deletions(-)

index f5a350347bc5..77e76e283370 100644
@@ -55,6 +55,12 @@ seq_buf_has_overflowed(struct seq_buf *s)
 	return s->len > s->size;
 }
 
+/*
+ * Mark @s as overflowed, which discards the length of what it holds. The
+ * bytes up to its last one are the string from then on, as that is where
+ * seq_buf_str() terminates it, so a caller that could not fill the buffer
+ * has to NUL them itself before calling this.
+ */
 static inline void
 seq_buf_set_overflow(struct seq_buf *s)
 {

index 35a5964370b4..60e9eadb3ef7 100644
@@ -175,7 +175,9 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
  * @s: seq_buf descriptor
  * @str: simple string to record
  *
- * Copy a simple string into the sequence buffer.
+ * Copy a simple string into the sequence buffer. If @str does not fit,
+ * as much of it as fits is copied, followed by a null byte, as
+ * seq_buf_printf() does.
  *
  * Returns: zero on success, -1 on overflow.
  */

@@ -194,6 +196,11 @@ int seq_buf_puts(struct seq_buf *s, const char *str)
 		s->len += len - 1;
 		return 0;
 	}
+	/* Copy what fits, so the buffer never holds stale bytes */
+	if (s->len < s->size) {
+		strscpy(s->buffer + s->len, str, s->size - s->len);
+		s->len = s->size;
+	}
 	seq_buf_set_overflow(s);
 	return -1;
 }

@@ -229,7 +236,7 @@ EXPORT_SYMBOL_GPL(seq_buf_putc);
  *
  * There may be cases where raw memory needs to be written into the
  * buffer and a strcpy() would not work. Using this function allows
- * for such cases.
+ * for such cases. If @mem does not fit, as much of it as fits is copied.
  *
  * Returns: zero on success, -1 on overflow.
  */

@@ -242,9 +249,15 @@ int seq_buf_putmem(struct seq_buf *s, const void *mem, unsigned int len)
 		s->len += len;
 		return 0;
 	}
+	/* Copy what fits, so the buffer never holds stale bytes */
+	if (s->len < s->size) {
+		memcpy(s->buffer + s->len, mem, s->size - s->len);
+		s->len = s->size;
+	}
 	seq_buf_set_overflow(s);
 	return -1;
 }
+EXPORT_SYMBOL_GPL(seq_buf_putmem);
 
 #define MAX_MEMHEX_BYTES	8U
 #define HEX_CHARS		(MAX_MEMHEX_BYTES*2 + 1)

2.55.0

Yes, good catch; lpfc does. For v5 there's a new patch ahead of this one that has lpfc_rx_monitor_report() print each record with seq_buf_printf() and, when one doesn't fit, end the string where it began, so its debugfs output still stops at the last whole record.

I've checked all the other users of seq_buf_puts() and didn't find any other cases like this.

-Kees

Kees Cook

Using seq_buf_set_overflow() would leave the bytes between "len" and "size" untouched, so if seq_buf_str() is used on an overflowed seq_buf, those bytes may be exposed. For any paths that don't claim partially written bytes, by setting "len = size" before calling seq_buf_set_overflow(), wipe the unclaimed bytes. The seq_buf_puts() and related APIs already claim those bytes now, so only the unclaimed cases remain. A specific example of this was seq_buf_path() which uses d_path() and would write to the tail before discovering it was out of space, and would correctly mark a seq_buf as overflowed, but the path fragment would be left over.

Clear from len to the end of the buffer in seq_buf_set_overflow(), which every overflow goes through, including seq_buf_commit() with a negative count.

Add a test that fills a seq_buf, leaves it too little room for a path, and checks that nothing of the path is left in the buffer. The tests run before anything writable is mounted, so it takes its file from shmem.

seq_buf_path() was never exported, unlike the other writers, so the test failed to link as a module. Export it.

Tests passed under qemu on ARCH=x86_64 with GCC 16.2.0 and CONFIG_KASAN=y, and on big-endian ARCH=s390 with GCC s390x-linux-gnu 16.2.0, and the test builds as a module (CONFIG_SEQ_BUF_KUNIT_TEST=m).

---
 include/linux/seq_buf.h   |  8 +++++--
 lib/seq_buf.c             |  5 +++++
 lib/tests/seq_buf_kunit.c | 45 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 2 deletions(-)

index 77e76e283370..0c0a0db04b09 100644
@@ -5,6 +5,7 @@
 #include <linux/bug.h>
 #include <linux/minmax.h>
 #include <linux/seq_file.h>
+#include <linux/string.h>
 #include <linux/types.h>
 
 /*

@@ -58,12 +59,15 @@ seq_buf_has_overflowed(struct seq_buf *s)
 /*
  * Mark @s as overflowed, which discards the length of what it holds. The
  * bytes up to its last one are the string from then on, as that is where
- * seq_buf_str() terminates it, so a caller that could not fill the buffer
- * has to NUL them itself before calling this.
+ * seq_buf_str() terminates it, so clear whatever was not written: a writer
+ * sets len to how much it filled, and anything past that was never adopted.
  */
 static inline void
 seq_buf_set_overflow(struct seq_buf *s)
 {
+	if (s->len < s->size)
+		memset(s->buffer + s->len, 0, s->size - s->len);
+
 	s->len = s->size + 1;
 }
 

index 60e9eadb3ef7..8da2e9447adf 100644
@@ -76,6 +76,8 @@ int seq_buf_vprintf(struct seq_buf *s, const char *fmt, va_list args)
 			s->len += len;
 			return 0;
 		}
+		/* vsnprintf() wrote as much as fits, so none of it is stale */
+		s->len = s->size;
 	}
 	seq_buf_set_overflow(s);
 	return -1;

@@ -164,6 +166,8 @@ int seq_buf_bprintf(struct seq_buf *s, const char *fmt, const u32 *binary)
 			s->len += ret;
 			return 0;
 		}
+		/* bstr_printf() wrote as much as fits, so none of it is stale */
+		s->len = s->size;
 	}
 	seq_buf_set_overflow(s);
 	return -1;

@@ -343,6 +347,7 @@ int seq_buf_path(struct seq_buf *s, const struct path *path, const char *esc)
 
 	return res;
 }
+EXPORT_SYMBOL_GPL(seq_buf_path);
 
 /**
  * seq_buf_to_user - copy the sequence buffer to user space

2.55.0

Several strlcat() call sites being converted to seq_buf need behavior seq_buf doesn't currently provide. The return from seq_buf_used() is not the length of the string in a seq_buf. Once the buffer is full or has overflowed it returns the buffer size, which counts the byte that seq_buf_str() replaces with the NUL, so a caller that needs the string and its length has to call seq_buf_str() and then walk the string with strlen().

Move the termination out of seq_buf_str() into a helper that returns where it put the NUL, and add seq_buf_strlen(), which terminates the buffer in the same way and returns that offset.

As discussed in review, don't add WARN_ON() for seq_buf_strlen() and drop it from seq_buf_str().

Add tests comparing seq_buf_strlen() against strlen() of seq_buf_str() for empty, appended, truncated, exactly full, and overflowed buffers, checking that seq_buf_strlen() alone terminates a full buffer, and checking that a zero-sized seq_buf reports an empty string from both accessors without touching the buffer.

Tests passed under qemu on ARCH=x86_64 with GCC 16.2.0 and CONFIG_KASAN=y, and on big-endian ARCH=s390 with GCC s390x-linux-gnu 16.2.0.

---
 include/linux/seq_buf.h   |  65 +++++++++++++++++--
 lib/tests/seq_buf_kunit.c | 129 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 188 insertions(+), 6 deletions(-)

index 0c0a0db04b09..87ccc62f1c62 100644
@@ -89,6 +89,27 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
 	return min(s->len, s->size);
 }
 
+/*
+ * NUL-terminate the buffer in @s: directly after the data when there is
+ * room for it, otherwise in the last byte of the buffer. @s->size must not
+ * be zero.
+ *
+ * Returns: the offset of the NUL.
+ */
+static inline size_t __seq_buf_terminate(struct seq_buf *s)
+{
+	size_t end;
+
+	if (seq_buf_buffer_left(s))
+		end = s->len;
+	else
+		end = s->size - 1;
+
+	s->buffer[end] = 0;
+
+	return end;
+}
+
 /**
  * seq_buf_str - get NUL-terminated C string from seq_buf
  * @s: the seq_buf handle

@@ -98,7 +119,12 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
  *
  * Note, if this is called when the buffer has overflowed, then
  * the last byte of the buffer is zeroed, and the len will still
- * point passed it.
+ * point passed it. The same happens when the buffer is exactly
+ * full: the NUL takes the place of the last byte written, which is
+ * lost, though seq_buf_used() still counts it.
+ *
+ * A zero-sized seq_buf has nowhere to put a NUL, so the empty string
+ * is returned instead of writing to @s->buffer.
  *
  * After this function is called, s->buffer is safe to use
  * in string operations.

@@ -107,17 +133,44 @@ static inline unsigned int seq_buf_used(struct seq_buf *s)
  */
 static inline const char *seq_buf_str(struct seq_buf *s)
 {
-	if (WARN_ON(s->size == 0))
+	if (s->size == 0)
 		return "";
 
-	if (seq_buf_buffer_left(s))
-		s->buffer[s->len] = 0;
-	else
-		s->buffer[s->size - 1] = 0;
+	__seq_buf_terminate(s);
 
 	return s->buffer;
 }
 
+/**
+ * seq_buf_strlen - get the length of the NUL-terminated C string in seq_buf
+ * @s: the seq_buf handle
+ *
+ * This makes sure that the buffer in @s is NUL-terminated, exactly as
+ * seq_buf_str() does, and returns the length of the resulting string
+ * without walking it. Unlike seq_buf_used(), this does not count the byte
+ * given up to the NUL when the buffer is full or has overflowed. When the
+ * buffer is exactly full, that byte is the last one written, and calling
+ * either function loses it.
+ *
+ * A zero-sized seq_buf holds no string, so 0 is returned without writing
+ * to @s->buffer, matching what seq_buf_str() returns for one.
+ *
+ * After this function is called, s->buffer is safe to use
+ * in string operations.
+ *
+ * Returns: the offset of the NUL that terminates @s->buffer. That is the
+ * length of the string unless an earlier NUL is in the way, either one the
+ * data written to @s carried itself, or one seq_buf_set_overflow() left
+ * behind when it cleared what no writer had claimed.
+ */
+static inline size_t seq_buf_strlen(struct seq_buf *s)
+{
+	if (s->size == 0)
+		return 0;
+
+	return __seq_buf_terminate(s);
+}
+
 /**
  * seq_buf_get_buf - get buffer to write arbitrary data to
  * @s: the seq_buf handle

2.55.0

...

	if (seq_buf_strlen(s))
		return s->buffer;

	return "";

With Best Regards,
Andy Shevchenko

It could, but I'm vaguely nervous about the difference between

	s->buffer[0] == '\0'
and
	.data ""

i.e. we only force the return of seq_buf_str() to be _not_ just s->buffer when s->buffer is weirdly impossible (due to size == 0). I'd rather not make all 0-len strings return the .data segment's const "" string...

Kees Cook

...

Good point. Perhaps then to add a short note to the kernel doc, so we won't see patches based on the suggestion like I gave?

With Best Regards,
Andy Shevchenko

Yeah, reasonable. :) For v5 I've added this to seq_buf_str()'s kernel-doc:

* A zero-sized seq_buf has nowhere to put a NUL, so the empty string
* is returned instead of writing to @s->buffer. Any other seq_buf
* returns @s->buffer, even when it holds an empty string, so callers
* always get their own buffer back.

-Kees

Kees Cook

Hi Kees, Andy,

Are such buffers actually used on purpose anywhere? Why not keep the WARN_ON?

Have a lovely day!
Alex

<https://www.alejandro-colomar.es>

Yes, though not often: the sched_ext debug dump builds a nested per-CPU seq_buf from seq_buf_get_buf(), which returns a size of 0 once the dump buffer has overflowed, and it handles that case on purpose (see the "$s may already have overflowed" comment in kernel/sched/ext/ext.c).

Greg asked about the WARN_ON() in v2[1]: a size that comes from a device or from userspace would have to be checked before every seq_buf_init(), or it becomes a crash under panic_on_warn, and an empty buffer can just hold the empty string. So the accessors do that instead.

[1] https://lore.kernel.org/all/2026091953-cherub-empty-ef35@gregkh/

-Kees

Kees Cook

Hi Kees,

Hmmmm, so IIUC, it is a sentinel value, and not really a buffer size?

Did you consider not holding the size of the buffer, but rather an end pointer? It also allows you to find how much you can write to the buffer, but has slightly better semantics: the end doesn't change if you advance the position. 'end' can only change through reallocations (which I don't know if you do in the kernel).

It's fundamentally the same issue as was discussed with ARRAY_END() vs ARRAY_SIZE(), where ARRAY_END() is more robust. Also, using NULL as a sentinel value is more readable; otherwise, you wonder why a buffer can have 0 bytes.

I think it'd simplify the implementation of the buffer.

Makes sense. Thanks for clarifying!

Have a lovely night!
Alex

<https://www.alejandro-colomar.es>

It's the real size of what's left. seq_buf_get_buf() hands out the unused tail of the seq_buf. Once that has overflowed there is no tail, so it returns NULL for the buffer and 0 for the size, and sched_ext initializes the nested seq_buf with both. So that case does get the NULL; the 0 is how much room it has.

The size doesn't change either: it is set by seq_buf_init(), and writers advance "len", an offset from the start of the buffer. seq_buf never reallocates.

An end pointer wouldn't remove the empty case: buffer == end is the same condition as size == 0, so the accessors would still need the check. It also makes overflow harder to record. seq_buf marks it with len == size + 1, and the matching position pointer, end + 1, would be weird.

Changing the representation would also means changes outside lib/seq_buf.c, since tracing reads seq.len and seq.size directly (trace_seq treats a zero size as "not yet initialized", for one). I'm not saying that's a good thing to bypass the API, but rather that such a change should be its own series.

-Kees

Kees Cook

Hi Kees,

The size changes here:

	$ grepc -h -B8 seq_buf_get_buf include/
	/**
	 * seq_buf_get_buf - get buffer to write arbitrary data to
	 * @s: the seq_buf handle
	 * @bufp: the beginning of the buffer is stored here
	 *
	 * Returns: the number of bytes available in the buffer, or zero if
	 * there's no space.
	 */
	static inline size_t seq_buf_get_buf(struct seq_buf *s, char **bufp)
	{
		WARN_ON(s->len > s->size + 1);

		if (s->len < s->size) {
			*bufp = s->buffer + s->len;
			return s->size - s->len;
		}

		*bufp = NULL;
		return 0;
	}

With an end pointer, that would be simplified to:

	static inline size_t my_seq_buf_get_buf(struct seq_buf *s, char **bufp)
	{
		WARN_ON(s->p >= s->end);

		*bufp = s->p;
		if (s->p != NULL)
			return s->end - s->p;
		return 0;
	}

Moreover, if we change the API of that function, we could entirely get rid of this function. Here's an example where I see seq_buf_get_buf() used, where I've modified it to use pointers:

index 3219f0da0fe4..82904e4456e3 100644
@@ -6893,8 +6893,7 @@ static void scx_dump_cpu(struct scx_sched *sch, struct seq_buf *s,
         * Use a nested seq_buf to generate the standard dump so that we
         * can decide whether to commit later.
         */
-       avail = seq_buf_get_buf(s, &buf);
-       seq_buf_init(&ns, buf, avail);
+       seq_buf_init(&ns, s->p, s->end);
 
        dump_newline(&ns);
        scx_dump_line(&ns, "CPU %-4d: nr_run=%u flags=0x%x cpu_rel=%d ops_qseq=%lu ksync=%lu",

Considering the .p would replace the current .len, and would encode overflow as a NULL pointer, and .end would replace the current .size, and wouldn't ever change (not even on overflow, sice that info would be stored in .p). Now you wouldn't need to test whether the remaining size can be 0, but would instead test whether ->p is NULL (you could do that internally if you want to avoid accessing internals).

The thing is that you'd never reach buffer==end. If that would have to happen, you'd instead store a NULL in buffer. This has the nice property that if you try to read it as a string, it will crash (instead of giving you a pointer that is not a string, which could result in a buffer overrun).

Not really. I'll show an example without the structure; just using plain string-copying functions, which shows it more obviously. The structures are, after all, just an encapsulation of this logic.

With sizes and lengths:

	char    buf[42];
	size_t  len, size;

	len = 0;
	for (int i = 0; i < 10; i++) {
		x = strscpy(buf + len, "foo", size - len);
		if (x < 0)
			goto fail;
		len += x;
	}
vs
	char  buf[42];
	char  *p, *end;

	p = buf;
	e = ARRAY_END(buf);
	for (int i = 0; i < 10; i++)
		p = stpecpy(p, end, "foo");
	if (p == NULL)
		goto fail;

Let's say that each iteration of the loop is a step where we'd inherit an old buffer into a new one with the functions showed above. With a length and a size, you need to update the size. With a position pointer and an end pointer, you only need to update the position pointer, and it encodes the information about the overflow/truncation too. Because truncation is encoded as an invalid position (NULL), we can never recover from that, and thus we don't need to have so many checks in each iteration.

An important detail is that the functions that use such a position pointer should check whether the position is NULL, and do nothing. In this example, stpecpy() has such an early check:

	char *
	stpecpy(char *dst, const char *end, const char *restrict src)
	{
		ssize_t  dlen;

		if (dst == NULL)
			return NULL;

		dlen = strscpy(dst, src, end - dst);
		if (dlen < 0)
			return NULL;

		return dst + dlen;
	}

As you can see, we can never reach a point where p==end, because once truncation happens, p becomes NULL and never recovers from that. This simplifies greatly the handling of truncation.

Agree. I was just mentioning as an idea to keep for the long term.

Have a lovely day!
Alex

<https://www.alejandro-colomar.es>

Seven callers call seq_buf_str() only to NUL-terminate the buffer, discarding the returned pointer. Two of them need a comment to say so.

Add seq_buf_terminate(), wrapping the __seq_buf_terminate() helper that seq_buf_str() and seq_buf_strlen() already use, and convert those callers. It returns void: returning the offset would just be seq_buf_strlen() under another name. A zero-sized seq_buf is left untouched, as in the other accessors.

Add tests for the three cases: room for the NUL after the data, an overflowed buffer where it lands in the last byte, and a zero-sized buffer that must not be written to.

Build tested ARCH=x86_64 defconfig with GCC 16.2.0, plus CONFIG_HIST_TRIGGERS=y and CONFIG_BPF_SYSCALL=y to reach the converted call sites in kernel/trace/trace_events_hist.c and kernel/bpf/diagnostics.c. Tests run 24/24 passing on ARCH=um.

---
 include/linux/seq_buf.h          | 23 +++++++++++++++++++++++
 kernel/bpf/diagnostics.c         |  6 +++---
 kernel/trace/trace_events.c      |  4 ++--
 kernel/trace/trace_events_hist.c |  6 ++----
 lib/tests/seq_buf_kunit.c        | 28 ++++++++++++++++++++++++++++
 5 files changed, 58 insertions(+), 9 deletions(-)

index 87ccc62f1c62..195e612a212a 100644
@@ -171,6 +171,29 @@ static inline size_t seq_buf_strlen(struct seq_buf *s)
 	return __seq_buf_terminate(s);
 }
 
+/**
+ * seq_buf_terminate - NUL-terminate the string in a seq_buf
+ * @s: the seq_buf handle
+ *
+ * Terminate @s->buffer exactly as seq_buf_str() and seq_buf_strlen() do,
+ * for callers that want neither the pointer nor the length and only need
+ * the buffer to be safe to read as a C string. A zero-sized seq_buf has
+ * nowhere to put a NUL and is left untouched.
+ *
+ * Nothing is returned on purpose: a caller that wants the length should
+ * use seq_buf_strlen(), which says so.
+ *
+ * After this function is called, s->buffer is safe to use
+ * in string operations.
+ */
+static inline void seq_buf_terminate(struct seq_buf *s)
+{
+	if (s->size == 0)
+		return;
+
+	__seq_buf_terminate(s);
+}
+
 /**
  * seq_buf_get_buf - get buffer to write arbitrary data to
  * @s: the seq_buf handle

index 0abbbe177e31..594cf3c8b74c 100644
@@ -351,7 +351,7 @@ static void diag_fmt_restore(struct bpf_verifier_env *env, struct diag_fmt_mark
 
 	if (mark.chunk) {
 		mark.chunk->seq.len = mark.len;
-		seq_buf_str(&mark.chunk->seq);
+		seq_buf_terminate(&mark.chunk->seq);
 	}
 }
 

@@ -631,11 +631,11 @@ static void format_disasm_line(struct bpf_verifier_env *env, int insn_idx,
 		return;
 
 	print_bpf_insn(&cbs, insn, env->allow_ptr_leaks);
-	seq_buf_str(&ctx.seq);
+	seq_buf_terminate(&ctx.seq);
 	ctx.seq.len = strnlen(line->text, sizeof(line->text));
 	while (ctx.seq.len && line->text[ctx.seq.len - 1] == '\n')
 		seq_buf_pop(&ctx.seq);
-	seq_buf_str(&ctx.seq);
+	seq_buf_terminate(&ctx.seq);
 
 	line->valid = true;
 }

index 9dbc2441763b..39bb391546de 100644
@@ -4906,7 +4906,7 @@ static __init int event_trace_enable(void)
 	 */
 	__trace_early_add_events(tr);
 
-	seq_buf_str(&bootup_event_seq);
+	seq_buf_terminate(&bootup_event_seq);
 	early_enable_events(tr, bootup_event_buf, false);
 
 	trace_printk_start_comm();

@@ -4935,7 +4935,7 @@ static __init int event_trace_enable_again(void)
 	if (!tr)
 		return -ENODEV;
 
-	seq_buf_str(&bootup_event_seq);
+	seq_buf_terminate(&bootup_event_seq);
 	early_enable_events(tr, bootup_event_buf, true);
 
 	return 0;

index 963e0d6b61fd..57bd1cd5c657 100644
@@ -2988,8 +2988,7 @@ find_synthetic_field_var(struct hist_trigger_data *target_hist_data,
 	seq_buf_init(&s, synthetic_name, MAX_FILTER_STR_VAL);
 	seq_buf_printf(&s, "synthetic_%s", field_name);
 
-	/* Terminate synthetic_name with a NUL. */
-	seq_buf_str(&s);
+	seq_buf_terminate(&s);
 
 	if (seq_buf_has_overflowed(&s)) {
 		kfree(synthetic_name);

@@ -3106,8 +3105,7 @@ create_field_var_hist(struct hist_trigger_data *target_hist_data,
 	if (saved_filter)
 		seq_buf_printf(&s, " if %s", saved_filter);
 
-	/* Terminate cmd with a NUL. */
-	seq_buf_str(&s);
+	seq_buf_terminate(&s);
 
 	if (seq_buf_has_overflowed(&s)) {
 		kfree(cmd);

index 569ab3d96f10..b6fc7b784859 100644
@@ -633,6 +633,33 @@ static void seq_buf_strlen_zero_size_test(struct kunit *test)
 	KUNIT_EXPECT_STREQ(test, buf, "untouched");
 }
 
+static void seq_buf_terminate_test(struct kunit *test)
+{
+	char buf[16];
+	struct seq_buf s;
+
+	/* Terminates directly after the data when there is room. */
+	memset(buf, 'z', sizeof(buf));
+	seq_buf_init(&s, buf, sizeof(buf));
+	seq_buf_puts(&s, "ab");
+	seq_buf_terminate(&s);
+	KUNIT_EXPECT_STREQ(test, buf, "ab");
+
+	/* Terminates in the last byte once the buffer has overflowed. */
+	memset(buf, 'z', sizeof(buf));
+	seq_buf_init(&s, buf, 4);
+	seq_buf_puts(&s, "abcdef");
+	KUNIT_EXPECT_TRUE(test, seq_buf_has_overflowed(&s));
+	seq_buf_terminate(&s);
+	KUNIT_EXPECT_STREQ(test, buf, "abc");
+
+	/* A zero-sized seq_buf is left alone. */
+	strscpy(buf, "untouched", sizeof(buf));
+	seq_buf_init(&s, buf, 0);
+	seq_buf_terminate(&s);
+	KUNIT_EXPECT_STREQ(test, buf, "untouched");
+}
+
 static struct kunit_case seq_buf_test_cases[] = {
 	KUNIT_CASE(seq_buf_init_test),
 	KUNIT_CASE(seq_buf_declare_test),

@@ -656,6 +683,7 @@ static struct kunit_case seq_buf_test_cases[] = {
 	KUNIT_CASE(seq_buf_strlen_puts_overflow_test),
 	KUNIT_CASE(seq_buf_strlen_embedded_nul_test),
 	KUNIT_CASE(seq_buf_strlen_zero_size_test),
+	KUNIT_CASE(seq_buf_terminate_test),
 	KUNIT_CASE(seq_buf_do_printk_test),
 	{}
 };

2.55.0

format_disasm_line() strips trailing newlines from the disassembly it stores, as diag_print_insn_context() adds its own. Since commit 483a1bb0b6cf8 ("bpf: Do not print a newline after disassembly in bpf_verbose_insn()"), print_bpf_insn() does not emit one, so there is nothing left to strip.

The strnlen() resync of the seq_buf length only existed so the loop could index the buffer safely after an overflow, and the second termination only restored the NUL that seq_buf_pop() does not write. Remove all of it, keeping a single seq_buf_terminate().

Build tested ARCH=x86_64 defconfig with GCC 16.2.0 and CONFIG_BPF_SYSCALL=y.

---
 kernel/bpf/diagnostics.c | 4 ----
 1 file changed, 4 deletions(-)

index 594cf3c8b74c..8f64bfd9afee 100644
@@ -632,10 +632,6 @@ static void format_disasm_line(struct bpf_verifier_env *env, int insn_idx,
 
 	print_bpf_insn(&cbs, insn, env->allow_ptr_leaks);
 	seq_buf_terminate(&ctx.seq);
-	ctx.seq.len = strnlen(line->text, sizeof(line->text));
-	while (ctx.seq.len && line->text[ctx.seq.len - 1] == '\n')
-		seq_buf_pop(&ctx.seq);
-	seq_buf_terminate(&ctx.seq);
 
 	line->valid = true;
 }

2.55.0

From: Bill Wendling <[email protected]>

Several strlcat() call sites being converted to seq_buf need behavior seq_buf doesn't currently provide. The normal seq_buf_init() always sets the new buffer size to 0 via seq_buf_clear(). Code migrating from strlcat(buf, ...), which appends to whatever buf already contains, can't use seq_buf_init() without discarding that existing content. Add seq_buf_init_append(), which preserves the existing contents and positions the seq_buf to append after it. A buffer with no NUL within its size starts out overflowed, as strlcat() treats it as already truncated. Add KUnit tests for behavior coverage.

Tests passed under qemu on ARCH=x86_64 with GCC 16.2.0 and CONFIG_KASAN=y, and on big-endian ARCH=s390 with GCC s390x-linux-gnu 16.2.0.

---
 include/linux/seq_buf.h   | 25 +++++++++++++++
 lib/tests/seq_buf_kunit.c | 67 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)

index 195e612a212a..50b1e78eeea6 100644
@@ -71,6 +71,31 @@ seq_buf_set_overflow(struct seq_buf *s)
 	s->len = s->size + 1;
 }
 
+/**
+ * seq_buf_init_append - initialize a seq_buf over a buffer that may
+ *			 already hold NUL-terminated content
+ * @s: the seq_buf handle
+ * @buf: pointer to the (possibly non-empty) buffer
+ * @size: total size of @buf
+ *
+ * Unlike seq_buf_init(), which always starts @buf at len=0, this
+ * preserves whatever NUL-terminated content @buf already holds and
+ * positions @s to append after it. Useful for converting code that used
+ * to append to an existing buffer with strlcat()/scnprintf() and friends.
+ *
+ * If @buf holds no NUL within @size, @s starts out overflowed, as
+ * strlcat() treats such a buffer as already truncated.
+ */
+static inline void
+seq_buf_init_append(struct seq_buf *s, char *buf, unsigned int size)
+{
+	s->buffer = buf;
+	s->size = size;
+	s->len = strnlen(buf, size);
+	if (s->len == size)
+		seq_buf_set_overflow(s);
+}
+
 /*
  * How much buffer is left on the seq_buf?
  */

2.55.0

Yeah, fair. It's not worded clearly. I've fixed the wording for v5 to say seq_buf_init() 'empties the buffer (sets its "len" to 0) via seq_buf_clear()'.

-Kees

Kees Cook

perf_stats_show() and flags_show() build their output with a seq_buf and return seq_buf_used(), which may include the trailing NUL byte when the seq_buf has overflowed. Use seq_buf_strlen() instead. As suggested in review, use it as well for flags_show()'s check whether any flag was printed.

Build tested ARCH=powerpc ppc64_defconfig with GCC powerpc64-linux-gnu 16.2.0:
arch/powerpc/platforms/pseries/papr_scm.o

---
 arch/powerpc/platforms/pseries/papr_scm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 75da96c08cdd..5c3bf06bb59b 100644
@@ -1108,7 +1108,7 @@ static ssize_t perf_stats_show(struct device *dev,
 
 free_stats:
 	kfree(stats);
-	return rc ? rc : (ssize_t)seq_buf_used(&s);
+	return rc ?: (ssize_t)seq_buf_strlen(&s);
 }
 static DEVICE_ATTR_ADMIN_RO(perf_stats);
 

@@ -1147,10 +1147,10 @@ static ssize_t flags_show(struct device *dev,
 	if (health & PAPR_PMEM_SCRUBBED_AND_LOCKED)
 		seq_buf_printf(&s, "scrubbed locked ");
 
-	if (seq_buf_used(&s))
+	if (seq_buf_strlen(&s))
 		seq_buf_printf(&s, "\n");
 
-	return seq_buf_used(&s);
+	return seq_buf_strlen(&s);
 }
 DEVICE_ATTR_RO(flags);
 

2.55.0

flags_show() build their output with a seq_buf and return seq_buf_used(), which may include the trailing NUL byte when the seq_buf has overflowed. Use seq_buf_strlen() instead.

The flag names are far shorter than the PAGE_SIZE buffer sysfs provides, so this cannot overflow today.

Build tested ARCH=x86_64 with GCC 16.2.0, built out of tree with make M=tools/testing/nvdimm:
tools/testing/nvdimm/test/ndtest.o

---
 tools/testing/nvdimm/test/ndtest.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 2051ad5d4882..f097f2992966 100644
@@ -693,7 +693,7 @@ static ssize_t flags_show(struct device *dev,
 	if (seq_buf_used(&s))
 		seq_buf_printf(&s, "\n");
 
-	return seq_buf_used(&s);
+	return seq_buf_strlen(&s);
 }
 static DEVICE_ATTR_RO(flags);
 

2.55.0

The kernel-doc in include/linux/seq_buf.h and lib/seq_buf.c documents the seq_buf interface, but no .rst file pulls either of them in, so none of it reaches the generated documentation.

Add the missing kernel-doc for seq_buf_clear() and seq_buf_init(), and a Sequence Buffers section to the kernel API documentation. The static internal helper seq_buf_can_fit() is left out. Additionally fix seq_buf_hex_dump() indentation to avoid the reported Sphinx error:

ERROR: Unexpected indentation.
WARNING: Block quote ends without a blank line; unexpected unindent.

Verified with "make SPHINXDIRS=core-api htmldocs", which rendered happily into core-api/kernel-api.html.

---
 Documentation/core-api/kernel-api.rst |  9 +++++++++
 include/linux/seq_buf.h               | 12 ++++++++++++
 lib/seq_buf.c                         | 13 +++++++------
 3 files changed, 28 insertions(+), 6 deletions(-)

index 4c4a57c1c094..f5a0aedbbb48 100644
@@ -96,6 +96,15 @@ Error Pointers
 .. kernel-doc:: include/linux/err.h
    :internal:
 
+Sequence Buffers
+----------------
+
+.. kernel-doc:: include/linux/seq_buf.h
+   :internal:
+
+.. kernel-doc:: lib/seq_buf.c
+   :no-identifiers: seq_buf_can_fit
+
 Sorting
 -------
 

index 50b1e78eeea6..c97dd9b0ba53 100644
@@ -31,6 +31,10 @@ struct seq_buf {
 		.size = SIZE,				\
 	}
 
+/**
+ * seq_buf_clear - reset the seq_buf to be read / appended from the beginning
+ * @s: the seq_buf handle
+ */
 static inline void seq_buf_clear(struct seq_buf *s)
 {
 	s->len = 0;

@@ -38,6 +42,14 @@ static inline void seq_buf_clear(struct seq_buf *s)
 		s->buffer[0] = '\0';
 }
 
+/**
+ * seq_buf_init - initialize a seq_buf
+ * @s: the seq_buf handle
+ * @buf: pointer to the buffer
+ * @size: total size of @buf
+ *
+ * The contents of the buffer are ignored.
+ */
 static inline void
 seq_buf_init(struct seq_buf *s, char *buf, unsigned int size)
 {

index 8da2e9447adf..54b76044e4ba 100644
@@ -409,12 +409,13 @@ int seq_buf_to_user(struct seq_buf *s, char __user *ubuf, size_t start, int cnt)
  *
  * Function is an analogue of print_hex_dump() and thus has similar interface.
  *
- * linebuf size is maximal length for one line.
- * 32 * 3 - maximum bytes per line, each printed into 2 chars + 1 for
- *	separating space
- * 2 - spaces separating hex dump and ASCII representation
- * 32 - ASCII representation
- * 1 - terminating '\0'
+ * linebuf size is maximal length for one line::
+ *
+ *	32 * 3 - maximum bytes per line, each printed into 2 chars + 1 for
+ *		 separating space
+ *	2 - spaces separating hex dump and ASCII representation
+ *	32 - ASCII representation
+ *	1 - terminating '\0'
  *
  * Returns: zero on success, -1 on overflow.
  */

2.55.0

For bpf bits: