diff mbox series

[8/8] ucounts: delete these duplicate static variables ue_zero and ue_int_max

Message ID tencent_AA60553AD4DF41859328E9CACC367EFDCF08@qq.com
State New
Headers
Series [1/8] sysctl: introduce sysctl SYSCTL_U8_MAX and SYSCTL_LONG_S32_MAX |

Commit Message

Wen Yang Feb. 25, 2024, 4:05 a.m. UTC 
  From: Wen Yang <wenyang.linux@foxmail.com>

Since these static variables (ue_zero and ue_int_max) are only used for
boundary checks and will not be changed, remove it and use the ones in
our shared const array.

Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Joel Granados <j.granados@samsung.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: Shuah Khan <skhan@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org
---
 kernel/ucount.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Eric W. Biederman Feb. 25, 2024, 12:29 p.m. UTC | #1 
wenyang.linux@foxmail.com writes:

> From: Wen Yang <wenyang.linux@foxmail.com>
>
> Since these static variables (ue_zero and ue_int_max) are only used for
> boundary checks and will not be changed, remove it and use the ones in
> our shared const array.

What happened to the plans to kill the shared const array?

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.

As I recall the last version of this actually broke the code,
(but not on little endian).

This one if the constants are properly named looks better
than that, but I don't see any reason why you want shared
constants for such a handful of things.  Especially when
it has proven to be error prone in the past.

The only people I can see who find a significant benefit by
consolidating all of the constants into one place are people who know
how to stomp kernel memory.

Eric


>
> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Joel Granados <j.granados@samsung.com>
> Cc: Christian Brauner <brauner@kernel.org>
> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
> Cc: Shuah Khan <skhan@linuxfoundation.org>
> Cc: linux-kernel@vger.kernel.org
> ---
>  kernel/ucount.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/ucount.c b/kernel/ucount.c
> index 4aa6166cb856..05bbba02ae4f 100644
> --- a/kernel/ucount.c
> +++ b/kernel/ucount.c
> @@ -58,17 +58,14 @@ static struct ctl_table_root set_root = {
>  	.permissions = set_permissions,
>  };
>  
> -static long ue_zero = 0;
> -static long ue_int_max = INT_MAX;
> -
>  #define UCOUNT_ENTRY(name)					\
>  	{							\
>  		.procname	= name,				\
>  		.maxlen		= sizeof(long),			\
>  		.mode		= 0644,				\
>  		.proc_handler	= proc_doulongvec_minmax,	\
> -		.extra1		= &ue_zero,			\
> -		.extra2		= &ue_int_max,			\
> +		.extra1		= SYSCTL_LONG_ZERO,		\
> +		.extra2		= SYSCTL_LONG_S32_MAX,		\
>  	}
>  static struct ctl_table user_table[] = {
>  	UCOUNT_ENTRY("max_user_namespaces"),
Wen Yang Feb. 25, 2024, 3:21 p.m. UTC | #2 
On 2024/2/25 20:29, Eric W. Biederman wrote:
> wenyang.linux@foxmail.com writes:
> 
>> From: Wen Yang <wenyang.linux@foxmail.com>
>>
>> Since these static variables (ue_zero and ue_int_max) are only used for
>> boundary checks and will not be changed, remove it and use the ones in
>> our shared const array.
> 
> What happened to the plans to kill the shared const array?
> 
> 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.
> 
> As I recall the last version of this actually broke the code,
> (but not on little endian).
>
Thank you. While developing a driver recently, we accidentally 
discovered some redundant code related to extra1/extra2, so we tried to 
optimize it a bit.


Thank you for your comments. This plan (kill the shared const array) 
seems meaningful and should require some work. We are very willing to 
participate.

I am glad to receive your feedback. This plan (kill the shared const 
array) seems meaningful and should require a lot of work. We are very 
willing to participate.


> This one if the constants are properly named looks better
> than that, but I don't see any reason why you want shared
> constants for such a handful of things.  Especially when
> it has proven to be error prone in the past.
>


This patch series replaces multiple static variables (such as zero, 
two_five_five, n_65535, ue_int_max, etc) with some unified macros (such 
as SYSCTL_U8_ZERO, SYSCTL_U8_MAX, SYSCTL_U16_MAX, etc.).

Although according to the current implementation of sysctl, these macros 
are currently defined as pointers to the elements of this shared array, 
and they can also be easily switched from pointers to appropriate 
numbers when the shared array of sysctl is removed according to the 
above plan.

So the current patch series is also beneficial for subsequent 
optimization, that is, deleting this shared const array.


> The only people I can see who find a significant benefit by
> consolidating all of the constants into one place are people who know
> how to stomp kernel memory.
> 

As above, thanks.


--
Best wishes,
Wen

> 
> 
>>
>> Signed-off-by: Wen Yang <wenyang.linux@foxmail.com>
>> Cc: Luis Chamberlain <mcgrof@kernel.org>
>> Cc: Kees Cook <keescook@chromium.org>
>> Cc: Joel Granados <j.granados@samsung.com>
>> Cc: Christian Brauner <brauner@kernel.org>
>> Cc: "Eric W. Biederman" <ebiederm@xmission.com>
>> Cc: Shuah Khan <skhan@linuxfoundation.org>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   kernel/ucount.c | 7 ++-----
>>   1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/kernel/ucount.c b/kernel/ucount.c
>> index 4aa6166cb856..05bbba02ae4f 100644
>> --- a/kernel/ucount.c
>> +++ b/kernel/ucount.c
>> @@ -58,17 +58,14 @@ static struct ctl_table_root set_root = {
>>   	.permissions = set_permissions,
>>   };
>>   
>> -static long ue_zero = 0;
>> -static long ue_int_max = INT_MAX;
>> -
>>   #define UCOUNT_ENTRY(name)					\
>>   	{							\
>>   		.procname	= name,				\
>>   		.maxlen		= sizeof(long),			\
>>   		.mode		= 0644,				\
>>   		.proc_handler	= proc_doulongvec_minmax,	\
>> -		.extra1		= &ue_zero,			\
>> -		.extra2		= &ue_int_max,			\
>> +		.extra1		= SYSCTL_LONG_ZERO,		\
>> +		.extra2		= SYSCTL_LONG_S32_MAX,		\
>>   	}
>>   static struct ctl_table user_table[] = {
>>   	UCOUNT_ENTRY("max_user_namespaces"),
diff mbox series

Patch

diff --git a/kernel/ucount.c b/kernel/ucount.c
index 4aa6166cb856..05bbba02ae4f 100644
--- a/kernel/ucount.c
+++ b/kernel/ucount.c
@@ -58,17 +58,14 @@  static struct ctl_table_root set_root = {
 	.permissions = set_permissions,
 };
 
-static long ue_zero = 0;
-static long ue_int_max = INT_MAX;
-
 #define UCOUNT_ENTRY(name)					\
 	{							\
 		.procname	= name,				\
 		.maxlen		= sizeof(long),			\
 		.mode		= 0644,				\
 		.proc_handler	= proc_doulongvec_minmax,	\
-		.extra1		= &ue_zero,			\
-		.extra2		= &ue_int_max,			\
+		.extra1		= SYSCTL_LONG_ZERO,		\
+		.extra2		= SYSCTL_LONG_S32_MAX,		\
 	}
 static struct ctl_table user_table[] = {
 	UCOUNT_ENTRY("max_user_namespaces"),