Re: Race condition in ipv6 code

[Ben Greear <greearb@candelatech.com>]

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?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com