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

[Wen Yang <wen.yang@linux.dev>]

On 10/29/26 18:58, David Laight wrote:
> On Thu, 29 Oct 2026 00:34:39 +0800
> wen.yang@linux.dev wrote:
> 
>> From: Wen Yang <wen.yang@linux.dev>
>>
>> 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@email.froward.int.ebiederm.org
>>
>> 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.


Thanks,
Wen

> David
>