On Wednesday 06 October 2010 12:41:45 Johannes Berg wrote: > On Wed, 2010-10-06 at 12:20 +0200, Christian Lamparter wrote: > > On Wednesday 06 October 2010 12:10:26 Johannes Berg wrote: > > > On Wed, 2010-10-06 at 12:00 +0200, Christian Lamparter wrote: > > > > > > > The timer itself is part of the station's private struct. > > > > The clean-up routine will deactivate the timer as soon as > > > > the station is removed. Therefore the extra sta->lock > > > > protection should not be necessary. > > > > > > > rcu_read_lock(); > > > > - spin_lock(&sta->lock); > > > > ieee80211_release_reorder_timeout(sta, *ptid); > > > > - spin_unlock(&sta->lock); > > > > rcu_read_unlock(); > > > > > > There's a comment on ieee80211_release_reorder_timeout() saying that the > > > lock must be held -- which is probably not true? We don't generally hold > > > that lock on the RX path...? > > > > That comment is more or less a 1:1 copy from the comment about > > struct tid_ampdu_rx (in sta_info.h). > > Kinda. > > > > * This structure is protected by RCU and the per-station > > > * spinlock. Assignments to the array holding it must hold > > > * the spinlock, only the RX path can access it under RCU > > > * lock-free. > > > > thing is: we now have the reorder_lock which protects the > > reorder buffer against "destructive access". So, is it "ok" > > to trim the comments a bit? > > Well, so if this patch is OK, it would be, but looking at > tid_agg_rx->head_seq_num and buf_size for example, they're not always > protected by the reorder lock (though they could easily be). > > In fact, there are more races, like for example > ieee80211_release_reorder_frames not being invoked with the reorder lock > held from ieee80211_rx_h_ctrl, which could lead to issues? > > Basically the thing is that until your patch, the data in the struct > didn't actually need locking because it was accessed by the RX path only > which is not concurrent. > I see. So basically all rx handlers are affected by these rx->sta races. John, can you please revert (or at least drop from the upcoming 2.6.37-rcX cycle): (mac80211: fix release_reorder_timeout in scan) mac80211: fix rcu-unsafe pointer dereference mac80211: AMPDU rx reorder timeout timer (mac80211: remove unused rate function parameter) mac80211: put rx handlers into separate functions -- 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
