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

[Wen Yang]

On 2024/2/25 20:29, Eric W. Biederman wrote:
> wenyang.linux@xxxxxxxxxxx writes:
>
> > From: Wen Yang <wenyang.linux@xxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxx>
> > Cc: Luis Chamberlain <mcgrof@xxxxxxxxxx>
> > Cc: Kees Cook <keescook@xxxxxxxxxxxx>
> > Cc: Joel Granados <j.granados@xxxxxxxxxxx>
> > Cc: Christian Brauner <brauner@xxxxxxxxxx>
> > Cc: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx>
> > Cc: Shuah Khan <skhan@xxxxxxxxxxxxxxxxxxx>
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > ---
> >   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"),