Received-SPF: pass (google.com: domain of linux-kernel+bounces-110279-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <tencent_583360D51D354010FEE44A3A3B739D5FF506@qq.com>
Date: Thu, 21 Mar 2024 23:39:10 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v2 0/9] encode the values directly in the table entry
Content-Language: en-US
To: Joel Granados <j.granados@samsung.com>
Cc: "Eric W . Biederman" <ebiederm@xmission.com>,
 Luis Chamberlain <mcgrof@kernel.org>, Kees Cook <keescook@chromium.org>,
 Christian Brauner <brauner@kernel.org>, linux-kernel@vger.kernel.org
References: <CGME20240309103209eucas1p25311b485e321e701eeacbdb3e2ba8758@eucas1p2.samsung.com>
 <tencent_C5E6023F97E7CC2A046AAEA09BC9ACF43907@qq.com>
 <20240321152143.yxoonjy3m6bdddkf@joelS2.panther.com>
From: Wen Yang <wenyang.linux@foxmail.com>
In-Reply-To: <20240321152143.yxoonjy3m6bdddkf@joelS2.panther.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


On 2024/3/21 23:21, Joel Granados wrote:
> On Sat, Mar 09, 2024 at 06:31:17PM +0800, wenyang.linux@foxmail.com wrote:
>> From: Wen Yang <wenyang.linux@foxmail.com>
>>
>> The boundary check of multiple modules uses these static variables (such as
>> two_five_five, n_65535, ue_int_max, etc), and they are also not changed.
> This message is a bit cryptic. I had to do a fair amount of research to
> get what you meant here. Having the context in front is OK with me, but
> I would add a bit more information so the reader does not have to go to
> the code and grep for the variables that you mean. Something like this:
> "When using a sysctl proc_handler that requires a boundary check (like
> proce_dointvec_minmax) it is common to use a const variable like n_65535
> in net/rxrpc/sysctl.c or OTHER EXAMPLES...). This is suboptimal because
> YOUR REASONS HERE"
> 
> 

Thanks a lot for your very considerate input – this is highly 
appreciated. We will revise the change message and send v3 later.

--
Best wishes,
Wen


>>
>> Eric points out: "by turning .extra1 and .extra2 into longs instead of
>> keeping them as pointers and needing constants to be pointed at somewhere
>> ... 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."
> I think it would be better to just link to the lore discussion.
> 
>>
>> This patch series achieves direct encoding values in table entries and still
>> maintains compatibility with existing extra1/extra2 pointers.
>> Afterwards, we can remove these unnecessary static variables progressively and
>> also gradually kill the shared const array.
> Two things:
> 1. Please name the const array: sysctl_vals
> 2. What is missing from this patchset to completely kill sysctl_vals?
> 
>>
>> Wen Yang (9):
>>    sysctl: support encoding values directly in the table entry
>>    kernel/sysctl-test: add some kunit test cases for min/max detection
>>    rxrpc: delete these unnecessary static variables n_65535, four,
>>      max_500, etc
>>    net: ipv6: delete these unnecessary static variables two_five_five and
>>      minus_one
>>    svcrdma: delete these unnecessary static variables min_ord, max_ord,
>>      etc
>>    sysctl: delete these unnecessary static variables i_zero and
>>      i_one_hundred
>>    epoll: delete these unnecessary static variables long_zero and
>>      long_max
>>    fs: inotify: delete these unnecessary static variables it_zero and
>>      it_int_max
>>    ucounts: delete these unnecessary static variables ue_zero and
>>      ue_int_max
>>
>>   fs/eventpoll.c                   |  19 +-
>>   fs/notify/inotify/inotify_user.c |  49 +++--
>>   include/linux/sysctl.h           | 108 ++++++++++-
>>   kernel/sysctl-test.c             | 300 +++++++++++++++++++++++++++++++
>>   kernel/sysctl.c                  |  61 +++++--
>>   kernel/ucount.c                  |   8 +-
>>   lib/test_sysctl.c                |  12 +-
>>   net/ipv6/addrconf.c              |  15 +-
>>   net/rxrpc/sysctl.c               | 169 ++++++++---------
>>   net/sunrpc/xprtrdma/svc_rdma.c   |  21 +--
>>   10 files changed, 571 insertions(+), 191 deletions(-)
>>
>> Cc: Eric W. Biederman <ebiederm@xmission.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: linux-kernel@vger.kernel.org
>>
>> -- 
>> 2.25.1
>>
>