Re: [PATCH 1/9] mac80211: Fix RCU pointer dereference in mesh_path_discard_frame()

[Johannes Berg <johannes@sipsolutions.net>]

On Thu, 25 Aug 2011 11:45:11 -0700, Javier Cardona wrote:
> On Thu, Aug 25, 2011 at 11:21 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Thu, 25 Aug 2011 11:16:50 -0700, Javier Cardona wrote:
>>>
>>> On Wed, Aug 24, 2011 at 7:08 PM, Johannes Berg
>>> <johannes@sipsolutions.net> wrote:
>>>>
>>>> On Wed, 24 Aug 2011 18:40:44 -0700, Thomas Pedersen wrote:
>>>>
>>>>>                da = hdr->addr3;
>>>>>                ra = hdr->addr1;
>>>>> +               rcu_read_lock();
>>>>>                mpath = mesh_path_lookup(da, sdata);
>>>>> +               rcu_read_unlock();
>>>>>                if (mpath)
>>>>>                        sn = ++mpath->sn;
>>>>>               
>>>>>  mesh_path_error_tx(sdata->u.mesh.mshcfg.element_ttl,
>>>>> skb->data,
>>>>
>>>> You've got to be kidding. Didn't I just explain RCU :)
>>>
>>> The patch was prepared before your RCU session :(
>>> Just to confirm I got it right before we resubmit: given that not 
>>> only
>>> the path table accessed inside mesh_path_lookup() but also the 
>>> mpaths
>>> themselves are RCU protected, the right fix should have been
>>>
>>>               da = hdr->addr3;
>>>               ra = hdr->addr1;
>>> +             rcu_read_lock();
>>>               mpath = mesh_path_lookup(da, sdata);
>>>               if (mpath)
>>>                       sn = ++mpath->sn;
>>> +             rcu_read_unlock();
>>>               mesh_path_error_tx(sdata->u.mesh.mshcfg.element_ttl,
>>> skb->data,
>>>
>>> Correct?
>>
>> Frankly, I'm not sure, since you modify the mpath->sn you probably 
>> need to
>> hold a real lock, otherwise ++mpath->sn can race against itself in 
>> this very
>> function.
>
> Oh, I see.  That's a different issue from what I was originally 
> trying
> to fix (unprotected access to the path table inside the lookup
> function).  But you are completely right.  Changing the math sequence
> number requires taking the mpath state lock:
>
>              da = hdr->addr3;
>              ra = hdr->addr1;
> +           rcu_read_lock();
>              mpath = mesh_path_lookup(da, sdata);
> -             if (mpath)
> +             if (mpath) {
> +                    spin_lock_bh(&mpath->state_lock);
>                       sn = ++mpath->sn;
> +                    spin_unlock_bh(&mpath->state_lock);
> +           }
> +           rcu_read_unlock();
>              mesh_path_error_tx(sdata->u.mesh.mshcfg.element_ttl, 
> skb->data,

Seems about right to me, but I don't know about all the locking :)

johannes