Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

On 03/08/2018 03:49 PM, Luis R. Rodriguez wrote:
> On Thu, Mar 08, 2018 at 02:57:09PM -0500, Waiman Long wrote:
>> On 03/08/2018 01:31 PM, Luis R. Rodriguez wrote:
>>> On Thu, Mar 01, 2018 at 12:43:38PM -0500, Waiman Long wrote:
>>>> Even with clamped sysctl parameters, it is still not that straight
>>>> forward to figure out the exact range of those parameters. One may
>>>> try to write extreme parameter values to see if they get clamped.
>>>> To make it easier, a warning with the expected range will now be
>>>> printed in the kernel ring buffer when a clamped sysctl parameter
>>>> receives an out of range value.
>>>>
>>>> Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
>>>> ---
>>>>  include/linux/sysctl.h |  3 +++
>>>>  kernel/sysctl.c        | 52 ++++++++++++++++++++++++++++++++++++++++++--------
>>>>  2 files changed, 47 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h
>>>> index 448aa72..3db57af 100644
>>>> --- a/include/linux/sysctl.h
>>>> +++ b/include/linux/sysctl.h
>>>> @@ -130,11 +130,14 @@ struct ctl_table
>>>>   * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be
>>>>   *	flexibly clamped to min/max range in case the user provided
>>>>   *	an incorrect value.
>>>> + * @CTL_FLAGS_OOR_WARNED: Set to indicate that an out of range warning
>>>> + * 	had been issued for that entry.
>>>>   *
>>>>   * At most 16 different flags will be allowed.
>>>>   */
>>>>  enum ctl_table_flags {
>>>>  	CTL_FLAGS_CLAMP_RANGE		= BIT(0),
>>>> +	CTL_FLAGS_OOR_WARNED		= BIT(1),
>>>>  };
>>> Ugh, no. Now I see why you had to set this flag later.
>>>
>>> You are not using this flag to "warn" but rather for an internal
>>> status checker if you have warned or not. Internal flags should
>>> not be something the user sets. If we want a flag for warning
>>> that's one thing. If we need a flag to keep tabs if we have
>>> warned or not that needs to be kept separately and internally,
>>> nothing the user has to do set or reset.
>>>
>>>   Luis
>> What I want to do is a printk_once for each sysctl parameter. So the
>> flag is used as a marker that a warning has been printed.
>>
>> I do understand that it gets somewhat ugly in the case of msgmni and
>> shmmni because of the copying back of the flag. Another alternative that
>> had been suggested by Kees is to use prink_ratelimited. That we don't
>> need that flag at all.
> However it is done, a user flag should not be used also for internal
> flag settings. That's just gross. Internal state machine stuff should
> remain far from what the user is able to modify.
>
> Also, why can't it just use pr_warn_once() and be done with it?

Different sysctl parameters can use the same minmax proc_handler. Using
pr_warn_once() means mistake in one will prevent mistakes in other
parameters from showing up.

Cheers,
Longman