Re: Race condition in ipv6 code

[ebiederm@xmission.com (Eric W. Biederman)]

Ben Greear <greearb@candelatech.com> writes:

> On 01/13/2012 09:46 PM, Eric W. Biederman wrote:
>> Ben Greear<greearb@candelatech.com>  writes:
>>
>>> On 01/12/2012 11:40 PM, Eric W. Biederman wrote:
>>>
>>>> So I really think the best solution to avoid the locking craziness is to
>>>> have a wrapper that gets the value from userspace and calls
>>>> schedule_work to get another thread to actually process the change.  I
>>>> don't see any problems with writing a helper function for that.  The
>>>> only downside with using schedule_work is that we return to userspace
>>>> before the change has been fully installed in the kernel.  I don't
>>>> expect that would be a problem but stranger things have happened.
>>>
>>> That sounds a bit risky to me.  If something sets a value, and then
>>> queries it, it should always show the proper result for the previous
>>> calls.
>>
>> Which is easy to do if you keep two values.  One integer
>> for the userspace control and another integer for the internal
>> kernel state.
>>
>> The problem is that we have exactly one integer currently.
>>
>>> If the queries also went through the the same sched-work queue
>>> then maybe it would be OK.
>>
>> We can't want for anything that has to take the rtnl_lock.  That would
>> be the same as taking the rtnl_lock from a locking perspective.
>>
>> I expect I would use something like:
>> struct rtnl_protected_knob {
>> 	struct work_struct work;
>>          int userspace_value;
>>          int *kernel_var;
>>          void (*func)(int new_value, *kernel_var);
>> };
>>
>> userspace_value would be what userspace sees, and kernel_var would be a
>> pointer to the value that we manipulate in the kernel.
>
> What if valid values are 0-5 and user sets value to 6 and then immediately
> queries the value?  Would your method possibly return 6, when in fact when
> the kernel does the work it will internally either reject the setting and
> stay with the old value or round the 6 down to 5?

Cancel this schedule_work suggestion.  I have played with the problem a bit
and it looks feasible to add some unlocked notifications (called
asynchronously from workqueues) to add and delete the sysfs, proc and sysctl
bits.  At which point we can then just do a straight forward rtnl_lock
in the handlers.

I still think Francesco's patch looks like the best suggestion I have
seen so far.  Francesco's patch fixes a location where we don't have the
rtnl_lock when we want it and it gets it handles the rest in a simple
straight forward way.

Eric