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

[Wen Yang <wen.yang@linux.dev>]

On 2025/1/16 17:37, Joel Granados wrote:
> On Sun, Jan 05, 2025 at 11:28:53PM +0800, Wen Yang wrote:
>> do_proc_dointvec_conv() used the default range of int type, while
>> do_proc_dointvec_minmax_conv() additionally used "int * {min, max}" of
>> struct do_proc_dointvec_minmax_conv_param, which are actually passed
>> in table->extra{1,2} pointers.
>>
>> By changing the struct do_proc_dointvec_minmax_conv_param's
>> "int * {min, max}" to "int {min, max}" and setting the min/max value
>> via dereference the table->extra{1, 2}, those do_proc_dointvec_conv() and
>> do_proc_dointvec_minmax_conv() functions can be merged into one and reduce
>> code by one-third.
>>
>> Similar changes were also made for do_proc_douintvec_minmax_conv_param,
>> do_proc_douintvec_conv() and do_proc_douintvec_minmax_conv().
>>
> Before moving forward, I need the motivation.
> 
> Please answer this:
> *Why* are you pushing these two [1], [2] series?
> 
> Best
> 
> [1]
>    * https://lore.kernel.org/tencent_A7FD9E9E75AA74A87583A427F5D20F988B09@qq.com
>    * https://lore.kernel.org/tencent_C5E6023F97E7CC2A046AAEA09BC9ACF43907@qq.com
>    * https://lore.kernel.org/cover.1726365007.git.wen.yang@linux.dev
>    * https://lore.kernel.org/cover.1726910671.git.wen.yang@linux.dev
> 
> [2]
>    * https://lore.kernel.org/20240714173858.52163-1-wen.yang@linux.dev
>    * https://lore.kernel.org/20240715150223.54707-1-wen.yang@linux.dev
>    * https://lore.kernel.org/20240905134818.4104-1-wen.yang@linux.dev
>    * https://lore.kernel.org/20241201140058.5653-1-wen.yang@linux.dev
> 

Thank you for your comments.

Here is a brief report about it.

The boundary check of sysctl uses .extra{1, 2} pointers, which need to 
point to constant variables (such as two_five_five, n_65535, ue_int_max, 
etc. that appear in multiple modules).

We first try to stuff these variables into the shared const arrays ( 
(sysctl_vals, sysctl_long_vals) to save memory, as shown in series 1.

Thank Eric for pointing out:
- You can still save a lot more by turning .extra1 and .extra2
- into longs instead of keeping them as pointers and needing
- constants to be pointed at somewhere.

We have further found more detailed information about "kill the shared 
const array":

https://lkml.kernel.org/87zgpte9o4.fsf@email.froward.int.ebiederm.org

67ff32289 ("proc_sysctl: update docs for __register_sysctl_table()")


So we tried some ways to achieve it, as shown in series 2.

Step 1: The extra{1,2} pointer is used as {min, max} pointers in the 
do_proc_do{int, uint}vec_minmax_conv_param structures. By changing the 
{min, max} pointers to numeric values, the code is simplified and 
preparations are made for the second step.

Step 2: Add some sysctl helper functions to avoid direct access to
table->extra1/extra2, and also support encoding values directly in the 
table entry.

https://lore.kernel.org/all/0f4a5233b75e6484a6236d85d2b506c96ea41ef1.1726910671.git.wen.yang@linux.dev/

https://lore.kernel.org/all/a086609632328c2feee69b10067e43115885b2ae.1726910671.git.wen.yang@linux.dev/


Step 3: gradually remove the extra constant variables in multiple modules.

https://lore.kernel.org/all/5f753895a5f31cac65ddeb56b58fc17553785163.1726910671.git.wen.yang@linux.dev/

https://lore.kernel.org/all/20d67551ed7fa9c774d2b128ad9bc298a0a55c9d.1726910671.git.wen.yang@linux.dev/



--
Best wishes,
Wen




>> Selftest were also made:
>>
>> [23:16:38] ================ sysctl_test (11 subtests) =================
>> [23:16:38] [PASSED] sysctl_test_api_dointvec_null_tbl_data
>> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_maxlen_unset
>> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_len_is_zero
>> [23:16:38] [PASSED] sysctl_test_api_dointvec_table_read_but_position_set
>> [23:16:38] [PASSED] sysctl_test_dointvec_read_happy_single_positive
>> [23:16:38] [PASSED] sysctl_test_dointvec_read_happy_single_negative
>> [23:16:38] [PASSED] sysctl_test_dointvec_write_happy_single_positive
>> [23:16:38] [PASSED] sysctl_test_dointvec_write_happy_single_negative
>> [23:16:38] [PASSED] sysctl_test_api_dointvec_write_single_less_int_min
>> [23:16:38] [PASSED] sysctl_test_api_dointvec_write_single_greater_int_max
>> [23:16:38] [PASSED] sysctl_test_register_sysctl_sz_invalid_extra_value
>> [23:16:38] =================== [PASSED] sysctl_test ===================
>> [23:16:38] ============================================================
>> [23:16:38] Testing complete. Ran 11 tests: passed: 11
>>
>> 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 | 211 ++++++++++++++++++++----------------------------
>>   1 file changed, 86 insertions(+), 125 deletions(-)
>>
>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
>> index 7ae7a4136855..250dc9057259 100644
>> --- a/kernel/sysctl.c
>> +++ b/kernel/sysctl.c
>> @@ -424,22 +424,44 @@ static void proc_put_char(void **buf, size_t *size, char c)
>>   	}
>>   }
>>   
>> -static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
>> -				 int *valp,
>> -				 int write, void *data)
>> +/**
>> + * struct do_proc_dointvec_minmax_conv_param - proc_dointvec_minmax() range checking structure
>> + * @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(), proc_dou8vec_minmax() and so on.
>> + */
>> +struct do_proc_dointvec_minmax_conv_param {
>> +	int min;
>> +	int max;
>> +};
>> +
>> +static inline int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>> +		int *valp,
>> +		int write, void *data)
>>   {
>> +	struct do_proc_dointvec_minmax_conv_param *param = data;
>> +	long min, max;
>> +	long val;
>> +
>>   	if (write) {
>>   		if (*negp) {
>> -			if (*lvalp > (unsigned long) INT_MAX + 1)
>> -				return -EINVAL;
>> -			WRITE_ONCE(*valp, -*lvalp);
>> +			max = param ? param->max : 0;
>> +			min = param ? param->min : INT_MIN;
>> +			val = -*lvalp;
>>   		} else {
>> -			if (*lvalp > (unsigned long) INT_MAX)
>> -				return -EINVAL;
>> -			WRITE_ONCE(*valp, *lvalp);
>> +			max = param ? param->max : INT_MAX;
>> +			min = param ? param->min : 0;
>> +			val = *lvalp;
>>   		}
>> +
>> +		if (val > max || val < min)
>> +			return -EINVAL;
>> +		WRITE_ONCE(*valp, (int)val);
>>   	} else {
>> -		int val = READ_ONCE(*valp);
>> +		val = (int)READ_ONCE(*valp);
>>   		if (val < 0) {
>>   			*negp = true;
>>   			*lvalp = -(unsigned long)val;
>> @@ -448,21 +470,44 @@ static int do_proc_dointvec_conv(bool *negp, unsigned long *lvalp,
>>   			*lvalp = (unsigned long)val;
>>   		}
>>   	}
>> +
>>   	return 0;
>>   }
>>   
>> -static int do_proc_douintvec_conv(unsigned long *lvalp,
>> -				  unsigned int *valp,
>> -				  int write, void *data)
>> +/**
>> + * struct do_proc_douintvec_minmax_conv_param - proc_douintvec_minmax() range checking structure
>> + * @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;
>> +};
>> +
>> +static inline int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
>> +					 unsigned int *valp,
>> +					 int write, void *data)
>>   {
>> +	struct do_proc_douintvec_minmax_conv_param *param = data;
>> +	unsigned long min, max;
>> +
>>   	if (write) {
>> -		if (*lvalp > UINT_MAX)
>> +		max = param ? param->max : UINT_MAX;
>> +		min = param ? param->min : 0;
>> +
>> +		if (*lvalp > max || *lvalp < min)
>>   			return -EINVAL;
>> -		WRITE_ONCE(*valp, *lvalp);
>> +
>> +		WRITE_ONCE(*valp, (unsigned int)*lvalp);
>>   	} else {
>>   		unsigned int val = READ_ONCE(*valp);
>>   		*lvalp = (unsigned long)val;
>>   	}
>> +
>>   	return 0;
>>   }
>>   
>> @@ -489,7 +534,7 @@ static int __do_proc_dointvec(void *tbl_data, const struct ctl_table *table,
>>   	left = *lenp;
>>   
>>   	if (!conv)
>> -		conv = do_proc_dointvec_conv;
>> +		conv = do_proc_dointvec_minmax_conv;
>>   
>>   	if (write) {
>>   		if (proc_first_pos_non_zero_ignore(ppos, table))
>> @@ -667,7 +712,7 @@ static int __do_proc_douintvec(void *tbl_data, const struct ctl_table *table,
>>   	}
>>   
>>   	if (!conv)
>> -		conv = do_proc_douintvec_conv;
>> +		conv = do_proc_douintvec_minmax_conv;
>>   
>>   	if (write)
>>   		return do_proc_douintvec_w(i, table, buffer, lenp, ppos,
>> @@ -762,7 +807,7 @@ int proc_douintvec(const struct ctl_table *table, int write, void *buffer,
>>   		size_t *lenp, loff_t *ppos)
>>   {
>>   	return do_proc_douintvec(table, write, buffer, lenp, ppos,
>> -				 do_proc_douintvec_conv, NULL);
>> +				 do_proc_douintvec_minmax_conv, NULL);
>>   }
>>   
>>   /*
>> @@ -808,46 +853,6 @@ static int proc_taint(const struct ctl_table *table, int write,
>>   	return err;
>>   }
>>   
>> -/**
>> - * 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
>> - *
>> - * 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.
>> - */
>> -struct do_proc_dointvec_minmax_conv_param {
>> -	int *min;
>> -	int *max;
>> -};
>> -
>> -static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp,
>> -					int *valp,
>> -					int write, void *data)
>> -{
>> -	int tmp, ret;
>> -	struct do_proc_dointvec_minmax_conv_param *param = data;
>> -	/*
>> -	 * If writing, first do so via a temporary local int so we can
>> -	 * bounds-check it before touching *valp.
>> -	 */
>> -	int *ip = write ? &tmp : valp;
>> -
>> -	ret = do_proc_dointvec_conv(negp, lvalp, ip, write, data);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (write) {
>> -		if ((param->min && *param->min > tmp) ||
>> -		    (param->max && *param->max < tmp))
>> -			return -EINVAL;
>> -		WRITE_ONCE(*valp, tmp);
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>   /**
>>    * proc_dointvec_minmax - read a vector of integers with min/max values
>>    * @table: the sysctl table
>> @@ -867,53 +872,14 @@ 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
>> - *
>> - * 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;
>> -};
>> -
>> -static int do_proc_douintvec_minmax_conv(unsigned long *lvalp,
>> -					 unsigned int *valp,
>> -					 int write, void *data)
>> -{
>> -	int ret;
>> -	unsigned int tmp;
>> -	struct do_proc_douintvec_minmax_conv_param *param = data;
>> -	/* write via temporary local uint for bounds-checking */
>> -	unsigned int *up = write ? &tmp : valp;
>> -
>> -	ret = do_proc_douintvec_conv(lvalp, up, write, data);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (write) {
>> -		if ((param->min && *param->min > tmp) ||
>> -		    (param->max && *param->max < tmp))
>> -			return -ERANGE;
>> -
>> -		WRITE_ONCE(*valp, tmp);
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>   /**
>>    * proc_douintvec_minmax - read a vector of unsigned ints with min/max values
>>    * @table: the sysctl table
>> @@ -936,10 +902,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 +932,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 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 +983,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 +994,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 +1028,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 +1198,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 +1230,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
>>
>