On Thu, Aug 25, 2011 at 11:48 AM, Johannes Berg
<johannes@xxxxxxxxxxxxxxxx> wrote:
> On Thu, 25 Aug 2011 11:45:11 -0700, Javier Cardona wrote:
>>
>> On Thu, Aug 25, 2011 at 11:21 AM, Johannes Berg
>> <johannes@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx> 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 :)
After your RCU session I went back to the path table module and
reviewed the locking. That's what triggered some the patches on the
set:
http://thread.gmane.org/gmane.linux.kernel.wireless.general/75812/focus=75816
http://thread.gmane.org/gmane.linux.kernel.wireless.general/75812/focus=75818
http://thread.gmane.org/gmane.linux.kernel.wireless.general/75812/focus=75820
Mesh path tables and mpaths are protected by RCU.
The internal state of each mpath is protected by mpath->state_lock.
And there's a lock for each bucket on the mesh path tables.
Thanks!
Javier
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
