Re: [PATCH v4 05/11] seq_buf: Add seq_buf_strlen()

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6277 bytes --]

Hi Kees,

> Date: 2026-10-11 17:56:27-0700
> From: Kees Cook <kees@kernel.org>
>
> On Mon, Oct 05, 2026 at 06:43:23PM +0200, Alejandro Colomar wrote:
> > > 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).
> >
> > Hmmmm, so IIUC, it is a sentinel value, and not really a buffer size?
> 
> 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.
> 
> > 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).
> 
> 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.

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:

diff --git i/kernel/sched/ext/ext.c w/kernel/sched/ext/ext.c
index 3219f0da0fe4..82904e4456e3 100644
--- i/kernel/sched/ext/ext.c
+++ w/kernel/sched/ext/ext.c
@@ -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).

> An end pointer wouldn't remove the empty case: buffer == end is the same

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

> 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.

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.

> 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.

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


Have a lovely day!
Alex

> 
> -Kees
> 
> -- 
> Kees Cook

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]