Re: [RFC v1] seq_buf: Add seq_buf_create() to implement DECLARE_SEQ_BUF()

[Alejandro Colomar <alx@kernel.org>]

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

Hi Steve,

> Date: 2026-10-29 10:28:30-0400
> From: Steven Rostedt <rostedt@goodmis.org>
>
> On Thu, 29 Oct 2026 15:15:29 +0100
> Alejandro Colomar <alx+linux-hardening@kernel.org> wrote:
> 
> > > Honestly, you haven't convinced me that the pointer logic is any better. It
> > > looks to me that it may be just be moving the complexity elsewhere.  
> > 
> > The complexity doesn't move elsewhere, IMO.  It has an issue with file-
> > scope initialization, indeed, because derived pointers cannot be
> > constant expressions, and thus this precise problem, but other than
> > that, there's no off-by-one-prone edges in that design.
> 
> If you have to change the current API to satisfy the constraints of a new
> implementation, that is moving the complexity.

It adds some small churn, but I wouldn't call that "complexity", given
that it actually reduces the complexity of the callers too.

> > However, I understand the fear, so I'll try to prove the simplicity
> > before asking you to trust me about it.  (I guess by doing a full
> > conversion, and showing that the diff reduces code without showing
> > corner cases like the bug we're discussing at the moment in the other
> > thread.)
> > 
> > > Yes, using indexes can be error prone, but if you keep all that abstracted
> > > out to make sure you can validate that it works, it should be fine. Nothing
> > > that uses seq_buf should care about the internals.  
> > 
> > Actually, there are a few places where those internals somewhat already
> > escape the API.  For example:
> > 
> > 	avail = seq_buf_get_buf(s, &buf);
> > 	seq_buf_init(&ns, buf, avail);
> 
> How is this exposing the internals? It's an API and it only modifies what
> the user has control of (the buffer supplied in the first place).

It would be better to not 

> > could be rewritten to use a single function (we can already do that):
> > 
> > 	seq_buf_get_tail(&ns, s);
> > 
> > This way, we don't need to have a temporary 'char *buf' that we don't
> > really need in the caller, and we also avoid inspecting the available
> > size.
> 
> What happens if there's no size left?

More or less the same as now, I think.

Since I don't have a tree with the recent patches from Kees, I don't
know if that has changed recently, but from what's in v7.3, I think
it will behave as this:

-  If the buffer has already overflowed (len>size+1), it will WARN().
   Then, it will produce a null pointer, and a size of 0.

-  If the buffer is exactly full, then it will also produce a null
   pointer and a size of 0, but without WARN().

With pointers, it could do something like this:

	static inline void
	seq_buf_get_tail(struct seq_buf *tail, const struct seq_buf *s)
	{
		if (seq_buf_buffer_left(s)) {
			tail->buffer = s->p;
			tail->p = s->p;
			tail->end = s->end;
		} else {
			WARN_ON(seq_buf_has_overflowed(s));
			tail->buffer = (static char[1]){""};
			tail->p = NULL;
			tail->p = tail->buffer + 1;
		}
	}

Which gives you an overflowed buffer, which is safe to hold.  One can
read it, and will read an empty string.  Any attempts to write to it
would be no-ops, since it's already overflowed.

> > This could be done already with this:
> > 
> > 	diff --git i/include/linux/seq_buf.h w/include/linux/seq_buf.h
> > 	index f006f2654e4c..f7ee062374bf 100644
> > 	--- i/include/linux/seq_buf.h
> > 	+++ w/include/linux/seq_buf.h
> > 	@@ -58,6 +58,16 @@ seq_buf_init(struct seq_buf *s, char *buf, unsigned int size)
> > 		seq_buf_clear(s);
> > 	 }
> > 	 
> > 	+static inline void
> > 	+seq_buf_get_tail(struct seq_buf *tail, const struct seq_buf *s)
> > 	+{
> > 	+       char *p;
> > 	+       size_t size;
> > 	+
> > 	+       size = seq_buf_get_buf(s, &p);
> > 	+       seq_buf_init(tail, p, size);
> > 	+}
> > 	+
> > 	 /*
> > 	  * seq_buf have a buffer that might overflow. When this happens
> > 	  * len is set to be greater than size.
> > 
> > > Using indexes should not
> > > be a point of failure as it's not extended outside the scope of the
> > > internals of seq_buf.  
> > 
> > Except that many places do look at the size by looking at the return
> > value of things like seq_buf_get_buf().
> 
> That's not the internals. And your pointer algorithm had better supply the
> same API.

The pointer algorithm presents the remaining size with
seq_buf_buffer_left(), but doesn't expose it when getting the tail of
a buffer, because I didn't find it necessary.  We could expose it there
too, though.

> The amount of buffer left has nothing to do with the
> implementation of seq_buf.
> 
> > 
> > > That is, once you prove the internals of seq_buf is correct, then you are done.  
> > 
> > And also except that you still need to prove the internals of seq_buf,
> > which aren't exactly trivial.  With pointers, those internals become
> > simpler.
> > 
> > I have a draft for moving to pointers, and it cleans the implementation
> > quite a lot:
> > 
> > 	 include/linux/seq_buf.h |  86 +++++++++++----------------
> > 	 lib/seq_buf.c           | 101 ++++++++++----------------------
> > 	 2 files changed, 66 insertions(+), 121 deletions(-)
> > 
> > (It's far from ready.)
> 
> Don't bother. I don't think you understand the usage of seq_buf.

I'm certainly not understanding all the use cases of it.  I'm just
seeing it being used to replace snprintf(3) and strscpy(9) calls, in
theory to make them safer, and I'm doubting that will really improve
safety.  For this use case, a pointer-based API is much safer and
simpler, and I'd entirely avoid seq_buf.  That's why I'm trying to learn
seq_buf and see if it can be changed in that way.


Cheers,
Alex

> I also
> already don't like the changes and I don't see why they are even necessary.
> 
> -- Steve
> 

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

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