Re: [PATCH 1/9] mac80211: Fix RCU pointer dereference in mesh_path_discard_frame()
From: Javier Cardona <hidden>
Date: 2011-08-25 19:05:09
On Thu, Aug 25, 2011 at 11:48 AM, Johannes Berg [off-list ref] wrote:
On Thu, 25 Aug 2011 11:45:11 -0700, Javier Cardona wrote:quoted
On Thu, Aug 25, 2011 at 11:21 AM, Johannes Berg [off-list ref] wrote:quoted
On Thu, 25 Aug 2011 11:16:50 -0700, Javier Cardona wrote:quoted
On Wed, Aug 24, 2011 at 7:08 PM, Johannes Berg [off-list ref] wrote:quoted
On Wed, 24 Aug 2011 18:40:44 -0700, Thomas Pedersen wrote:quoted
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