Re: [RFC PATCH 00/39] sysctl: replace overloaded extra pointers with typed bounds and context

[Joel Granados]

On Sat, Nov 07, 2026 at 05:05:07PM +0000, David Laight wrote:
> On Sat, 7 Nov 2026 16:27:39 +0100
> Joel Granados <joel.granados@xxxxxxxxxx> wrote:
> 
> > On Mon, Nov 02, 2026 at 11:40:40PM +0800, Wen Yang wrote:
> > > 
> > > 
> > > On 10/29/26 18:58, David Laight wrote:  
> > > > On Thu, 29 Oct 2026 00:34:39 +0800
> > > > wen.yang@xxxxxxxxx wrote:
> > > >   
> > > > > From: Wen Yang <wen.yang@xxxxxxxxx>
> > > > > 
> > > > > struct ctl_table describes an entry with data, maxlen and, until this
> > > > > series, two untyped misc pointers in extra1 and extra2. Those two pointers
> > > > > have accumulated at least seven unrelated jobs: a fixed bound, a bound read
> > > > > through a pointer on every call, an object or namespace pointer, a small
> > > > > integer cookie encoded into the pointer, a lock signal, NULL standing for
> > > > > "no bound", and a value the core itself casts to unsigned int while
> > > > > validating a u8 range. Nothing in the entry says which job applies, so
> > > > > readers infer it from the handler, and the core gets it wrong where it
> > > > > treats extra1 as an unsigned int.  
> > <--- snip --->
> > > > > (~574 new min/max initializers) instead of pointing at constants. The
> > > > > sysctl ABI is unchanged; the parport spintime minimum is corrected as
> > > > > described above.
> > > > > 
> > > > > The cost is 8 bytes per ctl_table entry on 64-bit (56 -> 64) and 4 bytes on
> > > > > 32-bit, because priv is appended; the range read path gains no indirection
> > > > > and the migrated entries no longer dereference a bound pointer.  
> > > > 
> > > > Can you remove the little-used 'priv' parameter by making the extra
> > > > information just follow the table entry (nested structure) and have
> > > > the relevant functions use container_of() to get the outer structure
> > > > and extra values?
> > > >   
> > > 
> > > 
> > > Hi David, Hi Joel,
> > > 
> > > I reworked the context side of the series so that priv is gone entirely;
> > > and the final tree has no per-entry context pointer.
> > > A small helper recovers the enclosing object, but it needs the slot
> > > index from the caller:
> > > 
> > >   #define sysctl_table_container(entry, type, member, index) ({       \
> > >           static_assert(__same_type(*(entry),                         \
> > >                                     typeof_member(type, member[0])),  \
> > >                         "pointer type mismatch");                     \
> > >           (type *)((void *)(entry) - offsetof(type, member) -         \
> > >                    (size_t)(index) * sizeof(*(entry)));               \
> > >   })
> > > 
> > > The reason plain container_of() is not enough is that the core keeps the
> > > table as a contiguous array.  For
> > > 
> > >   struct addrconf_sysctl_table {
> > >           struct addrconf_sysctl_ctx context;
> > >           struct ctl_table table[];
> > >   };
> > > 
> > > the handler receives an interior entry, and
> > > 
> > >   container_of(ctl, struct addrconf_sysctl_table, table)
> > >       == (char *)outer + index * sizeof(struct ctl_table)
> > > 
> > > for ctl == &outer->table[index].  It only returns the outer object for
> > > index 0.  Passing the index explicitly keeps the original registered
> > > entry usable:
> > > 
> > >   static int addrconf_sysctl_forward(const struct ctl_table *ctl, ...)
> > >   {
> > >           struct addrconf_sysctl_table *t =
> > >                   sysctl_table_container(ctl, struct addrconf_sysctl_table,
> > >                                          table, ADDRCONF_SYSCTL_FORWARDING);
> > >           const struct addrconf_sysctl_ctx *ctx = &t->context;
> > >           ...
> > >   }
> > > 
> > > Shared handlers get one wrapper per slot; the common body does not need
> > > to know its position:
> > > 
> > >   #define DEVINET_CONF_WRAPPER(attr)                                  \
> > >   static int devinet_conf_##attr(const struct ctl_table *ctl, ...)    \
> > >   {                                                                   \
> > >           struct devinet_sysctl_table *t =                            \
> > >                   sysctl_table_container(ctl,                         \
> > >                           struct devinet_sysctl_table, devinet_vars,  \
> > >                           IPV4_DEVCONF_##attr - 1);                   \
> > >           return devinet_conf_proc_common(ctl, &t->context, ...);     \
> > >   }  
> > 
> > Two questions before seeing the actual implementation of this.
> > 
> > 1. This means that we will have to add more macros to the sysctl
> >    callers? There is a clear signal in [1] against overuse of macros.
> > 2. I understand that you are doing this to avoid having priv. Is it to
> >    save memory?  Yes. priv is a void* and we are also trying to get rid
> >    of these. But this changes removes the use of two void pointers and
> >    replaces it with just one.
> 
> The idea behind removing the 'void *priv' is mostly to save memory.
> There are a lot of systctl and hardly any need it.
> 
> This solution does look nasty.
> I suggested another where the size of the array entries was passed in
> by the caller.
That would mean changing all the proc handler signature. Right?

> If that works it would allow the per-value function use container_of()
> to get extra fields.
Would this mean that the callers would have to have to keep track of the
ctl_table offsets? in other words the sysctl indices and the caller's
indices would have to be synced. right? This goes a bit against the
second point in [1]. Which I'm still willing to live with, but worth
avoiding if it can be done.

Best

[1] https://lore.kernel.org/atoDngZsxy3TWmO_@hobbes


> That only really helps if only a few 'private' functions need it.
> 
> David
> 



> > 
> > Best
> > 
> > [1] https://lore.kernel.org/CAHk-=wjUmLrJd4KM4xvJHUN4P0r8oSu5LyvG4JipnhxizBrE9Q@xxxxxxxxxxxxxx
> > > 
> > > Single-entry tables simply use slot 0:
> > > 
> > >   struct devinet_forward_table *t =
> > >           sysctl_table_container(ctl, struct devinet_forward_table, table,
> > > 0);
> > > 
> > > 
> > > The series is 20 commits over 134 files, with no priv at any point.  If
> > > you would prefer a different shape for the helper or the wrappers, I am
> > > happy to adjust it.
> > > 
> > > 
> > > Thanks,
> > > Wen
> > >   
> > > > David
> > > >   
>

Attachment: signature.asc
Description: PGP signature