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

[Wen Yang]

On 2024/12/18 22:11, Joel Granados wrote:
> On Sun, Dec 01, 2024 at 10:00:58PM +0800, Wen Yang wrote:
> > The do_proc_dointvec_minmax_conv_param structure provides the minimum and
> > maximum values for doing range checking for the proc_dointvec_minmax()
> > handler, however min/max is a pointer and may be NULL, so the following
> > code snippet appears multiple times:
> >          if ((param->min && *param->min > tmp) ||
> >              (param->max && *param->max < tmp))
> >
> > And int types also have min/max values, so when the pointer is NULL,
> > explicitly setting min/max to INT_{MIN/MAX} could simplify the code a bit.
> Sorry, but this does not make sense to me. This simplification is way
> too small and it seems that it is just being done for the sake of it.
> Additionally, by giving these min/max values you are potentially
> changing the behaviour of existing calls, which is concerning for
> such a small change/gain.

Thanks for your comments.

The implementation of do_proc_dointvec_conv() utilizes the default range 
of the int type, while do_proc_dointvec_minmax_conv() additionally 
utilizes min/max pointers, which are actually table->extra {1,2} 
pointers passed in.

If we can dereference the table->extra {1,2} pointers to numerical 
values in advance (such as the modification here), we can take advantage 
of memory locality and improve performance a bit.

If the current simplification is too small, we could further improve it, 
such as considering do_proc_minmax_conv_param, 
do_proc_minmax_conv_param, do_proc_dointvec_minmax_conv, 
do_proc_douintvec_conv, do_proc_douintvec_minmax_conv , etc.

All of this is in preparation for ultimately killing the table 
->{extra1, extra2} pointers.

We will rework later and send v5.

--
Best wishes，
Wen


> Thx for the contribution but will stay with the pointers for now.
>
> Best
>
> > Similar changes were also made for do_proc_douintvec_minmax_conv_param.
> >
> > Signed-off-by: Wen Yang <wen.yang@xxxxxxxxx>
> > Cc: Joel Granados <joel.granados@xxxxxxxxxx>
> > Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: Dave Young <dyoung@xxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >   kernel/sysctl.c | 75 ++++++++++++++++++++++---------------------------
> >   1 file changed, 34 insertions(+), 41 deletions(-)
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 79e6cb1d5c48..47e2fe4fe978 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -810,16 +810,16 @@ static int proc_taint(const struct ctl_table *table, int write,
> >   
> >   /**
> >    * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure
> > - * @min: pointer to minimum allowable value
> > - * @max: pointer to maximum allowable value
> > + * @min: the minimum allowable value
> > + * @max: the maximum allowable value
> >    *
> >    * The do_proc_dointvec_minmax_conv_param structure provides the
> >    * minimum and maximum values for doing range checking for those sysctl
> > - * parameters that use the proc_dointvec_minmax() handler.
> > + * parameters that use the proc_dointvec_minmax(), proc_dou8vec_minmax() and so on.
> >    */
> >   struct ram {
> > -	int *min;
> > -	int *max;
> > +	int min;
> > +	int max;
> >   };
> >   
> >   static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> > @@ -839,8 +839,7 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> >   		return ret;
> >   
> >   	if (write) {
> > -		if ((param->min && *param->min > tmp) ||
> > -		    (param->max && *param->max < tmp))
> > +		if ((param->min > tmp) || (param->max < tmp))
> >   			return -EINVAL;
> >   		WRITE_ONCE(*valp, tmp);
> >   	}
> > @@ -867,26 +866,26 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
> >   int proc_dointvec_minmax(const struct ctl_table *table, int write,
> >   		  void *buffer, size_t *lenp, loff_t *ppos)
> >   {
> > -	struct do_proc_dointvec_minmax_conv_param param = {
> > -		.min = (int *) table->extra1,
> > -		.max = (int *) table->extra2,
> > -	};
> > +	struct do_proc_dointvec_minmax_conv_param param;
> > +
> > +	param.min = (table->extra1) ? *(int *) table->extra1 : INT_MIN;
> > +	param.max = (table->extra2) ? *(int *) table->extra2 : INT_MAX;
> >   	return do_proc_dointvec(table, write, buffer, lenp, ppos,
> >   				do_proc_dointvec_minmax_conv, &param);
> >   }
> >   
> >   /**
> >    * struct do_proc_douintvec_minmax_conv_param - proc_douintvec_minmax() range checking structure
> > - * @min: pointer to minimum allowable value
> > - * @max: pointer to maximum allowable value
> > + * @min: minimum allowable value
> > + * @max: maximum allowable value
> >    *
> >    * The do_proc_douintvec_minmax_conv_param structure provides the
> >    * minimum and maximum values for doing range checking for those sysctl
> >    * parameters that use the proc_douintvec_minmax() handler.
> >    */
> >   struct do_proc_douintvec_minmax_conv_param {
> > -	unsigned int *min;
> > -	unsigned int *max;
> > +	unsigned int min;
> > +	unsigned int max;
> >   };
> >   
> >   static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> > @@ -904,8 +903,7 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> >   		return ret;
> >   
> >   	if (write) {
> > -		if ((param->min && *param->min > tmp) ||
> > -		    (param->max && *param->max < tmp))
> > +		if ((param->min > tmp) || (param->max < tmp))
> >   			return -ERANGE;
> >   
> >   		WRITE_ONCE(*valp, tmp);
> > @@ -936,10 +934,11 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
> >   int proc_douintvec_minmax(const struct ctl_table *table, int write,
> >   			  void *buffer, size_t *lenp, loff_t *ppos)
> >   {
> > -	struct do_proc_douintvec_minmax_conv_param param = {
> > -		.min = (unsigned int *) table->extra1,
> > -		.max = (unsigned int *) table->extra2,
> > -	};
> > +	struct do_proc_douintvec_minmax_conv_param param;
> > +
> > +	param.min = (table->extra1) ? *(unsigned int *) table->extra1 : 0;
> > +	param.max = (table->extra2) ? *(unsigned int *) table->extra2 : UINT_MAX;
> > +
> >   	return do_proc_douintvec(table, write, buffer, lenp, ppos,
> >   				 do_proc_douintvec_minmax_conv, &param);
> >   }
> > @@ -965,23 +964,17 @@ int proc_dou8vec_minmax(const struct ctl_table *table, int write,
> >   			void *buffer, size_t *lenp, loff_t *ppos)
> >   {
> >   	struct ctl_table tmp;
> > -	unsigned int min = 0, max = 255U, val;
> > +	unsigned int val;
> >   	u8 *data = table->data;
> > -	struct do_proc_douintvec_minmax_conv_param param = {
> > -		.min = &min,
> > -		.max = &max,
> > -	};
> > +	struct do_proc_douintvec_minmax_conv_param param;
> >   	int res;
> >   
> >   	/* Do not support arrays yet. */
> >   	if (table->maxlen != sizeof(u8))
> >   		return -EINVAL;
> >   
> > -	if (table->extra1)
> > -		min = *(unsigned int *) table->extra1;
> > -	if (table->extra2)
> > -		max = *(unsigned int *) table->extra2;
> > -
> > +	param.min = (table->extra1) ? *(unsigned int *) table->extra1 : 0;
> > +	param.max = (table->extra2) ? *(unsigned int *) table->extra2 : 255U;
> >   	tmp = *table;
> >   
> >   	tmp.maxlen = sizeof(val);
> > @@ -1022,7 +1015,7 @@ static int __do_proc_doulongvec_minmax(void *data,
> >   		void *buffer, size_t *lenp, loff_t *ppos,
> >   		unsigned long convmul, unsigned long convdiv)
> >   {
> > -	unsigned long *i, *min, *max;
> > +	unsigned long *i, min, max;
> >   	int vleft, first = 1, err = 0;
> >   	size_t left;
> >   	char *p;
> > @@ -1033,8 +1026,9 @@ static int __do_proc_doulongvec_minmax(void *data,
> >   	}
> >   
> >   	i = data;
> > -	min = table->extra1;
> > -	max = table->extra2;
> > +	min = (table->extra1) ? *(unsigned long *) table->extra1 : 0;
> > +	max = (table->extra2) ? *(unsigned long *) table->extra2 : ULONG_MAX;
> > +
> >   	vleft = table->maxlen / sizeof(unsigned long);
> >   	left = *lenp;
> >   
> > @@ -1066,7 +1060,7 @@ static int __do_proc_doulongvec_minmax(void *data,
> >   			}
> >   
> >   			val = convmul * val / convdiv;
> > -			if ((min && val < *min) || (max && val > *max)) {
> > +			if ((val < min) || (val > max)) {
> >   				err = -EINVAL;
> >   				break;
> >   			}
> > @@ -1236,8 +1230,7 @@ static int do_proc_dointvec_ms_jiffies_minmax_conv(bool *negp, unsigned long *lv
> >   		return ret;
> >   
> >   	if (write) {
> > -		if ((param->min && *param->min > tmp) ||
> > -				(param->max && *param->max < tmp))
> > +		if ((param->min > tmp) || (param->max < tmp))
> >   			return -EINVAL;
> >   		*valp = tmp;
> >   	}
> > @@ -1269,10 +1262,10 @@ int proc_dointvec_jiffies(const struct ctl_table *table, int write,
> >   int proc_dointvec_ms_jiffies_minmax(const struct ctl_table *table, int write,
> >   			  void *buffer, size_t *lenp, loff_t *ppos)
> >   {
> > -	struct do_proc_dointvec_minmax_conv_param param = {
> > -		.min = (int *) table->extra1,
> > -		.max = (int *) table->extra2,
> > -	};
> > +	struct do_proc_dointvec_minmax_conv_param param;
> > +
> > +	param.min = (table->extra1) ? *(int *) table->extra1 : INT_MIN;
> > +	param.max = (table->extra2) ? *(int *) table->extra2 : INT_MAX;
> >   	return do_proc_dointvec(table, write, buffer, lenp, ppos,
> >   			do_proc_dointvec_ms_jiffies_minmax_conv, &param);
> >   }
> > --
> > 2.25.1