Re: [PATCH v3 3/6] sysctl: Add flags to support min/max range clamping

[Waiman Long]

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@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

OK, got it.

Cheers,
Longman