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 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?

  Luis