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 <[email protected]> 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
