Re: [PATCH v3 4/6] sysctl: Warn when a clamped sysctl parameter is set out of range

[Waiman Long]

On 03/01/2018 04:38 PM, Andrew Morton wrote:
> On Thu,  1 Mar 2018 12:43:38 -0500 Waiman Long <longman@xxxxxxxxxx> wrote:
>
>> Even with clamped sysctl parameters, it is still not that straight
>> forward to figure out the exact range of those parameters. One may
>> try to write extreme parameter values to see if they get clamped.
>> To make it easier, a warning with the expected range will now be
>> printed in the kernel ring buffer when a clamped sysctl parameter
>> receives an out of range value.
>>
>> ...
>>
>> +		if (clamped && param->name &&
>> +		   !(*param->flags & CTL_FLAGS_OOR_WARNED)) {
>> +			proc_ctl_warn(d, param->name,
>> +				param->min ? *param->min : -INT_MAX,
>> +				param->max ? *param->max :  INT_MAX, val);
>> +			*param->flags |= CTL_FLAGS_OOR_WARNED;
>> +		}
> The handling of ctl_table.flags looks racy on SMP or preemptible. 
> That's not at all a serious problem in this usage, but such handling of
> ctl_table.flags may be a problem in the future.  Which means that if
> some future user of this field *is* sensitive to races then people are
> going to have to come back to this code and add the needed locking.
>
> So we should at least think about what that locking is to be, and
> document it in some fashion.  Do we already hold an appropriate lock at
> this time?  If so, what is it?
>
> If some such future user of ctl_table.flags has to add a new lock to
> the ctl_table for this purpose then we just eliminated your use-16-bit
> space saving trick and we may as well use a ulong and operate on it
> with bitops.

We don't actually need locking if it is only the flags that we are
worrying about. Doing some kind of atomic bit setting should be enough.
I should probably add some comment to elaborate a bit more on this.
Thanks for reminding me about this forward looking concern.

Cheers,
Longman