On Mon, 2012-07-09 at 12:15 +0300, Arik Nemtsov wrote: > On Mon, Jul 9, 2012 at 12:10 PM, Johannes Berg > <johannes@xxxxxxxxxxxxxxxx> wrote: > > On Mon, 2012-07-09 at 11:48 +0300, Arik Nemtsov wrote: > >> On Mon, Jul 9, 2012 at 10:59 AM, Johannes Berg > >> <johannes@xxxxxxxxxxxxxxxx> wrote: > >> > On Sun, 2012-07-08 at 19:27 +0300, Arik Nemtsov wrote: > >> >> On Sat, Jul 7, 2012 at 12:05 AM, Johannes Berg > >> >> <johannes@xxxxxxxxxxxxxxxx> wrote: > >> >> > From: Johannes Berg <johannes.berg@xxxxxxxxx> > >> >> > > >> >> > Making the scan_sdata pointer usable with RCU makes > >> >> > it possible to dereference it in the RX path to see > >> >> > if a received frame actually matches the interface > >> >> > that is scanning. This is just preparations, making > >> >> > the pointer __rcu. > >> >> > >> >> I noticed no synchronize_rcu() in the start/stop scan calls. Good/bad idea? > >> > > >> > Well, start() certainly wouldn't need it since you'd only get NULL :-) > >> > > >> > stop() in theory could use it, but it doesn't actually matter because as > >> > long as the interface still exists the pointer is valid. We don't free > >> > the interface in scan stop, so we don't need to make sure that the > >> > pointer is cleared before we continue. And in the case that we *do* in > >> > fact clear the interface (when it's going down) we have synchronize_rcu > >> > already in those code paths due to say the interface list with RCU > >> > protection. > >> > >> I meant protecting these (in patch 2/3): > >> > >> - local->sched_scanning, > >> + rcu_dereference_protected(local->sched_scan_sdata, > >> + lockdep_is_held(&local->mtx)), > >> > >> The check is obviously racy here, but it was racy before as well I guess. > >> I'm not sure why something line test_bit(SCHED_SCANNING) wasn't used > >> in these places. > > > > I don't think I understand what you're trying to say ... why is this > > racy? We hold the mutex that we always hold when we assign the pointer. > > I mean this check in ieee80211_rx_h_passive_scan(): > > if (test_bit(SCAN_HW_SCANNING, &local->scanning) || > test_bit(SCAN_SW_SCANNING, &local->scanning) || > test_bit(SCAN_ONCHANNEL_SCANNING, &local->scanning) || > local->sched_scanning) > return ieee80211_scan_rx(rx->sdata, skb); > > since this is RCU, the pointer might be there a while longer after the > scan finished.. Oh. I was looking at the code after patch 3 and this no longer exists ;-) But then my first argument applies -- as long as the interface is there, the pointer is OK, and when the interface is removed we need to remove it from the RCU-managed interface list so need to synchronize_rcu() already. No? johannes -- 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
