Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Luis R. Rodriguez]

On Thu, Mar 08, 2018 at 02:35:32PM -0500, Waiman Long wrote:
> On 03/08/2018 12:57 PM, Luis R. Rodriguez wrote:
> > On Thu, Mar 08, 2018 at 05:51:09PM +0000, Luis R. Rodriguez wrote:
> >> On Thu, Mar 01, 2018 at 01:31:17PM -0800, Andrew Morton wrote:
> >>> On Thu,  1 Mar 2018 12:43:37 -0500 Waiman Long <longman@xxxxxxxxxx> wrote:
> >>>
> >>>> When minimum/maximum values are specified for a sysctl parameter in
> >>>> the ctl_table structure with proc_dointvec_minmax() handler, update
> >>>> to that parameter will fail with error if the given value is outside
> >>>> of the required range.
> >>>>
> >>>> There are use cases where it may be better to clamp the value of
> >>>> the sysctl parameter to the given range without failing the update,
> >>>> especially if the users are not aware of the actual range limits.
> >>>> Reading the value back after the update will now be a good practice
> >>>> to see if the provided value exceeds the range limits.
> >>>>
> >>>> To provide this less restrictive form of range checking, a new flags
> >>>> field is added to the ctl_table structure. The new field is a 16-bit
> >>>> value that just fits into the hole left by the 16-bit umode_t field
> >>>> without increasing the size of the structure.
> >>>>
> >>>> When the CTL_FLAGS_CLAMP_RANGE flag is set in the ctl_table entry,
> >>>> any update from the userspace will be clamped to the given range
> >>>> without error.
> >>>>
> >>>> ...
> >>>>
> >>>> --- a/include/linux/sysctl.h
> >>>> +++ b/include/linux/sysctl.h
> >>>> @@ -116,6 +116,7 @@ struct ctl_table
> >>>>  	void *data;
> >>>>  	int maxlen;
> >>>>  	umode_t mode;
> >>>> +	uint16_t flags;
> >>> It would be nice to make this have type `enum ctl_table_flags', but I
> >>> guess there's then no reliable way of forcing it to be 16-bit.
> >>>
> >>> I guess this is the best we can do...
> >>>
> >> We can add this to the enum:
> >>
> >> enum ctl_table_flags {                                                                                                                                                                       
> >>        CTL_FLAGS_CLAMP_RANGE           = BIT(0),                                                                                                                                             
> >> +	__CTL_FLAGS_CLAMP_MAX          = BIT(16),
> >> }; 
> >>
> >>
> >> Then also:
> >>
> >> #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX + 1))-1)
> >>
> >> at the end of the definition, then a helper which can be used during
> >> parsing:
> >>
> >> static int check_ctl_table_flags(u16 flags)
> >> {
> >> 	if (flags & ~(CTL_TABLE_FLAGS_ALL))
> >> 		return -ERANGE;
> >> 	return 0;
> >> }
> >>
> >> Waiman please evaluate and add.
> > Also, I guess we have ... max bit used and max allowed (16) really, where one is the
> > max allowed bit field given current definitions, the other is the max flag possible
> > setting in the future. We might as well go with the smaller one, which is the current
> > max, so it can just be
> >
> > enum ctl_table_flags {
> > 	CTL_FLAGS_CLAMP_RANGE	 = BIT(0),
> > 	__CTL_FLAGS_CLAMP_MAX    = BIT(1),
> > };
> >
> >
> > #define CTL_TABLE_FLAGS_ALL	((BIT(__CTL_FLAGS_CLAMP_MAX))-1)
> >
> > That way we just check against the actual max defined, now the max allowed on
> > the entire flag setting.
> >
> >   Luis
> 
> Yes, I can certainly add check to see if the flags are out of range.
> However, I would like to know your opinion of what to do when an invalid
> flag bit is set. Do we just print a warning in the ring buffer or fail
> the registration of the ctl table?

We should fail setting. See sysctl_check_table_array(), that should just
reject the entry.

  Luis