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

[David Laight]

On Mon, 2 Nov 2026 23:40:40 +0800
Wen Yang <wen.yang@xxxxxxxxx> 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.
> >>
> >> The numeric half of this change is the direction Eric Biederman asked for:
> >>
> >>    You can still save a lot more by turning .extra1 and .extra2
> >>    into longs instead of keeping them as pointers and needing
> >>    constants to be pointed at somewhere.
> >>
> >>    [...] I don't see any reason why you want shared
> >>    constants for such a handful of things.  Especially when
> >>    it has proven to be error prone in the past.
> >>
> >> min/max are plain unsigned long members, so the comparisons are ordinary
> >> C and cannot be confused with a pointer. The shared const arrays that only
> >> existed to give those pointers an address (sysctl_vals and
> >> sysctl_long_vals) are removed as well.
> >>
> >> The tree already records this direction. fs/proc/proc_sysctl.c documents
> >> extra1/extra2 as "extra pointers usable by the proc handler routines" and
> >> carries:
> >>
> >>    XXX: we should eventually modify these to use long min / max [0]
> >>    [0] https://lkml.kernel.org/87zgpte9o4.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> >>
> >> This series completes that TODO and removes the comment.
> >>
> >> priv is this series' addition: the same pair of fields also hides handler
> >> context, which a numeric bound cannot express, and keeping it out of data
> >> is what removes the restore-before-delegate pattern.
> >>
> >> The overloading is also a memory-safety hazard. The obvious alternative --
> >> keep handler context in data and restore the value pointer in each handler
> >> -- needs an original_data field in nine addrconf and two devinet handlers;
> >> one missed restore turns a user-supplied integer into a write over a kernel
> >> pointer. nf_log reaches the same problem from the other side: it replaces
> >> data with a local buffer inside a single handler, so data means two things
> >> within one function. This revision removes that temptation instead of
> >> managing it.
> >>
> >> The core is not exempt. sysctl_check_table_array() reads
> >> *(unsigned int *)table->extra1 to validate a u8 range. That is only correct
> >> while every u8 entry happens to point extra1/extra2 at an int or unsigned
> >> int; a long bound, a context pointer or an encoded integer would be read as
> >> four bytes of the wrong object. No such entry exists today, so this is a
> >> latent class rather than a live bug, but the invariant is untyped and
> >> unchecked, and nf_log already encodes an integer into extra1.
> >>
> >> The refactor is the first five patches; they define the replacement
> >> contract and prove it covers every entry shape the tree uses today.
> >>
> >>    1. min/max hold numeric bounds by value, and priv holds handler context.
> >>       The range converters read min/max whenever extra1 and extra2 are
> >>       clear and keep reading the pointers while they are set, so existing
> >>       callers keep working while they migrate and no parallel handler API or
> >>       renamed symbol is introduced. Signed handlers read the bounds as
> >>       signed longs, an absent endpoint is the type limit rather than NULL so
> >>       zero stays a real bound, and a run-time range is refreshed before
> >>       every call. Registration rejects priv on a standard handler.
> >>
> >>    2. The debug test module is converted to numeric bounds as the first
> >>       user.
> >>
> >>    3. KUnit coverage for numeric bounds and the handlers that consume them:
> >>       full type ranges, one-sided and zero bounds, entries without backing
> >>       storage, explicit handlers for arrays and members, and rejection of
> >>       writes that would partially commit.
> >>
> >>    4. KUnit coverage for run-time ranges across the scalar widths and
> >>       jiffies, run-time bound pointers, sibling bounds, typed context, owner
> >>       derivation, bool, registration lifecycles, and simulated per-net
> >>       duplication.
> >>
> >>    5. KUnit coverage for priv: a standard handler must ignore it,
> >>       registration must reject it with a standard handler, and
> >>       re-registration must keep both the value pointer and the context. The
> >>       same patch moves the debug module's dynamic-value entry to the
> >>       data/priv shape.
> >>
> >> On the CTLTBL_ENTRY_* constructors: I kept the naming and argument order you
> >> suggested while reworking the series. As it turned out, no production table
> >> needed them -- the converted entries read naturally as designated
> >> initializers -- so I have not introduced them as a public API in this
> >> version; the KUnit file keeps an equivalent constructor privately, so the
> >> tests stay readable. I still think the public version is worth doing, and I
> >> would be happy to send it as a follow-up once there are real users, so the
> >> API and its callers can be reviewed together.
> >>
> >> Everything after patch 5 is migration and adds no new contract. Patches
> >> 6-32 convert the remaining users and derive owners from their backing
> >> storage. Patches 33-38 move handler context onto priv, one subsystem per
> >> patch; each of those files is touched exactly once and goes straight from
> >> the old pointer field to priv, with no intermediate data-as-context step.
> >> Patch 39 removes the extra pointers and the compatibility read.
> >>
> >> The parport conversion illustrates a concrete problem with the overloaded
> >> fields. In the base tree, registration overwrites the spintime minimum
> >> pointer in vars[0].extra1 with a port pointer. The range handler can then
> >> check a write against data from struct parport instead of the intended
> >> minimum. Moving the port context to priv and using numeric bounds also
> >> corrects this existing bug. A separate fix can address it independently;
> >> this RFC does not depend on that fix.
> >>
> >> Entries that used a *_minmax handler with neither extra pointer relied on
> >> NULL meaning "no bound". They now state their type limits explicitly to
> >> preserve that behaviour when the pointer path disappears. These include
> >> aio-max-nr, acpi_video_flags and nmi_wd_lpm_factor.
> >>
> >> Owners that can still be recovered from data are deliberately left alone:
> >> ipvs, neighbour, the fragment reassembly handlers, vsock, mptcp, rds and
> >> smc keep using container_of() on their backing storage. priv is only for
> >> entries where no such derivation exists. Mixed placement in data and priv
> >> was considered and rejected because it recreates the guessing problem this
> >> series removes.
> >>
> >> Relative to the base tree, the final state removes all 841 extra1/extra2
> >> assignments across 123 files, removes the shared sysctl_vals and
> >> sysctl_long_vals arrays (21 references), and states bounds at each entry
> >> (~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, ...);     \
>    }
> 
> 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.

Certainly worth thinking about for a followup patch.

A different option would be to pass the array element size when adding
the items.
Most code would just pass an array of 'struct ctl_table' but instead
you could pass an array of a larger structure that started with a
struct ctl_table.
That should let you use container_of().
That might even make some of these patches shorter/simpler (I just looked
at #33 (ipv6 addresses) and it is somewhat convoluted.

David

> 
> 
> Thanks,
> Wen
> 
> > David
> >