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

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

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

Hi Steve,

> Date: 2026-10-29 09:41:56-0400
> From: Steven Rostedt <rostedt@goodmis.org>
>
> On Thu, 29 Oct 2026 12:33:19 +0100
> Alejandro Colomar <alx+linux-hardening@kernel.org> wrote:
> 
> > > > diff --git a/include/linux/seq_buf.h b/include/linux/seq_buf.h
> > > > index 9f2839e73f8a..facc5643c049 100644
> > > > --- a/include/linux/seq_buf.h
> > > > +++ b/include/linux/seq_buf.h
> > > > @@ -25,10 +25,7 @@ struct seq_buf {
> > > >  };
> > > >  
> > > >  #define DECLARE_SEQ_BUF(NAME, SIZE)			\
> > > > -	struct seq_buf NAME = {				\
> > > > -		.buffer = (char[SIZE]) { 0 },		\
> > > > -		.size = SIZE,				\
> > > > -	}
> > > > +	struct seq_buf NAME = seq_buf_create((char[SIZE]){0}, SIZE)  
> > > 
> > > It can't use the DECLARE_* name as that is by default something that
> > > can be used outside of a function:
> > > 
> > >   DECLARE_SEQ_BUF(foo, 1024);
> > > 
> > > Needs to work. Just because nothing does that now, doesn't mean it will
> > > not happen in the future. From my understanding, all DECLARE_* macros
> > > for other types work this way.  
> > 
> > Hmmmm, then I would like to remove this macro entirely.  It is not
> > possible to implement it for a structure that has '.p' and '.end'
> > without a function call.
> > 
> > It would only be implementable with constexpr (or constinit? C++ has too
> > many constsomething things that slightly differ, and I don't know which
> > is which) functions that can be called at file scope, but those don't
> > exist in C.
> > 
> > Would it be okay for you to remove this, and perhaps replace it with
> > direct calls to seq_buf_create()?  Maybe like this:
> > 
> > 	#define SEQ_BUF_CREATE(SIZE)  seq_buf_create((char[SIZE]){0}, SIZE)
> 
> No.
> 
> 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.

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

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.

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


Cheers,
Alex

> 
> -- Steve
> 
> 
> > 
> > 	...
> > 
> > 	int
> > 	main(void)
> > 	{
> > 		struct seq_buf foo = SEQ_BUF_CREATE(1024);
> > 	}
> 

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

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