| Message ID | 1519764591-27456-3-git-send-email-longman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 27, 2018 at 03:49:48PM -0500, Waiman Long wrote: > When minimum/maximum values are specified for a sysctl parameter in > the ctl_table structure with proc_dointvec_minmax() handler, an > 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. Makes me wonder if we should add something which does let one query for the ranges. Then scripts can fetch that as well. > 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. > > Signed-off-by: Waiman Long <longman@redhat.com> > --- > include/linux/sysctl.h | 6 ++++++ > kernel/sysctl.c | 58 ++++++++++++++++++++++++++++++++++++++++---------- > 2 files changed, 53 insertions(+), 11 deletions(-) > > diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h > index b769ecf..eceeaee 100644 > --- 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; > struct ctl_table *child; /* Deprecated */ > proc_handler *proc_handler; /* Callback for text formatting */ > struct ctl_table_poll *poll; > @@ -123,6 +124,11 @@ struct ctl_table > void *extra2; > } __randomize_layout; > > +/* > + * ctl_table flags (16 different flags, at most) > + */ > +#define CTL_FLAGS_CLAMP_RANGE (1 << 0) /* Clamp to min/max range */ Since its only 16 best we kdocify, we can do so with /** * enum ctl_table_flags - flags for the ctl table * * @CTL_FLAGS_CLAMP_RANGE: If set this indicates that the entry should be * flexibly clamp to min/max range in case the user provided an incorrect * value. */ enum ctl_table_flags { CTL_FLAGS_CLAMP_RANGE = BIT(0), } This lets us document this nicely. > + > struct ctl_node { > struct rb_node node; > struct ctl_table_header *header; > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index 52b647a..2b2b30c 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -2505,15 +2505,21 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write, > * > * The do_proc_dointvec_minmax_conv_param structure provides the > * minimum and maximum values for doing range checking for those sysctl > - * parameters that use the proc_dointvec_minmax() handler. The error > - * code -EINVAL will be returned if the range check fails. > + * parameters that use the proc_dointvec_minmax() handler. > + * > + * The error code -EINVAL will be returned if the range check fails > + * and the CTL_FLAGS_CLAMP_RANGE bit is not set in the given flags. > + * If that flag is set, the new sysctl value will be clamped to the > + * given range without returning any error. This last part seems odd, we silently set the value to a limit if the user set an invalid value? Since this is actually not really undefined documenting that we set it to the max value if the input value is greater than the max allowed would be good. Likewise for the minimum. Luis
On 02/27/2018 07:47 PM, Luis R. Rodriguez wrote: > On Tue, Feb 27, 2018 at 03:49:48PM -0500, Waiman Long wrote: >> When minimum/maximum values are specified for a sysctl parameter in >> the ctl_table structure with proc_dointvec_minmax() handler, > an > >> 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. > Makes me wonder if we should add something which does let one query > for the ranges. Then scripts can fetch that as well. That will actually be better than printing out the range in the dmesg log. However, I haven't figured out an easy way of doing that. If you have any suggestion, please let me know about it. > >> 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. >> >> Signed-off-by: Waiman Long <longman@redhat.com> >> --- >> include/linux/sysctl.h | 6 ++++++ >> kernel/sysctl.c | 58 ++++++++++++++++++++++++++++++++++++++++---------- >> 2 files changed, 53 insertions(+), 11 deletions(-) >> >> diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h >> index b769ecf..eceeaee 100644 >> --- 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; >> struct ctl_table *child; /* Deprecated */ >> proc_handler *proc_handler; /* Callback for text formatting */ >> struct ctl_table_poll *poll; >> @@ -123,6 +124,11 @@ struct ctl_table >> void *extra2; >> } __randomize_layout; >> >> +/* >> + * ctl_table flags (16 different flags, at most) >> + */ >> +#define CTL_FLAGS_CLAMP_RANGE (1 << 0) /* Clamp to min/max range */ > Since its only 16 best we kdocify, we can do so with > > /** > * enum ctl_table_flags - flags for the ctl table > * > * @CTL_FLAGS_CLAMP_RANGE: If set this indicates that the entry should be > * flexibly clamp to min/max range in case the user provided an incorrect > * value. > */ > enum ctl_table_flags { > CTL_FLAGS_CLAMP_RANGE = BIT(0), > } > > This lets us document this nicely. Thanks for the suggestion. Will update the code accordingly. >> + >> struct ctl_node { >> struct rb_node node; >> struct ctl_table_header *header; >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index 52b647a..2b2b30c 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -2505,15 +2505,21 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write, >> * >> * The do_proc_dointvec_minmax_conv_param structure provides the >> * minimum and maximum values for doing range checking for those sysctl >> - * parameters that use the proc_dointvec_minmax() handler. The error >> - * code -EINVAL will be returned if the range check fails. >> + * parameters that use the proc_dointvec_minmax() handler. >> + * >> + * The error code -EINVAL will be returned if the range check fails >> + * and the CTL_FLAGS_CLAMP_RANGE bit is not set in the given flags. >> + * If that flag is set, the new sysctl value will be clamped to the >> + * given range without returning any error. > This last part seems odd, we silently set the value to a limit if the > user set an invalid value? > > Since this is actually not really undefined documenting that we set it > to the max value if the input value is greater than the max allowed would > be good. Likewise for the minimum. > > Luis Will clarify the comment on that. Cheers, Longman
On Wed, Feb 28, 2018 at 12:53:40PM -0500, Waiman Long wrote: > On 02/27/2018 07:47 PM, Luis R. Rodriguez wrote: > > On Tue, Feb 27, 2018 at 03:49:48PM -0500, Waiman Long wrote: > >> When minimum/maximum values are specified for a sysctl parameter in > >> the ctl_table structure with proc_dointvec_minmax() handler, > > an > > > >> 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. > > Makes me wonder if we should add something which does let one query > > for the ranges. Then scripts can fetch that as well. > > That will actually be better than printing out the range in the dmesg > log. However, I haven't figured out an easy way of doing that. If you > have any suggestion, please let me know about it. I think a macro that also adds yet another proc read-only entry with a postfix "_range" with an internal handler which prints the range may suffice. Luis
On 02/28/2018 01:43 PM, Luis R. Rodriguez wrote: > On Wed, Feb 28, 2018 at 12:53:40PM -0500, Waiman Long wrote: >> On 02/27/2018 07:47 PM, Luis R. Rodriguez wrote: >>> On Tue, Feb 27, 2018 at 03:49:48PM -0500, Waiman Long wrote: >>>> When minimum/maximum values are specified for a sysctl parameter in >>>> the ctl_table structure with proc_dointvec_minmax() handler, >>> an >>> >>>> 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. >>> Makes me wonder if we should add something which does let one query >>> for the ranges. Then scripts can fetch that as well. >> That will actually be better than printing out the range in the dmesg >> log. However, I haven't figured out an easy way of doing that. If you >> have any suggestion, please let me know about it. > I think a macro that also adds yet another proc read-only entry with a postfix > "_range" with an internal handler which prints the range may suffice. > > Luis I think that is a possible solution. Instead of adding a macro, I will add one more flag which does the magic when the ctl_table entry is being processed. I think that will be simpler from the user point of view. Cheers, Longman
On Wed, Feb 28, 2018 at 01:58:53PM -0500, Waiman Long wrote: > On 02/28/2018 01:43 PM, Luis R. Rodriguez wrote: > > On Wed, Feb 28, 2018 at 12:53:40PM -0500, Waiman Long wrote: > >> On 02/27/2018 07:47 PM, Luis R. Rodriguez wrote: > >>> On Tue, Feb 27, 2018 at 03:49:48PM -0500, Waiman Long wrote: > >>>> When minimum/maximum values are specified for a sysctl parameter in > >>>> the ctl_table structure with proc_dointvec_minmax() handler, > >>> an > >>> > >>>> 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. > >>> Makes me wonder if we should add something which does let one query > >>> for the ranges. Then scripts can fetch that as well. > >> That will actually be better than printing out the range in the dmesg > >> log. However, I haven't figured out an easy way of doing that. If you > >> have any suggestion, please let me know about it. > > I think a macro that also adds yet another proc read-only entry with a postfix > > "_range" with an internal handler which prints the range may suffice. > > > > Luis > > I think that is a possible solution. Instead of adding a macro, I will > add one more flag which does the magic when the ctl_table entry is being > processed. I think that will be simpler from the user point of view. Agreed, flag should work nicely too. And likely easier to read / review and maintain. Luis
On 02/28/2018 01:58 PM, Waiman Long wrote: > On 02/28/2018 01:43 PM, Luis R. Rodriguez wrote: >> On Wed, Feb 28, 2018 at 12:53:40PM -0500, Waiman Long wrote: >>> On 02/27/2018 07:47 PM, Luis R. Rodriguez wrote: >>>> On Tue, Feb 27, 2018 at 03:49:48PM -0500, Waiman Long wrote: >>>>> When minimum/maximum values are specified for a sysctl parameter in >>>>> the ctl_table structure with proc_dointvec_minmax() handler, >>>> an >>>> >>>>> 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. >>>> Makes me wonder if we should add something which does let one query >>>> for the ranges. Then scripts can fetch that as well. >>> That will actually be better than printing out the range in the dmesg >>> log. However, I haven't figured out an easy way of doing that. If you >>> have any suggestion, please let me know about it. >> I think a macro that also adds yet another proc read-only entry with a postfix >> "_range" with an internal handler which prints the range may suffice. >> >> Luis > I think that is a possible solution. Instead of adding a macro, I will > add one more flag which does the magic when the ctl_table entry is being > processed. I think that will be simpler from the user point of view. > > Cheers, > Longman > This patch will take a bit more time to work on. So I am going to do it as a separate patch on top of the current one later. I don't want to delay the review of the current patch set. Cheers, Longman
diff --git a/include/linux/sysctl.h b/include/linux/sysctl.h index b769ecf..eceeaee 100644 --- 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; struct ctl_table *child; /* Deprecated */ proc_handler *proc_handler; /* Callback for text formatting */ struct ctl_table_poll *poll; @@ -123,6 +124,11 @@ struct ctl_table void *extra2; } __randomize_layout; +/* + * ctl_table flags (16 different flags, at most) + */ +#define CTL_FLAGS_CLAMP_RANGE (1 << 0) /* Clamp to min/max range */ + struct ctl_node { struct rb_node node; struct ctl_table_header *header; diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 52b647a..2b2b30c 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -2505,15 +2505,21 @@ static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write, * * The do_proc_dointvec_minmax_conv_param structure provides the * minimum and maximum values for doing range checking for those sysctl - * parameters that use the proc_dointvec_minmax() handler. The error - * code -EINVAL will be returned if the range check fails. + * parameters that use the proc_dointvec_minmax() handler. + * + * The error code -EINVAL will be returned if the range check fails + * and the CTL_FLAGS_CLAMP_RANGE bit is not set in the given flags. + * If that flag is set, the new sysctl value will be clamped to the + * given range without returning any error. * * min: ptr to minimum allowable value * max: ptr to maximum allowable value + * flags: ptr to flags */ struct do_proc_dointvec_minmax_conv_param { int *min; int *max; + uint16_t *flags; }; static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, @@ -2523,9 +2529,21 @@ static int do_proc_dointvec_minmax_conv(bool *negp, unsigned long *lvalp, struct do_proc_dointvec_minmax_conv_param *param = data; if (write) { int val = *negp ? -*lvalp : *lvalp; - if ((param->min && *param->min > val) || - (param->max && *param->max < val)) - return -EINVAL; + bool clamp = param->flags && + (*param->flags & CTL_FLAGS_CLAMP_RANGE); + + if (param->min && *param->min > val) { + if (clamp) + val = *param->min; + else + return -EINVAL; + } + if (param->max && *param->max < val) { + if (clamp) + val = *param->max; + else + return -EINVAL; + } *valp = val; } else { int val = *valp; @@ -2562,6 +2580,7 @@ int proc_dointvec_minmax(struct ctl_table *table, int write, struct do_proc_dointvec_minmax_conv_param param = { .min = (int *) table->extra1, .max = (int *) table->extra2, + .flags = &table->flags, }; return do_proc_dointvec(table, write, buffer, lenp, ppos, do_proc_dointvec_minmax_conv, ¶m); @@ -2572,15 +2591,21 @@ int proc_dointvec_minmax(struct ctl_table *table, int write, * * The do_proc_dointvec_minmax_conv_param structure provides the * minimum and maximum values for doing range checking for those sysctl - * parameters that use the proc_douintvec_minmax() handler. The error - * code -ERANGE will be returned if the range check fails. + * parameters that use the proc_douintvec_minmax() handler. + * + * The error code -ERANGE will be returned if the range check fails + * and the CTL_FLAGS_CLAMP_RANGE bit is not set in the given flags. + * If that flag is set, the new sysctl value will be clamped to the + * given range without returning any error. * * min: ptr to minimum allowable value * max: ptr to maximum allowable value + * flags: ptr to flags */ struct do_proc_douintvec_minmax_conv_param { unsigned int *min; unsigned int *max; + uint16_t *flags; }; static int do_proc_douintvec_minmax_conv(unsigned long *lvalp, @@ -2591,14 +2616,24 @@ static int do_proc_douintvec_minmax_conv(unsigned long *lvalp, if (write) { unsigned int val = *lvalp; + bool clamp = param->flags && + (*param->flags & CTL_FLAGS_CLAMP_RANGE); if (*lvalp > UINT_MAX) return -EINVAL; - if ((param->min && *param->min > val) || - (param->max && *param->max < val)) - return -ERANGE; - + if (param->min && *param->min > val) { + if (clamp) + val = *param->min; + else + return -ERANGE; + } + if (param->max && *param->max < val) { + if (clamp) + val = *param->max; + else + return -ERANGE; + } *valp = val; } else { unsigned int val = *valp; @@ -2633,6 +2668,7 @@ int proc_douintvec_minmax(struct ctl_table *table, int write, struct do_proc_douintvec_minmax_conv_param param = { .min = (unsigned int *) table->extra1, .max = (unsigned int *) table->extra2, + .flags = &table->flags, }; return do_proc_douintvec(table, write, buffer, lenp, ppos, do_proc_douintvec_minmax_conv, ¶m);
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. Signed-off-by: Waiman Long <longman@redhat.com> --- include/linux/sysctl.h | 6 ++++++ kernel/sysctl.c | 58 ++++++++++++++++++++++++++++++++++++++++---------- 2 files changed, 53 insertions(+), 11 deletions(-)