Re: [PATCH v5] sysctl: simplify the min/max boundary check

From: Wen Yang
Date: Thu Mar 06 2025 - 08:34:08 EST



On 2025/3/3 17:26, Joel Granados wrote:
On Thu, Jan 30, 2025 at 10:32:14PM +0800, Wen Yang wrote:


On 2025/1/28 01:51, Eric W. Biederman wrote:
Joel Granados <joel.granados@xxxxxxxxxx> writes:

On Thu, Jan 23, 2025 at 12:30:25PM -0600, Eric W. Biederman wrote:
"Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> writes:

Joel Granados <joel.granados@xxxxxxxxxx> writes:

On Sun, Jan 19, 2025 at 10:59:21PM +0800, Wen Yang wrote:
...
struct ctl_table {
const char *procname; /* Text ID for /proc/sys */
void *data;
int maxlen;
umode_t mode;
proc_handler *proc_handler; /* Callback for text formatting */
struct ctl_table_poll *poll;
unsigned long min;
unsigned long max;
} __randomize_layout;

That is just replace extra1 and extra2 with min and max members. The
members don't have any reason to be pointers. Without being pointers
the min/max functions can just use long values to cap either ints or
longs, and there is no room for error. The integer promotion rules
will ensure that even negative values can be stored in unsigned long
min and max values successfully. Plus it is all bog standard C
so there is nothing special to learn.

There are a bunch of fiddly little details to transition from where we
are today. The most straightforward way I can see of making the
transition is to add the min and max members. Come up with replacements
for proc_doulongvec_minmax and proc_dointvec_minmax that read the new
min and max members. Update all of the users. Update the few users
that use extra1 or extra2 for something besides min and max. Then
remove extra1 and extra2. At the end it is simpler and requires the
same or a little less space.

That was and remains my suggestion.


Thanks for your valuable suggestions. We will continue to move forward along
it and need your more guidance.

But there are also a few codes that do take the extra{1, 2} as pointers, for
example:

int neigh_sysctl_register(struct net_device *dev, struct neigh_parms *p,
proc_handler *handler)
{
...
for (i = 0; i < NEIGH_VAR_GC_INTERVAL; i++) {
t->neigh_vars[i].data += (long) p;
t->neigh_vars[i].extra1 = dev;
t->neigh_vars[i].extra2 = p;
}
...
}

static void neigh_proc_update(const struct ctl_table *ctl, int write)
{
struct net_device *dev = ctl->extra1;
struct neigh_parms *p = ctl->extra2;
struct net *net = neigh_parms_net(p);
int index = (int *) ctl->data - p->data;
...
}
Quick question: Do you have a systemic way of identifying these? Do you
have a grep or awk scripts somewhere? I'm actually very interested in
finding out what is the impact of this.


Thanks, we may use the following simple scripts:

- the extra {1,2} as pointers to some objects:
$ grep "\.extra1\|\.extra2" * -R | grep -v "SYSCTL_" | grep -v "\&"

- the extra {1,2} as pointers to elements in the shared constant array:
$ grep "\.extra1\|\.extra2" * -R | grep "SYSCTL_"

- the extra {1,2} as pointers to additional constant variables:
$ grep "\.extra1\|\.extra2" * -R | grep "\&"


--
Best wishes,
Wen






So could we modify it in this way to make it compatible with these two
situations:

@@ -137,8 +137,16 @@ struct ctl_table {
umode_t mode;
proc_handler *proc_handler; /* Callback for text formatting */
struct ctl_table_poll *poll;
- void *extra1;
- void *extra2;
+ union {
+ struct {
+ void *extra1;
+ void *extra2;
+ };
+ struct {
+ unsigned long min;
+ unsigned long max;
+ };
+ };
} __randomize_layout;


--
Best wishes,
Wen