Re: 3.9-rc1 NULL pointer crash at find_pid_ns

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

Sasha Levin <sasha.levin@oracle.com> writes:

> On 03/07/2013 01:05 PM, ebiederm@xmission.com wrote:
>> Sasha Levin <sasha.levin@oracle.com> writes:
>> 
>>> On 03/07/2013 12:46 PM, Eric Dumazet wrote:
>>>> On Thu, 2013-03-07 at 12:36 -0500, Sasha Levin wrote:
>>>>
>>>>> Looks like the hlist change is probably the issue, though it specifically
>>>>> uses:
>>>>>
>>>>> 	#define hlist_entry_safe(ptr, type, member) \
>>>>>         	(ptr) ? hlist_entry(ptr, type, member) : NULL
>>>>>
>>>>> I'm still looking at the code in question and it's assembly, but I can't
>>>>> figure out what's going wrong. I was also trying to see what's so special
>>>>> about this loop in find_pid_ns as opposed to the rest of the kernel code
>>>>> that uses hlist_for_each_entry_rcu() but couldn't find out why.
>>>>>
>>>>> Is it somehow possible that if we rcu_dereference_raw() the same thing twice
>>>>> inside the same rcu_read_lock() section we'll get different results? That's
>>>>> really the only reason for this crash that comes to mind at the moment, very
>>>>> unlikely - but that's all I have right now.
>>>>>
>>>>
>>>> Yep
>>>>
>>>> #define hlist_entry_safe(ptr, type, member) \
>>>> 	(ptr) ? hlist_entry(ptr, type, member) : NULL
>>>>
>>>> Is not safe, as ptr can be evaluated twice, and thats not good at all...
>>>
>>> ptr is being evaluated twice, but in this case this is an
>>> rcu_dereference_raw() value within the same rcu_read_lock() section.
>>>
>>> Is it still problematic?
>> 
>> Definitely.
>> 
>> Head in this instance the expression: &pid_hash[pid_hashfn(nr, ns)]
>> 
>> And the crash clearly shows that when hilst_entry is being evaluated the
>> HEAD is NULL.
>
> Okay, I'm even more confused now.
>
> The expression in question is:
>
> 	hlist_entry_safe(rcu_dereference_bh(hlist_first_rcu(head)))
>
> You're saying that "rcu_dereference_bh(hlist_first_rcu(head))" can change between
> the two evaluations we do. That would mean that 'head' has changed in between, right?
>
> In that case, the list itself has changed - which means that RCU has changed the
> list underneath us.
>
> hlist_first_rcu() doesn't have any side-effects, it doesn't modify the list whatsoever,
> so the only thing that can change is 'head'. Why is it allowed to change if the list
> is protected by RCU?

The pointer to the first element of the list goes to NULL.

With RCU pointers can change and the guranateee that is made is that if
you follow a stale pointer the storage pointed to by the stale pointer
does not become invalid until you exit the rcu critical section.

Eric