From: Wen Yang <[email protected]>
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 <[email protected]>
Cc: Luis Chamberlain <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Joel Granados <[email protected]>
Cc: Christian Brauner <[email protected]>
Cc: "Eric W. Biederman" <[email protected]>
Cc: Shuah Khan <[email protected]>
Cc: [email protected]
---
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"),
--
2.25.1
[email protected] writes:
> From: Wen Yang <[email protected]>
>
> 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 <[email protected]>
> Cc: Luis Chamberlain <[email protected]>
> Cc: Kees Cook <[email protected]>
> Cc: Joel Granados <[email protected]>
> Cc: Christian Brauner <[email protected]>
> Cc: "Eric W. Biederman" <[email protected]>
> Cc: Shuah Khan <[email protected]>
> Cc: [email protected]
> ---
> 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"),
On 2024/2/25 20:29, Eric W. Biederman wrote:
> [email protected] writes:
>
>> From: Wen Yang <[email protected]>
>>
>> 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 <[email protected]>
>> Cc: Luis Chamberlain <[email protected]>
>> Cc: Kees Cook <[email protected]>
>> Cc: Joel Granados <[email protected]>
>> Cc: Christian Brauner <[email protected]>
>> Cc: "Eric W. Biederman" <[email protected]>
>> Cc: Shuah Khan <[email protected]>
>> Cc: [email protected]
>> ---
>> 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"),
On Sun, Feb 25, 2024 at 06:29:33AM -0600, Eric W. Biederman wrote:
> [email protected] writes:
>
> > From: Wen Yang <[email protected]>
> >
> > 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?
I agree with removing sysctl_vals. I wanted to get more info but was not
able to find much in lore.kernel.org. I was wondering if the push
towards removing it had been discussed in the lists? Or was it more like
a conference hallway discussion?
Best
>
> 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
>
--
Joel Granados