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

[Luis R. Rodriguez]

On Thu, Mar 08, 2018 at 04:40:17PM -0500, Waiman Long wrote:
> 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.

OK then use a separate internal set of flags for internal book keeping.
Its nothing the user should ever have to set.

  Luis