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

[David Laight]

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?

David