1. Patchew
  2. linux
  3. View series

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

Wen Yang posted 1 patch 1 year, 2 months ago
There is a newer version of this series
kernel/sysctl.c | 75 ++++++++++++++++++++++---------------------------
1 file changed, 34 insertions(+), 41 deletions(-)
[RESEND PATCH v4] sysctl: simplify the min/max boundary check
Posted by Wen Yang 1 year, 2 months ago 
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.

Similar changes were also made for do_proc_douintvec_minmax_conv_param.

Signed-off-by: Wen Yang <wen.yang@linux.dev>
Cc: Joel Granados <joel.granados@kernel.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Eric W. Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Dave Young <dyoung@redhat.com>
Cc: linux-kernel@vger.kernel.org
---
 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 do_proc_dointvec_minmax_conv_param {
-	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
Re: [RESEND PATCH v4] sysctl: simplify the min/max boundary check
Posted by Joel Granados 1 year, 1 month ago 
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.

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@linux.dev>
> Cc: Joel Granados <joel.granados@kernel.org>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Eric W. Biederman <ebiederm@xmission.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: Dave Young <dyoung@redhat.com>
> Cc: linux-kernel@vger.kernel.org
> ---
>  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 do_proc_dointvec_minmax_conv_param {
> -	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
> 

-- 

Joel Granados
Re: [RESEND PATCH v4] sysctl: simplify the min/max boundary check
Posted by Wen Yang 1 year, 1 month ago 

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@linux.dev>
>> Cc: Joel Granados <joel.granados@kernel.org>
>> Cc: Luis Chamberlain <mcgrof@kernel.org>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Eric W. Biederman <ebiederm@xmission.com>
>> Cc: Christian Brauner <brauner@kernel.org>
>> Cc: Dave Young <dyoung@redhat.com>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   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
>>
>
Re: Re: [RESEND PATCH v4] sysctl: simplify the min/max boundary check
Posted by Joel Granados 1 year ago 
On Thu, Jan 02, 2025 at 12:26:36AM +0800, Wen Yang wrote:
> 
> 
> 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.
> > 
I'm answering this one late because you sent a V5.
> 
> 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.
1. Locality for performance? What is your use case here? These are
   functions that are usually called once. Is there some hot path
   through the sysctls that I'm not aware of?

2. Please clarify what do you want to accomplish? It seem like you keep jumping:
    - "Simplify code a bit"
    - "Locality"
    - "Avoid duplicate code"
    - "prepare for removing extra{1,2}

> 
> 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.
I did not mean "send a bigger simplification"; sorry if I did not
explain myself. I meant that I do *not* see any gain in removing lines
of code for this particular case.

> 
> All of this is in preparation for ultimately killing the table ->{extra1,
> extra2} pointers.
I am curious about this one because I think this is what you ultimately
want to do. But I'm still not clear on why. Is it to save memory?

> 
> We will rework later and send v5.
I see that you have already sent it. I'll add further comments to that
one.

Best

> 
> --
> 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@linux.dev>
> > > Cc: Joel Granados <joel.granados@kernel.org>
> > > Cc: Luis Chamberlain <mcgrof@kernel.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> > > Cc: Eric W. Biederman <ebiederm@xmission.com>
> > > Cc: Christian Brauner <brauner@kernel.org>
> > > Cc: Dave Young <dyoung@redhat.com>
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >   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
> > > 
> > 

-- 

Joel Granados

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.