From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from out-186.mta1.migadu.com (out-186.mta1.migadu.com [95.215.58.186]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6E7711854 for ; Wed, 1 Jan 2025 16:26:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=95.215.58.186 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735748820; cv=none; b=kLP+1cssC7lyR1Bfy3cjfrl6p5GdmK5ljLGV5oqnzWmKi99K4ux3bxx9dsoXkgPIVWsi6yo4YqwPXOlp1a2/GDTt3bKQ5L+VBMQsRGBM49pIi+SCbp4Tfjvvbozf4tHi4w7zMj+QE0c73mVsO6Dn646tG292JCXwhoPelbeiijE= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1735748820; c=relaxed/simple; bh=I/aQfFmb/b5B1TR1GPMUX+cd8AyJM65ynkQ3Oju7jeA=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=rEjTmd1wHa54sn+wL77+WaHqSLepMVQJLZxilUhuUSbJAIWhAgmZ38phO9utJVEatpL6T+nDw88NE7DzrJpRoR/r4ELRra/jYcR/sT2cCe6/Lm8M9Uloy1vQbz8MjQC8kT9PPVq3nNqxGmhM10tx6uXudxAdL17cU1qrDyPuUZI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev; spf=pass smtp.mailfrom=linux.dev; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b=I5sv2jaM; arc=none smtp.client-ip=95.215.58.186 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.dev Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linux.dev Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=linux.dev header.i=@linux.dev header.b="I5sv2jaM" Message-ID: <26626215-78fe-4dcf-b0bd-5881b9002e3e@linux.dev> DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.dev; s=key1; t=1735748815; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=wWUJlg+Y+BuWEB7vRjJh1Bp5gedeMATuNbHmwqRYgLA=; b=I5sv2jaMfDsb+wRZQoenW3fMH/tebj6SFAnPFSIWKCRZEuqvfRNovzWTz13NBtlGTYol6h 0FjrFR9PJ8S+0pEvp+7zrDDUtCdACfnaCDa8rL7hVzxPut8lrT1mLRuOLiorWyowDa8mil gJAAMd3/vtT963rToa29o6T5OkQtNr0= Date: Thu, 2 Jan 2025 00:26:36 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Subject: Re: [RESEND PATCH v4] sysctl: simplify the min/max boundary check To: Joel Granados Cc: "Eric W . Biederman" , Luis Chamberlain , Kees Cook , Christian Brauner , Dave Young , linux-kernel@vger.kernel.org References: <20241201140058.5653-1-wen.yang@linux.dev> <7cuqiavbm3vhdnpwumknb6eog4r73w7c2tcivwkvjer3a6hrwc@4zu7d5bng4wj> Content-Language: en-US X-Report-Abuse: Please report any abuse attempt to abuse@migadu.com and include these headers. From: Wen Yang In-Reply-To: <7cuqiavbm3vhdnpwumknb6eog4r73w7c2tcivwkvjer3a6hrwc@4zu7d5bng4wj> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Migadu-Flow: FLOW_OUT 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 >> Cc: Joel Granados >> Cc: Luis Chamberlain >> Cc: Kees Cook >> Cc: Eric W. Biederman >> Cc: Christian Brauner >> Cc: Dave Young >> 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, ¶m); >> } >> >> /** >> * 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, ¶m); >> } >> @@ -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, ¶m); >> } >> -- >> 2.25.1 >> >