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
