Re: [PATCH 1/9] mac80211: Fix RCU pointer dereference in mesh_path_discard_frame()
From: Johannes Berg <johannes@sipsolutions.net>
Date: 2011-08-25 18:48:10
On Thu, 25 Aug 2011 11:45:11 -0700, Javier Cardona wrote:
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 :) johannes