Re: [RFC PATCH 00/39] sysctl: replace overloaded extra pointers with typed bounds and context
From: Wen Yang
Date: Tue Nov 10 2026 - 12:49:08 EST
On 11/9/26 18:07, Joel Granados wrote:
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:
<--- snip --->
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.
(~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.
Hi Joel, David,
That would mean changing all the proc handler signature. Right?
Thanks. With David's stride approach, the signature could stay unchanged:
struct nf_log_sysctl_entry {
struct ctl_table ct;
struct net *net;
int family;
};
static int nf_log_handler(const struct ctl_table *table, int write,
void *buffer, size_t *lenp, loff_t *ppos)
{
const struct nf_log_sysctl_entry *entry =
container_of(table, struct nf_log_sysctl_entry, ctl);
return nf_log_common(table, entry->net, entry->family,
write, buffer, lenp, ppos);
}
Proposed regisger API, not implemented yet:
regisge_net_sysctl_stride(net, "net/netfilter/nf_log",
entries, ARRAY_SIZE(entries),
sizeof(entries[0]));
If that works it would allow the per-value function use container_of()Would this mean that the callers would have to have to keep track of the
to get extra fields.
ctl_table offsets? in other words the sysctl indices and the caller's
indices would have to be synced. right?
static const struct ctl_table *
sysctl_entry_at(const struct ctl_table_header *head, size_t i)
{
return (const struct ctl_table *)
((const char *)head->ctl_table +
i * head->ctl_table_stride);
}
Table walks would retain the index:
for (i = 0; i < head->ctl_table_size; i++) {
entry = sysctl_entry_at(head, i);
node = &head->node[i];
...
}
Insertion/deletion would take the node directly. Readdir would retain
its RB-tree node cursor, recovering each entry through that node's header:
entry = sysctl_entry_at(node->header, node - node->header->node);
No entry - head->ctl_table, caller slot constants or per-slot wrappers.
RB-tree order remains unchanged. Old APIs and generated directory/link
tables use sizeof(struct ctl_table).
The two mechanisms have existing precedents:
/* include/linux/bsearch.h: explicit element size */
pivot = base + (num >> 1) * size;
result = cmp(key, pivot);
/* include/linux/hwmon-sysfs.h: extended descriptor */
struct sensor_device_attribute {
struct device_attribute dev_attr;
int index;
};
/* drivers/hwmon/adcxx.c: ordinary device_attribute callback */
const struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
hwmon registers &ad_input[i].dev_attr via device_create_file(); the core
passes that same base descriptor to the callback. These are precedents
for explicit-size traversal and embedded callback context separately,
not an existing strided sysctl registration API.
I agree that my fixed-slot solution adds maintenance overhead. Would
it make sense to explore this in three small steps?
1. Entry-access helpers, unchanged packed layout.
2. Header stride and compatible registration wrappers.
3. nf_log conversion and code/memory comparison.
I would appreciate your thoughts on whether this is worth pursuing.
Thanks,
Wen
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