| Message ID | 20180301133117.1d4b27e327f8e51275c82489@linux-foundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/01/2018 04:31 PM, Andrew Morton wrote: > On Thu, 1 Mar 2018 12:43:37 -0500 Waiman Long <longman@redhat.com> 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... > > --- a/include/linux/sysctl.h~sysctl-add-flags-to-support-min-max-range-clamping-fix > +++ a/include/linux/sysctl.h > @@ -116,7 +116,7 @@ struct ctl_table > void *data; > int maxlen; > umode_t mode; > - uint16_t flags; > + uint16_t flags; /* enum ctl_table_flags */ > struct ctl_table *child; /* Deprecated */ > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > @@ -125,7 +125,7 @@ struct ctl_table > } __randomize_layout; > > /** > - * enum ctl_table_flags - flags for the ctl table > + * enum ctl_table_flags - flags for the ctl table (struct ctl_table.flags) > * > * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be > * flexibly clamped to min/max range in case the user provided The flags field is actually a bit mask of various bits defined in the enum type. So I will not change the type of flags to enum ctl_table_flags as it is not strictly an enum type. However, I can certainly add the suggested comment change in the next version of the patch. Cheers, Longman
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@redhat.com> 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. Luis
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@redhat.com> 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
On 03/08/2018 12:51 PM, 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@redhat.com> 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... >> Actually, I can make the flags just an unsigned integer. I chose uint16_t for backporting purpose. For upstream code, it is not a concern at all. That will allow more bits for expansion in the future. > 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; > } I don't think that helper function works unless the parameter is changed to int instead of u16. I will just change the flags to uint and forget about checking for out-of-range bit. Cheers, Longman
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@redhat.com> 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? Cheers, Longman
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@redhat.com> 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
On 03/08/2018 03:45 PM, Luis R. Rodriguez wrote: > 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@redhat.com> 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 OK, got it. Cheers, Longman
--- a/include/linux/sysctl.h~sysctl-add-flags-to-support-min-max-range-clamping-fix +++ a/include/linux/sysctl.h @@ -116,7 +116,7 @@ struct ctl_table void *data; int maxlen; umode_t mode; - uint16_t flags; + uint16_t flags; /* enum ctl_table_flags */ struct ctl_table *child; /* Deprecated */ proc_handler *proc_handler; /* Callback for text formatting */ struct ctl_table_poll *poll; @@ -125,7 +125,7 @@ struct ctl_table } __randomize_layout; /** - * enum ctl_table_flags - flags for the ctl table + * enum ctl_table_flags - flags for the ctl table (struct ctl_table.flags) * * @CTL_FLAGS_CLAMP_RANGE: Set to indicate that the entry should be * flexibly clamped to min/max range in case the user provided