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

[David Laight]

On Thu, 29 Oct 2026 00:34:40 +0800
wen.yang@xxxxxxxxx wrote:

> To: Joel Granados <joel.granados@xxxxxxxxxx>
> Cc: Wen Yang <wen.yang@xxxxxxxxx>,  linux-fsdevel@xxxxxxxxxxxxxxx,  linux-kernel@xxxxxxxxxxxxxxx
> Subject: [RFC PATCH 01/39] sysctl: replace overloaded extra pointers with typed bounds and context
> Date: Thu, 29 Oct 2026 00:34:40 +0800
> 
> From: Wen Yang <wen.yang@xxxxxxxxx>
> 
> struct ctl_table has two untyped misc pointers, extra1 and extra2, whose
> meaning depends on the handler that reads them. In-tree use has grown to
> cover 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 and
> writers have to infer it from the handler, and the core itself gets it
> wrong where it treats extra1 as an unsigned int.
> 
> The overloading is also a memory-safety hazard. An entry that stores
> handler context in data cannot be handed to a standard handler without
> first restoring the real storage, because proc_dointvec() writes through
> data. Every call site has to remember the restore, and one that forgets
> 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.
> 
> Replace both jobs with two fields that cannot be confused:
> 
>   - min/max hold numeric bounds by value. Signed handlers read them as
>     signed longs; an absent endpoint is the type limit, not NULL, so zero
>     stays a real bound. A range that changes at run time is refreshed
>     before every call instead of being snapshotted at registration.
> 
>   - priv holds handler context. The core never reads it, standard handlers
>     never touch it, and registration rejects it on a standard handler so
>     context cannot be silently ignored. data and maxlen keep the meaning
>     their handler gives them, including proc_dostring() name lengths and
>     proc_do_large_bitmap() bit counts.
> 
> The range converters read min/max whenever extra1 and extra2 are both
> clear, and keep reading the pointers while they are set. That lets callers
> migrate one subsystem at a time without renaming handlers or adding a
> parallel API: the existing proc_*_minmax entry points keep their names and
> gain numeric semantics as entries drop their extras. Entries that already
> relied on NULL meaning "no bound" now state their type limits explicitly,
> so their behaviour does not change and the meaning of zero stays
> unambiguous.
> 
> This patch is the refactor itself. The next patch converts the debug test
> module to numeric bounds as the first user, and the KUnit suite after it
> makes the contract executable; everything after those is migration of
> existing users and adds no new semantics: fixed bounds become numeric
> values, run-time bounds are refreshed per call, context moves off data, and
> the extra pointers and the compatibility read are removed once the last
> user is gone. Owners that can still be recovered from data keep using
> container_of(); priv is only for entries where no derivation exists.
> 
> Replacing the two pointers with unsigned long min/max follows Eric
> Biederman's suggestion; priv is added here because the same pair of fields
> also carries handler context, which a numeric bound cannot express.
> 
> Suggested-by: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> Suggested-by: Joel Granados <joel.granados@xxxxxxxxxx>
> Signed-off-by: Wen Yang <wen.yang@xxxxxxxxx>
...
> 
> diff --git a/arch/powerpc/platforms/pseries/mobility.c b/arch/powerpc/platforms/pseries/mobility.c
> index b5c2abd12432..294897f3fbb5 100644
> --- a/arch/powerpc/platforms/pseries/mobility.c
> +++ b/arch/powerpc/platforms/pseries/mobility.c
> @@ -10,6 +10,7 @@
>  #define pr_fmt(fmt) "mobility: " fmt
>  
>  #include <linux/cpu.h>
> +#include <linux/limits.h>
>  #include <linux/kernel.h>
>  #include <linux/kobject.h>
>  #include <linux/nmi.h>
> @@ -61,6 +62,8 @@ static const struct ctl_table nmi_wd_lpm_factor_ctl_table[] = {
>  		.maxlen		= sizeof(int),
>  		.mode		= 0644,
>  		.proc_handler	= proc_douintvec_minmax,
> +		.min		= 0,
> +		.max		= UINT_MAX,
>  	},

You could save a lot of churn by not adding the '.min = 0' line
and making the function treat the 'silly' case where both min and max
are zero as meaning 'no upper bound'.

As well as saving a lot of diffs, it will be less error prone.

David