[RFC 0/3] mac80211 scanning restructuring

[RFC 0/3] mac80211 scanning restructuring

[Johannes Berg]

I decided that with multi-channel coming and thus us using more
virtual interfaces, the scanning code was going to be the first
victim of some factoring ;-)

Please review. The only thing that isn't quite clear to me is
whether or not I can really remove the channel == oper_channel
check, but it's only applied to probe resp/beacon frames so it
seems a bit pointless to try to keep it?

johannes

[RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

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.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/ieee80211_i.h |    2 +-
 net/mac80211/iface.c       |    9 +++++----
 net/mac80211/scan.c        |   33 ++++++++++++++++++++++++---------
 3 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index b0830ae..da66a73 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -974,7 +974,7 @@ struct ieee80211_local {
 	unsigned long leave_oper_channel_time;
 	enum mac80211_scan_state next_scan_state;
 	struct delayed_work scan_work;
-	struct ieee80211_sub_if_data *scan_sdata;
+	struct ieee80211_sub_if_data __rcu *scan_sdata;
 	enum nl80211_channel_type _oper_channel_type;
 	struct ieee80211_channel *oper_channel, *csa_channel;
 
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index c4c20cf7..43e1052 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -112,10 +112,11 @@ static u32 __ieee80211_recalc_idle(struct ieee80211_local *local)
 		}
 	}
 
-	if (local->scan_sdata &&
-	    !(local->hw.flags & IEEE80211_HW_SCAN_WHILE_IDLE)) {
+	sdata = rcu_dereference_protected(local->scan_sdata,
+					  lockdep_is_held(&local->mtx));
+	if (sdata && !(local->hw.flags & IEEE80211_HW_SCAN_WHILE_IDLE)) {
 		scanning = true;
-		local->scan_sdata->vif.bss_conf.idle = false;
+		sdata->vif.bss_conf.idle = false;
 	}
 
 	list_for_each_entry(sdata, &local->interfaces, list) {
@@ -636,7 +637,7 @@ static void ieee80211_do_stop(struct ieee80211_sub_if_data *sdata,
 
 	clear_bit(SDATA_STATE_RUNNING, &sdata->state);
 
-	if (local->scan_sdata == sdata)
+	if (rcu_access_pointer(local->scan_sdata) == sdata)
 		ieee80211_scan_cancel(local);
 
 	/*
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index d1b4b9d..48d3219 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -294,7 +294,13 @@ static void __ieee80211_scan_completed(struct ieee80211_hw *hw, bool aborted,
 		return;
 
 	if (was_hw_scan && !aborted && ieee80211_prep_hw_scan(local)) {
-		int rc = drv_hw_scan(local, local->scan_sdata, local->hw_scan_req);
+		int rc;
+
+		rc = drv_hw_scan(local,
+			rcu_dereference_protected(local->scan_sdata,
+						  lockdep_is_held(&local->mtx)),
+			local->hw_scan_req);
+
 		if (rc == 0)
 			return;
 	}
@@ -395,7 +401,10 @@ void ieee80211_run_deferred_scan(struct ieee80211_local *local)
 	if (!local->scan_req || local->scanning)
 		return;
 
-	if (!ieee80211_can_scan(local, local->scan_sdata))
+	if (!ieee80211_can_scan(local,
+				rcu_dereference_protected(
+					local->scan_sdata,
+					lockdep_is_held(&local->mtx))))
 		return;
 
 	ieee80211_queue_delayed_work(&local->hw, &local->scan_work,
@@ -406,9 +415,12 @@ static void ieee80211_scan_state_send_probe(struct ieee80211_local *local,
 					    unsigned long *next_delay)
 {
 	int i;
-	struct ieee80211_sub_if_data *sdata = local->scan_sdata;
+	struct ieee80211_sub_if_data *sdata;
 	enum ieee80211_band band = local->hw.conf.channel->band;
 
+	sdata = rcu_dereference_protected(local->scan_sdata,
+					  lockdep_is_held(&local->mtx));;
+
 	for (i = 0; i < local->scan_req->n_ssids; i++)
 		ieee80211_send_probe_req(
 			sdata, NULL,
@@ -440,7 +452,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	if (!ieee80211_can_scan(local, sdata)) {
 		/* wait for the work to finish/time out */
 		local->scan_req = req;
-		local->scan_sdata = sdata;
+		rcu_assign_pointer(local->scan_sdata, sdata);
 		return 0;
 	}
 
@@ -474,7 +486,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 	}
 
 	local->scan_req = req;
-	local->scan_sdata = sdata;
+	rcu_assign_pointer(local->scan_sdata, sdata);
 
 	if (local->ops->hw_scan) {
 		__set_bit(SCAN_HW_SCANNING, &local->scanning);
@@ -534,7 +546,7 @@ static int __ieee80211_start_scan(struct ieee80211_sub_if_data *sdata,
 		ieee80211_recalc_idle(local);
 
 		local->scan_req = NULL;
-		local->scan_sdata = NULL;
+		rcu_assign_pointer(local->scan_sdata, NULL);
 	}
 
 	return rc;
@@ -721,7 +733,8 @@ void ieee80211_scan_work(struct work_struct *work)
 
 	mutex_lock(&local->mtx);
 
-	sdata = local->scan_sdata;
+	sdata = rcu_dereference_protected(local->scan_sdata,
+					  lockdep_is_held(&local->mtx));
 
 	/* When scanning on-channel, the first-callback means completed. */
 	if (test_bit(SCAN_ONCHANNEL_SCANNING, &local->scanning)) {
@@ -742,7 +755,7 @@ void ieee80211_scan_work(struct work_struct *work)
 		int rc;
 
 		local->scan_req = NULL;
-		local->scan_sdata = NULL;
+		rcu_assign_pointer(local->scan_sdata, NULL);
 
 		rc = __ieee80211_start_scan(sdata, req);
 		if (rc) {
@@ -894,7 +907,9 @@ void ieee80211_scan_cancel(struct ieee80211_local *local)
 
 	if (test_bit(SCAN_HW_SCANNING, &local->scanning)) {
 		if (local->ops->cancel_hw_scan)
-			drv_cancel_hw_scan(local, local->scan_sdata);
+			drv_cancel_hw_scan(local,
+				rcu_dereference_protected(local->scan_sdata,
+						lockdep_is_held(&local->mtx)));
 		goto out;
 	}
 
-- 
1.7.10.4

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Arik Nemtsov]

On Sat, Jul 7, 2012 at 12:05 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> 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?

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Johannes Berg]

On Sun, 2012-07-08 at 19:27 +0300, Arik Nemtsov wrote:
> On Sat, Jul 7, 2012 at 12:05 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > 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.

johannes

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Arik Nemtsov]

On Mon, Jul 9, 2012 at 10:59 AM, Johannes Berg
<johannes@sipsolutions.net> 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@sipsolutions.net> wrote:
>> > From: Johannes Berg <johannes.berg@intel.com>
>> >
>> > 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.

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Johannes Berg]

On Mon, 2012-07-09 at 11:48 +0300, Arik Nemtsov wrote:
> On Mon, Jul 9, 2012 at 10:59 AM, Johannes Berg
> <johannes@sipsolutions.net> 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@sipsolutions.net> wrote:
> >> > From: Johannes Berg <johannes.berg@intel.com>
> >> >
> >> > 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.

johannes

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Arik Nemtsov]

On Mon, Jul 9, 2012 at 12:10 PM, Johannes Berg
<johannes@sipsolutions.net> 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@sipsolutions.net> 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@sipsolutions.net> wrote:
>> >> > From: Johannes Berg <johannes.berg@intel.com>
>> >> >
>> >> > 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..

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Johannes Berg]

On Mon, 2012-07-09 at 12:15 +0300, Arik Nemtsov wrote:
> On Mon, Jul 9, 2012 at 12:10 PM, Johannes Berg
> <johannes@sipsolutions.net> 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@sipsolutions.net> 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@sipsolutions.net> wrote:
> >> >> > From: Johannes Berg <johannes.berg@intel.com>
> >> >> >
> >> >> > 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

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Arik Nemtsov]

On Mon, Jul 9, 2012 at 12:23 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2012-07-09 at 12:15 +0300, Arik Nemtsov wrote:
>> On Mon, Jul 9, 2012 at 12:10 PM, Johannes Berg
>> <johannes@sipsolutions.net> 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@sipsolutions.net> 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@sipsolutions.net> wrote:
>> >> >> > From: Johannes Berg <johannes.berg@intel.com>
>> >> >> >
>> >> >> > 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?

The add/remove interface part is covered, yes.

What happens when starting/stopping sched scan? The rcu pointer is
removed in ieee80211_request_sched_scan_stop(), but we may still think
we are sched scanning for a while inside
ieee80211_rx_h_passive_scan().

Probably nothing too bad will happen though..

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Johannes Berg]

On Mon, 2012-07-09 at 12:39 +0300, Arik Nemtsov wrote:

> >> >> >> 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?
> 
> The add/remove interface part is covered, yes.
> 
> What happens when starting/stopping sched scan? The rcu pointer is
> removed in ieee80211_request_sched_scan_stop(), but we may still think
> we are sched scanning for a while inside
> ieee80211_rx_h_passive_scan().
> 
> Probably nothing too bad will happen though..

Yes, well... Say we stick synchronize_rcu() into sched_scan_stop(). What
would actually change?
We'd still think we're sched scanning (in the RX handler) for exactly
the same amount of time, except the sched scan stop code would now wait
for it, while doing nothing. It has nothing to do after the wait (since
it doesn't free the RCU data or anything).

IOW -- nothing changes.

johannes

Re: [RFC 1/3] mac80211: make scan_sdata pointer usable with RCU

[Arik Nemtsov]

On Mon, Jul 9, 2012 at 12:43 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2012-07-09 at 12:39 +0300, Arik Nemtsov wrote:
>
>> >> >> >> 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?
>>
>> The add/remove interface part is covered, yes.
>>
>> What happens when starting/stopping sched scan? The rcu pointer is
>> removed in ieee80211_request_sched_scan_stop(), but we may still think
>> we are sched scanning for a while inside
>> ieee80211_rx_h_passive_scan().
>>
>> Probably nothing too bad will happen though..
>
> Yes, well... Say we stick synchronize_rcu() into sched_scan_stop(). What
> would actually change?
> We'd still think we're sched scanning (in the RX handler) for exactly
> the same amount of time, except the sched scan stop code would now wait
> for it, while doing nothing. It has nothing to do after the wait (since
> it doesn't free the RCU data or anything).

I was referring to using test_bit(SCHED_SCANNING). You're right
syncronize_rcu() doesn't do anything here (I even realized my mistake
somewhere along the email chain I think).

Arik

[RFC 2/3] mac80211: track scheduled scan virtual interface

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Instead of tracking whether or not we're in a
scheduled scan, track the virtual interface
(sdata) in an RCU-protected pointer to make it
usable from RX to check the MAC address.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/ieee80211_i.h |    2 +-
 net/mac80211/main.c        |    3 ++-
 net/mac80211/rx.c          |    4 ++--
 net/mac80211/scan.c        |   20 ++++++++++----------
 4 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index da66a73..2e7dc0c 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -967,9 +967,9 @@ struct ieee80211_local {
 	int scan_channel_idx;
 	int scan_ies_len;
 
-	bool sched_scanning;
 	struct ieee80211_sched_scan_ies sched_scan_ies;
 	struct work_struct sched_scan_stopped_work;
+	struct ieee80211_sub_if_data __rcu *sched_scan_sdata;
 
 	unsigned long leave_oper_channel_time;
 	enum mac80211_scan_state next_scan_state;
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 731681c..e706f9e 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -326,7 +326,8 @@ static void ieee80211_restart_work(struct work_struct *work)
 
 	mutex_lock(&local->mtx);
 	WARN(test_bit(SCAN_HW_SCANNING, &local->scanning) ||
-	     local->sched_scanning,
+	     rcu_dereference_protected(local->sched_scan_sdata,
+				       lockdep_is_held(&local->mtx)),
 		"%s called with hardware scan in progress\n", __func__);
 	mutex_unlock(&local->mtx);
 
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 14b5fb4..4e09b7f 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -461,13 +461,13 @@ ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
 	struct sk_buff *skb = rx->skb;
 
 	if (likely(!(status->rx_flags & IEEE80211_RX_IN_SCAN) &&
-		   !local->sched_scanning))
+		   !rcu_access_pointer(local->sched_scan_sdata)))
 		return RX_CONTINUE;
 
 	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)
+	    rcu_access_pointer(local->sched_scan_sdata))
 		return ieee80211_scan_rx(rx->sdata, skb);
 
 	/* scanning finished during invoking of handlers */
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 48d3219..a5848d2 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -931,9 +931,9 @@ int ieee80211_request_sched_scan_start(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_local *local = sdata->local;
 	int ret, i;
 
-	mutex_lock(&sdata->local->mtx);
+	mutex_lock(&local->mtx);
 
-	if (local->sched_scanning) {
+	if (rcu_access_pointer(local->sched_scan_sdata)) {
 		ret = -EBUSY;
 		goto out;
 	}
@@ -964,7 +964,7 @@ int ieee80211_request_sched_scan_start(struct ieee80211_sub_if_data *sdata,
 	ret = drv_sched_scan_start(local, sdata, req,
 				   &local->sched_scan_ies);
 	if (ret == 0) {
-		local->sched_scanning = true;
+		rcu_assign_pointer(local->sched_scan_sdata, sdata);
 		goto out;
 	}
 
@@ -972,7 +972,7 @@ out_free:
 	while (i > 0)
 		kfree(local->sched_scan_ies.ie[--i]);
 out:
-	mutex_unlock(&sdata->local->mtx);
+	mutex_unlock(&local->mtx);
 	return ret;
 }
 
@@ -981,22 +981,22 @@ int ieee80211_request_sched_scan_stop(struct ieee80211_sub_if_data *sdata)
 	struct ieee80211_local *local = sdata->local;
 	int ret = 0, i;
 
-	mutex_lock(&sdata->local->mtx);
+	mutex_lock(&local->mtx);
 
 	if (!local->ops->sched_scan_stop) {
 		ret = -ENOTSUPP;
 		goto out;
 	}
 
-	if (local->sched_scanning) {
+	if (rcu_access_pointer(local->sched_scan_sdata)) {
 		for (i = 0; i < IEEE80211_NUM_BANDS; i++)
 			kfree(local->sched_scan_ies.ie[i]);
 
 		drv_sched_scan_stop(local, sdata);
-		local->sched_scanning = false;
+		rcu_assign_pointer(local->sched_scan_sdata, NULL);
 	}
 out:
-	mutex_unlock(&sdata->local->mtx);
+	mutex_unlock(&local->mtx);
 
 	return ret;
 }
@@ -1020,7 +1020,7 @@ void ieee80211_sched_scan_stopped_work(struct work_struct *work)
 
 	mutex_lock(&local->mtx);
 
-	if (!local->sched_scanning) {
+	if (!rcu_access_pointer(local->sched_scan_sdata)) {
 		mutex_unlock(&local->mtx);
 		return;
 	}
@@ -1028,7 +1028,7 @@ void ieee80211_sched_scan_stopped_work(struct work_struct *work)
 	for (i = 0; i < IEEE80211_NUM_BANDS; i++)
 		kfree(local->sched_scan_ies.ie[i]);
 
-	local->sched_scanning = false;
+	rcu_assign_pointer(local->sched_scan_sdata, NULL);
 
 	mutex_unlock(&local->mtx);
 
-- 
1.7.10.4

[RFC 3/3] mac80211: redesign scan RX

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

Scan receive is rather inefficient when there are
multiple virtual interfaces. We iterate all of the
virtual interfaces and then notify cfg80211 about
each beacon many times.

Redesign scan RX to happen before everything else.
Then we can also get rid of IEEE80211_RX_IN_SCAN
since we don't have to accept frames into the RX
handlers for scanning or scheduled scanning any
more. Overall, this simplifies the code.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/mac80211/debugfs.c     |    2 --
 net/mac80211/ieee80211_i.h |    6 +----
 net/mac80211/rx.c          |   46 +++++++----------------------------
 net/mac80211/scan.c        |   57 ++++++++++++++++++--------------------------
 4 files changed, 32 insertions(+), 79 deletions(-)

diff --git a/net/mac80211/debugfs.c b/net/mac80211/debugfs.c
index 778e591..b8dfb44 100644
--- a/net/mac80211/debugfs.c
+++ b/net/mac80211/debugfs.c
@@ -325,8 +325,6 @@ void debugfs_hw_add(struct ieee80211_local *local)
 		local->rx_handlers_drop_defrag);
 	DEBUGFS_STATS_ADD(rx_handlers_drop_short,
 		local->rx_handlers_drop_short);
-	DEBUGFS_STATS_ADD(rx_handlers_drop_passive_scan,
-		local->rx_handlers_drop_passive_scan);
 	DEBUGFS_STATS_ADD(tx_expand_skb_head,
 		local->tx_expand_skb_head);
 	DEBUGFS_STATS_ADD(tx_expand_skb_head_cloned,
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2e7dc0c..653be8d 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -209,7 +209,6 @@ typedef unsigned __bitwise__ ieee80211_rx_result;
  * enum ieee80211_packet_rx_flags - packet RX flags
  * @IEEE80211_RX_RA_MATCH: frame is destined to interface currently processed
  *	(incl. multicast frames)
- * @IEEE80211_RX_IN_SCAN: received while scanning
  * @IEEE80211_RX_FRAGMENTED: fragmented frame
  * @IEEE80211_RX_AMSDU: a-MSDU packet
  * @IEEE80211_RX_MALFORMED_ACTION_FRM: action frame is malformed
@@ -219,7 +218,6 @@ typedef unsigned __bitwise__ ieee80211_rx_result;
  * @rx_flags field of &struct ieee80211_rx_status.
  */
 enum ieee80211_packet_rx_flags {
-	IEEE80211_RX_IN_SCAN			= BIT(0),
 	IEEE80211_RX_RA_MATCH			= BIT(1),
 	IEEE80211_RX_FRAGMENTED			= BIT(2),
 	IEEE80211_RX_AMSDU			= BIT(3),
@@ -1016,7 +1014,6 @@ struct ieee80211_local {
 	unsigned int rx_handlers_drop_nullfunc;
 	unsigned int rx_handlers_drop_defrag;
 	unsigned int rx_handlers_drop_short;
-	unsigned int rx_handlers_drop_passive_scan;
 	unsigned int tx_expand_skb_head;
 	unsigned int tx_expand_skb_head_cloned;
 	unsigned int rx_expand_skb_head;
@@ -1251,8 +1248,7 @@ int ieee80211_request_scan(struct ieee80211_sub_if_data *sdata,
 			   struct cfg80211_scan_request *req);
 void ieee80211_scan_cancel(struct ieee80211_local *local);
 void ieee80211_run_deferred_scan(struct ieee80211_local *local);
-ieee80211_rx_result
-ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb);
+void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb);
 
 void ieee80211_mlme_notify_scan_completed(struct ieee80211_local *local);
 struct ieee80211_bss *
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 4e09b7f..ee96960 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -453,29 +453,6 @@ static void ieee80211_verify_alignment(struct ieee80211_rx_data *rx)
 
 /* rx handlers */
 
-static ieee80211_rx_result debug_noinline
-ieee80211_rx_h_passive_scan(struct ieee80211_rx_data *rx)
-{
-	struct ieee80211_local *local = rx->local;
-	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(rx->skb);
-	struct sk_buff *skb = rx->skb;
-
-	if (likely(!(status->rx_flags & IEEE80211_RX_IN_SCAN) &&
-		   !rcu_access_pointer(local->sched_scan_sdata)))
-		return RX_CONTINUE;
-
-	if (test_bit(SCAN_HW_SCANNING, &local->scanning) ||
-	    test_bit(SCAN_SW_SCANNING, &local->scanning) ||
-	    test_bit(SCAN_ONCHANNEL_SCANNING, &local->scanning) ||
-	    rcu_access_pointer(local->sched_scan_sdata))
-		return ieee80211_scan_rx(rx->sdata, skb);
-
-	/* scanning finished during invoking of handlers */
-	I802_DEBUG_INC(local->rx_handlers_drop_passive_scan);
-	return RX_DROP_UNUSABLE;
-}
-
-
 static int ieee80211_is_unicast_robust_mgmt_frame(struct sk_buff *skb)
 {
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
@@ -2732,7 +2709,6 @@ static void ieee80211_invoke_rx_handlers(struct ieee80211_rx_data *rx)
 			goto rxh_next;  \
 	} while (0);
 
-	CALL_RXH(ieee80211_rx_h_passive_scan)
 	CALL_RXH(ieee80211_rx_h_check)
 
 	ieee80211_rx_reorder_ampdu(rx);
@@ -2802,11 +2778,8 @@ static int prepare_for_handlers(struct ieee80211_rx_data *rx,
 			return 0;
 		if (ieee80211_is_beacon(hdr->frame_control)) {
 			return 1;
-		}
-		else if (!ieee80211_bssid_match(bssid, sdata->u.ibss.bssid)) {
-			if (!(status->rx_flags & IEEE80211_RX_IN_SCAN))
-				return 0;
-			status->rx_flags &= ~IEEE80211_RX_RA_MATCH;
+		} else if (!ieee80211_bssid_match(bssid, sdata->u.ibss.bssid)) {
+			return 0;
 		} else if (!multicast &&
 			   !ether_addr_equal(sdata->vif.addr, hdr->addr1)) {
 			if (!(sdata->dev->flags & IFF_PROMISC))
@@ -2843,11 +2816,9 @@ static int prepare_for_handlers(struct ieee80211_rx_data *rx,
 			 * and location updates. Note that mac80211
 			 * itself never looks at these frames.
 			 */
-			if (!(status->rx_flags & IEEE80211_RX_IN_SCAN) &&
-			    ieee80211_is_public_action(hdr, skb->len))
+			if (ieee80211_is_public_action(hdr, skb->len))
 				return 1;
-			if (!(status->rx_flags & IEEE80211_RX_IN_SCAN) &&
-			    !ieee80211_is_beacon(hdr->frame_control))
+			if (!ieee80211_is_beacon(hdr->frame_control))
 				return 0;
 			status->rx_flags &= ~IEEE80211_RX_RA_MATCH;
 		}
@@ -2940,11 +2911,6 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 	if (ieee80211_is_data(fc) || ieee80211_is_mgmt(fc))
 		local->dot11ReceivedFragmentCount++;
 
-	if (unlikely(test_bit(SCAN_HW_SCANNING, &local->scanning) ||
-		     test_bit(SCAN_ONCHANNEL_SCANNING, &local->scanning) ||
-		     test_bit(SCAN_SW_SCANNING, &local->scanning)))
-		status->rx_flags |= IEEE80211_RX_IN_SCAN;
-
 	if (ieee80211_is_mgmt(fc))
 		err = skb_linearize(skb);
 	else
@@ -2959,6 +2925,10 @@ static void __ieee80211_rx_handle_packet(struct ieee80211_hw *hw,
 	ieee80211_parse_qos(&rx);
 	ieee80211_verify_alignment(&rx);
 
+	if (unlikely(ieee80211_is_probe_resp(hdr->frame_control) ||
+		     ieee80211_is_beacon(hdr->frame_control)))
+		ieee80211_scan_rx(local, skb);
+
 	if (ieee80211_is_data(fc)) {
 		prev_sta = NULL;
 
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index a5848d2..f9d61a4 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -166,52 +166,47 @@ ieee80211_bss_info_update(struct ieee80211_local *local,
 	return bss;
 }
 
-ieee80211_rx_result
-ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb)
+void ieee80211_scan_rx(struct ieee80211_local *local, struct sk_buff *skb)
 {
 	struct ieee80211_rx_status *rx_status = IEEE80211_SKB_RXCB(skb);
-	struct ieee80211_mgmt *mgmt;
+	struct ieee80211_sub_if_data *sdata1, *sdata2;
+	struct ieee80211_mgmt *mgmt = (void *)skb->data;
 	struct ieee80211_bss *bss;
 	u8 *elements;
 	struct ieee80211_channel *channel;
 	size_t baselen;
 	int freq;
-	__le16 fc;
-	bool presp, beacon = false;
+	bool beacon;
 	struct ieee802_11_elems elems;
 
-	if (skb->len < 2)
-		return RX_DROP_UNUSABLE;
-
-	mgmt = (struct ieee80211_mgmt *) skb->data;
-	fc = mgmt->frame_control;
+	if (skb->len < 24 ||
+	    (!ieee80211_is_probe_resp(mgmt->frame_control) &&
+	     !ieee80211_is_beacon(mgmt->frame_control)))
+		return;
 
-	if (ieee80211_is_ctl(fc))
-		return RX_CONTINUE;
+	sdata1 = rcu_dereference(local->scan_sdata);
+	sdata2 = rcu_dereference(local->sched_scan_sdata);
 
-	if (skb->len < 24)
-		return RX_CONTINUE;
+	if (likely(!sdata1 && !sdata2))
+		return;
 
-	presp = ieee80211_is_probe_resp(fc);
-	if (presp) {
+	if (ieee80211_is_probe_resp(mgmt->frame_control)) {
 		/* ignore ProbeResp to foreign address */
-		if (!ether_addr_equal(mgmt->da, sdata->vif.addr))
-			return RX_DROP_MONITOR;
+		if (!ether_addr_equal(mgmt->da, sdata1->vif.addr) &&
+		    !ether_addr_equal(mgmt->da, sdata2->vif.addr))
+			return;
 
-		presp = true;
 		elements = mgmt->u.probe_resp.variable;
 		baselen = offsetof(struct ieee80211_mgmt, u.probe_resp.variable);
+		beacon = false;
 	} else {
-		beacon = ieee80211_is_beacon(fc);
 		baselen = offsetof(struct ieee80211_mgmt, u.beacon.variable);
 		elements = mgmt->u.beacon.variable;
+		beacon = true;
 	}
 
-	if (!presp && !beacon)
-		return RX_CONTINUE;
-
 	if (baselen > skb->len)
-		return RX_DROP_MONITOR;
+		return;
 
 	ieee802_11_parse_elems(elements, skb->len - baselen, &elems);
 
@@ -221,22 +216,16 @@ ieee80211_scan_rx(struct ieee80211_sub_if_data *sdata, struct sk_buff *skb)
 	else
 		freq = rx_status->freq;
 
-	channel = ieee80211_get_channel(sdata->local->hw.wiphy, freq);
+	channel = ieee80211_get_channel(local->hw.wiphy, freq);
 
 	if (!channel || channel->flags & IEEE80211_CHAN_DISABLED)
-		return RX_DROP_MONITOR;
+		return;
 
-	bss = ieee80211_bss_info_update(sdata->local, rx_status,
+	bss = ieee80211_bss_info_update(local, rx_status,
 					mgmt, skb->len, &elems,
 					channel, beacon);
 	if (bss)
-		ieee80211_rx_bss_put(sdata->local, bss);
-
-	if (channel == sdata->local->oper_channel)
-		return RX_CONTINUE;
-
-	dev_kfree_skb(skb);
-	return RX_QUEUED;
+		ieee80211_rx_bss_put(local, bss);
 }
 
 /* return false if no more work */
-- 
1.7.10.4

Re: [RFC 3/3] mac80211: redesign scan RX

[Eliad Peller]

On Sat, Jul 7, 2012 at 12:05 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Scan receive is rather inefficient when there are
> multiple virtual interfaces. We iterate all of the
> virtual interfaces and then notify cfg80211 about
> each beacon many times.
>
> Redesign scan RX to happen before everything else.
> Then we can also get rid of IEEE80211_RX_IN_SCAN
> since we don't have to accept frames into the RX
> handlers for scanning or scheduled scanning any
> more. Overall, this simplifies the code.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
[...]

> +       sdata1 = rcu_dereference(local->scan_sdata);
> +       sdata2 = rcu_dereference(local->sched_scan_sdata);
>
> -       if (skb->len < 24)
> -               return RX_CONTINUE;
> +       if (likely(!sdata1 && !sdata2))
> +               return;
>
> -       presp = ieee80211_is_probe_resp(fc);
> -       if (presp) {
> +       if (ieee80211_is_probe_resp(mgmt->frame_control)) {
>                 /* ignore ProbeResp to foreign address */
> -               if (!ether_addr_equal(mgmt->da, sdata->vif.addr))
> -                       return RX_DROP_MONITOR;
> +               if (!ether_addr_equal(mgmt->da, sdata1->vif.addr) &&
> +                   !ether_addr_equal(mgmt->da, sdata2->vif.addr))
> +                       return;

you should check sdata1 and sdata2 before dereferencing them.

other than that, looks good to me.

Eliad.

Re: [RFC 3/3] mac80211: redesign scan RX

[Johannes Berg]

On Sun, 2012-07-08 at 01:39 +0300, Eliad Peller wrote:

> > +       sdata1 = rcu_dereference(local->scan_sdata);
> > +       sdata2 = rcu_dereference(local->sched_scan_sdata);
> >
> > -       if (skb->len < 24)
> > -               return RX_CONTINUE;
> > +       if (likely(!sdata1 && !sdata2))
> > +               return;
> >
> > -       presp = ieee80211_is_probe_resp(fc);
> > -       if (presp) {
> > +       if (ieee80211_is_probe_resp(mgmt->frame_control)) {
> >                 /* ignore ProbeResp to foreign address */
> > -               if (!ether_addr_equal(mgmt->da, sdata->vif.addr))
> > -                       return RX_DROP_MONITOR;
> > +               if (!ether_addr_equal(mgmt->da, sdata1->vif.addr) &&
> > +                   !ether_addr_equal(mgmt->da, sdata2->vif.addr))
> > +                       return;
> 
> you should check sdata1 and sdata2 before dereferencing them.

Yes, good catch, thanks. It seems I should've crashed it in testing,
I'll make sure I tested the right code ... unless, I think our device
may be filtering probe responses to foreign addresses, and we don't have
sched scan. Yeah, that might do it.

Anyway, I'll fix it.

johannes

Re: [RFC 0/3] mac80211 scanning restructuring

[Ben Greear]

On 07/06/2012 02:05 PM, Johannes Berg wrote:
> I decided that with multi-channel coming and thus us using more
> virtual interfaces, the scanning code was going to be the first
> victim of some factoring ;-)
>
> Please review. The only thing that isn't quite clear to me is
> whether or not I can really remove the channel == oper_channel
> check, but it's only applied to probe resp/beacon frames so it
> seems a bit pointless to try to keep it?

For what it's worth, I don't see any problems with the patches.

Another enhancement I was thinking about would be to allow
vifs to piggy-back on other vif's scans.  Instead of
returning EBUSY when another vif is already scanning, just
register to receive the scanning vif's results when it finishes.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [RFC 0/3] mac80211 scanning restructuring

[Johannes Berg]

On Fri, 2012-07-06 at 14:30 -0700, Ben Greear wrote:
> On 07/06/2012 02:05 PM, Johannes Berg wrote:
> > I decided that with multi-channel coming and thus us using more
> > virtual interfaces, the scanning code was going to be the first
> > victim of some factoring ;-)
> >
> > Please review. The only thing that isn't quite clear to me is
> > whether or not I can really remove the channel == oper_channel
> > check, but it's only applied to probe resp/beacon frames so it
> > seems a bit pointless to try to keep it?
> 
> For what it's worth, I don't see any problems with the patches.

:-)
I think you should see much fewer calls to cfg80211 with this when
beacons are received, when you have many virtual interfaces, but I'm not
sure how you'd see that unless you carefully measure CPU utilization.

> Another enhancement I was thinking about would be to allow
> vifs to piggy-back on other vif's scans.  Instead of
> returning EBUSY when another vif is already scanning, just
> register to receive the scanning vif's results when it finishes.

Hmm, yes, technically that's possible. However, you'd have to verify
that it used exactly the same scan parameters, which seems like a lot of
overhead? Given that we give you the scan parameters in the nl80211
event when the scan finishes (at least I think we do), you could even do
this optimisation in userspace, when -EBUSY is returned?

johannes

Re: [RFC 0/3] mac80211 scanning restructuring

[Ben Greear]

On 07/06/2012 02:35 PM, Johannes Berg wrote:
> On Fri, 2012-07-06 at 14:30 -0700, Ben Greear wrote:
>> On 07/06/2012 02:05 PM, Johannes Berg wrote:
>>> I decided that with multi-channel coming and thus us using more
>>> virtual interfaces, the scanning code was going to be the first
>>> victim of some factoring ;-)
>>>
>>> Please review. The only thing that isn't quite clear to me is
>>> whether or not I can really remove the channel == oper_channel
>>> check, but it's only applied to probe resp/beacon frames so it
>>> seems a bit pointless to try to keep it?
>>
>> For what it's worth, I don't see any problems with the patches.
>
> :-)
> I think you should see much fewer calls to cfg80211 with this when
> beacons are received, when you have many virtual interfaces, but I'm not
> sure how you'd see that unless you carefully measure CPU utilization.
>
>> Another enhancement I was thinking about would be to allow
>> vifs to piggy-back on other vif's scans.  Instead of
>> returning EBUSY when another vif is already scanning, just
>> register to receive the scanning vif's results when it finishes.
>
> Hmm, yes, technically that's possible. However, you'd have to verify
> that it used exactly the same scan parameters, which seems like a lot of
> overhead? Given that we give you the scan parameters in the nl80211
> event when the scan finishes (at least I think we do), you could even do
> this optimisation in userspace, when -EBUSY is returned?

I was thinking to only enable the wait-for-peer-scan logic if
peer and requested scans are 'normal', or some simple subset of
mostly-normal that is easy to check for.  It would still always
be valid to return EBUSY if you cannot obviously share scan results.

Main issue that I see is that if one interface is constantly scanning
in wpa_supplicant because it can't find what it's looking for,
you can often get a failure if you manually run 'iw scan'
on the command line, and that is exactly when you often DO want
to see some manual scan results :)

I've already optimized wpa_supplicant to share scans in user-space
some time back (and code has been upstream for a while), and that is
a big help..but doesn't help non-supplicant programs.

Thanks,
Ben

>
> johannes
>


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com