Re: [PATCH v2 2/5] sysctl: Add flags to support min/max range clamping

[Waiman Long <longman@redhat.com>]

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