[PATCH] cfg80211: Fix support for flushing old scan results

[PATCH] cfg80211: Fix support for flushing old scan results

[Tim Kourt]

__cfg80211_bss_expire function was incorrectly used to flush the BSS
entries from the previous scan results, causing NL80211_SCAN_FLAG_FLUSH
flag to have no effect.

This patch is addressing the described issue by changing the semantics
of the function (__cfg80211_bss_expire) parameter from a confusing
expire_time (jiffies - IEEE80211_SCAN_RESULT_EXPIRE) to a simple
time_to_live interval and encapsulating the needed calculations inside
of the function. The rest of the function usages were changed accordingly.

Note: This patch enables flushing of the non-hidden BSSs.

Signed-off-by: Tim Kourt <tim.a.kourt@linux.intel.com>
---
 net/wireless/scan.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index d36c3eb..d459457 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -71,7 +71,7 @@ module_param(bss_entries_limit, int, 0644);
 MODULE_PARM_DESC(bss_entries_limit,
                  "limit to number of scan BSS entries (per wiphy, default 1000)");
 
-#define IEEE80211_SCAN_RESULT_EXPIRE	(30 * HZ)
+#define IEEE80211_SCAN_RESULT_TIME_TO_LIVE (30 * HZ)
 
 static void bss_free(struct cfg80211_internal_bss *bss)
 {
@@ -160,7 +160,7 @@ static bool __cfg80211_unlink_bss(struct cfg80211_registered_device *rdev,
 }
 
 static void __cfg80211_bss_expire(struct cfg80211_registered_device *rdev,
-				  unsigned long expire_time)
+				  unsigned long time_to_live)
 {
 	struct cfg80211_internal_bss *bss, *tmp;
 	bool expired = false;
@@ -170,7 +170,8 @@ static void __cfg80211_bss_expire(struct cfg80211_registered_device *rdev,
 	list_for_each_entry_safe(bss, tmp, &rdev->bss_list, list) {
 		if (atomic_read(&bss->hold))
 			continue;
-		if (!time_after(expire_time, bss->ts))
+
+		if (!time_after(jiffies, bss->ts + time_to_live))
 			continue;
 
 		if (__cfg80211_unlink_bss(rdev, bss))
@@ -181,6 +182,11 @@ static void __cfg80211_bss_expire(struct cfg80211_registered_device *rdev,
 		rdev->bss_generation++;
 }
 
+static void __cfg80211_bss_expire_all(struct cfg80211_registered_device *rdev)
+{
+	__cfg80211_bss_expire(rdev, 0);
+}
+
 static bool cfg80211_bss_expire_oldest(struct cfg80211_registered_device *rdev)
 {
 	struct cfg80211_internal_bss *bss, *oldest = NULL;
@@ -251,7 +257,7 @@ void ___cfg80211_scan_done(struct cfg80211_registered_device *rdev,
 	    request->flags & NL80211_SCAN_FLAG_FLUSH) {
 		/* flush entries from previous scans */
 		spin_lock_bh(&rdev->bss_lock);
-		__cfg80211_bss_expire(rdev, request->scan_start);
+		__cfg80211_bss_expire_all(rdev);
 		spin_unlock_bh(&rdev->bss_lock);
 	}
 
@@ -380,7 +386,7 @@ void cfg80211_sched_scan_results_wk(struct work_struct *work)
 			if (req->flags & NL80211_SCAN_FLAG_FLUSH) {
 				/* flush entries from previous scans */
 				spin_lock_bh(&rdev->bss_lock);
-				__cfg80211_bss_expire(rdev, req->scan_start);
+				__cfg80211_bss_expire_all(rdev);
 				spin_unlock_bh(&rdev->bss_lock);
 				req->scan_start = jiffies;
 			}
@@ -477,7 +483,7 @@ void cfg80211_bss_age(struct cfg80211_registered_device *rdev,
 
 void cfg80211_bss_expire(struct cfg80211_registered_device *rdev)
 {
-	__cfg80211_bss_expire(rdev, jiffies - IEEE80211_SCAN_RESULT_EXPIRE);
+	__cfg80211_bss_expire(rdev, IEEE80211_SCAN_RESULT_TIME_TO_LIVE);
 }
 
 const u8 *cfg80211_find_ie_match(u8 eid, const u8 *ies, int len,
@@ -738,7 +744,8 @@ struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy,
 		if (!is_valid_ether_addr(bss->pub.bssid))
 			continue;
 		/* Don't get expired BSS structs */
-		if (time_after(now, bss->ts + IEEE80211_SCAN_RESULT_EXPIRE) &&
+		if (time_after(now, bss->ts +
+					IEEE80211_SCAN_RESULT_TIME_TO_LIVE) &&
 		    !atomic_read(&bss->hold))
 			continue;
 		if (is_bss(&bss->pub, bssid, ssid, ssid_len)) {
-- 
2.9.4

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

On Fri, 2018-05-11 at 09:48 -0700, Tim Kourt wrote:
> __cfg80211_bss_expire function was incorrectly used to flush the BSS
> entries from the previous scan results, causing NL80211_SCAN_FLAG_FLUSH
> flag to have no effect.

Hmm. I guess I'm not convinced - what's the bug?

We flush anything that's older than our start, so that should work just
fine?

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Johannes,

On 05/18/2018 03:13 AM, Johannes Berg wrote:
> On Fri, 2018-05-11 at 09:48 -0700, Tim Kourt wrote:
>> __cfg80211_bss_expire function was incorrectly used to flush the BSS
>> entries from the previous scan results, causing NL80211_SCAN_FLAG_FLUSH
>> flag to have no effect.
> 
> Hmm. I guess I'm not convinced - what's the bug?
> 
> We flush anything that's older than our start, so that should work just
> fine?
> 

Just FYI, there's definitely something funny with the scanning code:

denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush
BSS 10:c3:7b:54:74:d4(on wlp2s0)
	last seen: 274.815s [boottime]
	freq: 5765
	beacon interval: 100 TUs
	signal: -35.00 dBm
	last seen: 349 ms ago
	Information elements from Probe Response frame:
	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00


Then if I try:
denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush ssid myssid
BSS 10:c3:7b:54:74:d4(on wlp2s0)
	last seen: 319.667s [boottime]
	freq: 5765
	beacon interval: 100 TUs
	signal: -42.00 dBm
	last seen: 350 ms ago
	Information elements from Probe Response frame:
	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
....
BSS 10:c3:7b:54:74:d4(on wlp2s0)
	last seen: 319.662s [boottime]
	freq: 5765
	beacon interval: 100 TUs
	signal: -37.00 dBm
	last seen: 355 ms ago
	Information elements from Probe Response frame:
	SSID: myssid

Shouldn't the second scan give a single result from that one BSS?

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Arend van Spriel]

On 5/18/2018 6:47 PM, Denis Kenzior wrote:
> Hi Johannes,
>
> On 05/18/2018 03:13 AM, Johannes Berg wrote:
>> On Fri, 2018-05-11 at 09:48 -0700, Tim Kourt wrote:
>>> __cfg80211_bss_expire function was incorrectly used to flush the BSS
>>> entries from the previous scan results, causing NL80211_SCAN_FLAG_FLUSH
>>> flag to have no effect.
>>
>> Hmm. I guess I'm not convinced - what's the bug?
>>
>> We flush anything that's older than our start, so that should work just
>> fine?
>>
>
> Just FYI, there's definitely something funny with the scanning code:
>
> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush
> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>      last seen: 274.815s [boottime]
>      freq: 5765
>      beacon interval: 100 TUs
>      signal: -35.00 dBm
>      last seen: 349 ms ago
>      Information elements from Probe Response frame:
>      SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
>
>
> Then if I try:
> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush ssid myssid
> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>      last seen: 319.667s [boottime]
>      freq: 5765
>      beacon interval: 100 TUs
>      signal: -42.00 dBm
>      last seen: 350 ms ago
>      Information elements from Probe Response frame:
>      SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
> ....
> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>      last seen: 319.662s [boottime]
>      freq: 5765
>      beacon interval: 100 TUs
>      signal: -37.00 dBm
>      last seen: 355 ms ago
>      Information elements from Probe Response frame:
>      SSID: myssid
>
> Shouldn't the second scan give a single result from that one BSS?

Looking at the 'last seen' values it does look ok. Both results have the 
same BSSID, but the first one shows the broadcast ssid (or so it seems). 
Neither iw nor nl80211 on the kernel side add the broadcast ssid. So 
question is what device are you using and does it use mac80211 software 
scanning or hardware scanning. I did not dive into mac80211 to see if 
the broadcast ssid is added there.

Regards,
Arend

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Arend,

On 05/18/2018 01:54 PM, Arend van Spriel wrote:
> On 5/18/2018 6:47 PM, Denis Kenzior wrote:
>> Hi Johannes,
>>
>> On 05/18/2018 03:13 AM, Johannes Berg wrote:
>>> On Fri, 2018-05-11 at 09:48 -0700, Tim Kourt wrote:
>>>> __cfg80211_bss_expire function was incorrectly used to flush the BSS
>>>> entries from the previous scan results, causing NL80211_SCAN_FLAG_FLUSH
>>>> flag to have no effect.
>>>
>>> Hmm. I guess I'm not convinced - what's the bug?
>>>
>>> We flush anything that's older than our start, so that should work just
>>> fine?
>>>
>>
>> Just FYI, there's definitely something funny with the scanning code:
>>
>> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush
>> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>>      last seen: 274.815s [boottime]
>>      freq: 5765
>>      beacon interval: 100 TUs
>>      signal: -35.00 dBm
>>      last seen: 349 ms ago
>>      Information elements from Probe Response frame:
>>      SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
>>
>>
>> Then if I try:
>> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush ssid myssid
>> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>>      last seen: 319.667s [boottime]
>>      freq: 5765
>>      beacon interval: 100 TUs
>>      signal: -42.00 dBm
>>      last seen: 350 ms ago
>>      Information elements from Probe Response frame:
>>      SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
>> ....
>> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>>      last seen: 319.662s [boottime]
>>      freq: 5765
>>      beacon interval: 100 TUs
>>      signal: -37.00 dBm
>>      last seen: 355 ms ago
>>      Information elements from Probe Response frame:
>>      SSID: myssid
>>
>> Shouldn't the second scan give a single result from that one BSS?
> 
> Looking at the 'last seen' values it does look ok. Both results have the 
> same BSSID, but the first one shows the broadcast ssid (or so it seems). 

Are you saying the first result is from the Beacon and the other is from 
the Probe Response?  Then why are the 'Information elements from Probe 
Response frame' the way they are?

> Neither iw nor nl80211 on the kernel side add the broadcast ssid. So 
> question is what device are you using and does it use mac80211 software 

Intel 7260.  We're seeing the same results with hwsim as well though. 
This was just a quick test to illustrate.

> scanning or hardware scanning. I did not dive into mac80211 to see if 
> the broadcast ssid is added there.

By the way, if you're interested.  The same tests with a Broadcom based 
device wouldn't even find the hidden network.  It would always come back 
with a single 'x00' SSID regardless of whether I added 'ssid myssid' at 
the end.

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Arend van Spriel]

On 5/18/2018 9:00 PM, Denis Kenzior wrote:
> Hi Arend,
>
> On 05/18/2018 01:54 PM, Arend van Spriel wrote:
>> On 5/18/2018 6:47 PM, Denis Kenzior wrote:
>>> Hi Johannes,
>>>
>>> On 05/18/2018 03:13 AM, Johannes Berg wrote:
>>>> On Fri, 2018-05-11 at 09:48 -0700, Tim Kourt wrote:
>>>>> __cfg80211_bss_expire function was incorrectly used to flush the BSS
>>>>> entries from the previous scan results, causing
>>>>> NL80211_SCAN_FLAG_FLUSH
>>>>> flag to have no effect.
>>>>
>>>> Hmm. I guess I'm not convinced - what's the bug?
>>>>
>>>> We flush anything that's older than our start, so that should work just
>>>> fine?
>>>>
>>>
>>> Just FYI, there's definitely something funny with the scanning code:
>>>
>>> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush
>>> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>>>      last seen: 274.815s [boottime]
>>>      freq: 5765
>>>      beacon interval: 100 TUs
>>>      signal: -35.00 dBm
>>>      last seen: 349 ms ago
>>>      Information elements from Probe Response frame:
>>>      SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
>>>
>>>
>>> Then if I try:
>>> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush ssid myssid
>>> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>>>      last seen: 319.667s [boottime]
>>>      freq: 5765
>>>      beacon interval: 100 TUs
>>>      signal: -42.00 dBm
>>>      last seen: 350 ms ago
>>>      Information elements from Probe Response frame:
>>>      SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
>>> ....
>>> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>>>      last seen: 319.662s [boottime]
>>>      freq: 5765
>>>      beacon interval: 100 TUs
>>>      signal: -37.00 dBm
>>>      last seen: 355 ms ago
>>>      Information elements from Probe Response frame:
>>>      SSID: myssid
>>>
>>> Shouldn't the second scan give a single result from that one BSS?
>>
>> Looking at the 'last seen' values it does look ok. Both results have
>> the same BSSID, but the first one shows the broadcast ssid (or so it
>> seems).
>
> Are you saying the first result is from the Beacon and the other is from
> the Probe Response?  Then why are the 'Information elements from Probe
> Response frame' the way they are?

Nope. I am not saying that. I am saying that there are two probe 
requests being sent. One with broadcast ssid, ie. ssid_len == 0, and 
with ssid 'myssid'. But it is speculation without a sniffer capture.

>> Neither iw nor nl80211 on the kernel side add the broadcast ssid. So
>> question is what device are you using and does it use mac80211 software
>
> Intel 7260.  We're seeing the same results with hwsim as well though.
> This was just a quick test to illustrate.

That seems to point to mac80211 although I am not very familiar with 
neither mac80211_hwsim nor iwlwifi.

>> scanning or hardware scanning. I did not dive into mac80211 to see if
>> the broadcast ssid is added there.
>
> By the way, if you're interested.  The same tests with a Broadcom based
> device wouldn't even find the hidden network.  It would always come back
> with a single 'x00' SSID regardless of whether I added 'ssid myssid' at
> the end.

Interesting. So that means firmware does not honor the ssids passed or 
brcmfmac does something wrong. Need to look into that.

Thanks,
Arend

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Arend,

>> Are you saying the first result is from the Beacon and the other is from
>> the Probe Response?  Then why are the 'Information elements from Probe
>> Response frame' the way they are?
> 
> Nope. I am not saying that. I am saying that there are two probe 
> requests being sent. One with broadcast ssid, ie. ssid_len == 0, and 
> with ssid 'myssid'. But it is speculation without a sniffer capture.

Ah I see what you mean now.  No, we traced this down to hostapd itself 
and it was receiving a single Probe Request with the ssid set and 
replying to it per spec.  So I'm pretty confident this scenario isn't 
what is happening.  Let me try to get some actual packet captures...

Regards
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

On Tue, 2018-05-22 at 09:48 -0500, Denis Kenzior wrote:
> Hi Arend,
> 
> > > Are you saying the first result is from the Beacon and the other is from
> > > the Probe Response?  Then why are the 'Information elements from Probe
> > > Response frame' the way they are?
> > 
> > Nope. I am not saying that. I am saying that there are two probe 
> > requests being sent. One with broadcast ssid, ie. ssid_len == 0, and 
> > with ssid 'myssid'. But it is speculation without a sniffer capture.
> 
> Ah I see what you mean now.  No, we traced this down to hostapd itself 
> and it was receiving a single Probe Request with the ssid set and 
> replying to it per spec.  So I'm pretty confident this scenario isn't 
> what is happening.  Let me try to get some actual packet captures...

Was "myssid" the real SSID, or did you hide that from us and it was
really 9 characters long in the original?

If it was really 9 characters long I could imagine that there's a
different bug with a beacon with all-zero-bytes having been received
(and getting stuck into the probe response buffer for some reason), and
then you *should* see both entries.

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

On Tue, 2018-05-22 at 16:50 +0200, Johannes Berg wrote:
> On Tue, 2018-05-22 at 09:48 -0500, Denis Kenzior wrote:
> > Hi Arend,
> > 
> > > > Are you saying the first result is from the Beacon and the other is from
> > > > the Probe Response?  Then why are the 'Information elements from Probe
> > > > Response frame' the way they are?
> > > 
> > > Nope. I am not saying that. I am saying that there are two probe 
> > > requests being sent. One with broadcast ssid, ie. ssid_len == 0, and 
> > > with ssid 'myssid'. But it is speculation without a sniffer capture.
> > 
> > Ah I see what you mean now.  No, we traced this down to hostapd itself 
> > and it was receiving a single Probe Request with the ssid set and 
> > replying to it per spec.  So I'm pretty confident this scenario isn't 
> > what is happening.  Let me try to get some actual packet captures...
> 
> Was "myssid" the real SSID, or did you hide that from us and it was
> really 9 characters long in the original?
> 
> If it was really 9 characters long I could imagine that there's a
> different bug with a beacon with all-zero-bytes having been received
> (and getting stuck into the probe response buffer for some reason), and
> then you *should* see both entries.

Or perhaps there's a bug with how we link the results between
hidden/non-hidden, but it seems to me that hostapd would never have
responded with a probe response with zeroed bytes.

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Johannes,

On 05/22/2018 09:51 AM, Johannes Berg wrote:
> On Tue, 2018-05-22 at 16:50 +0200, Johannes Berg wrote:
>> On Tue, 2018-05-22 at 09:48 -0500, Denis Kenzior wrote:
>>> Hi Arend,
>>>
>>>>> Are you saying the first result is from the Beacon and the other is from
>>>>> the Probe Response?  Then why are the 'Information elements from Probe
>>>>> Response frame' the way they are?
>>>>
>>>> Nope. I am not saying that. I am saying that there are two probe
>>>> requests being sent. One with broadcast ssid, ie. ssid_len == 0, and
>>>> with ssid 'myssid'. But it is speculation without a sniffer capture.
>>>
>>> Ah I see what you mean now.  No, we traced this down to hostapd itself
>>> and it was receiving a single Probe Request with the ssid set and
>>> replying to it per spec.  So I'm pretty confident this scenario isn't
>>> what is happening.  Let me try to get some actual packet captures...
>>
>> Was "myssid" the real SSID, or did you hide that from us and it was
>> really 9 characters long in the original?

See the other sub-thread.  It was 9 characters long, I hand-edited it to 
protect the innocent.

>>
>> If it was really 9 characters long I could imagine that there's a
>> different bug with a beacon with all-zero-bytes having been received
>> (and getting stuck into the probe response buffer for some reason), and
>> then you *should* see both entries.
> 
> Or perhaps there's a bug with how we link the results between
> hidden/non-hidden, but it seems to me that hostapd would never have
> responded with a probe response with zeroed bytes.
> 

We suspect it is likely a bug in cfg80211_bss_update logic somewhere.

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

Hi Denis,

> Just FYI, there's definitely something funny with the scanning code:
> 
> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush
> BSS 10:c3:7b:54:74:d4(on wlp2s0)
> 	last seen: 274.815s [boottime]
> 	freq: 5765
> 	beacon interval: 100 TUs
> 	signal: -35.00 dBm
> 	last seen: 349 ms ago
> 	Information elements from Probe Response frame:
> 	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00

This is already rather strange to start with. Can you provide a sniffer
capture of this situation?

Thing is - the all-zero-bytes there points to using hidden SSID with a
length of 9 characters, BUT
 * "myssid" is just 6 characters long - or did you edit that?
 * normally the zeroed-out SSID isn't transmitted in a *probe response*
   but only in beacons

> Then if I try:
> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush ssid myssid
> BSS 10:c3:7b:54:74:d4(on wlp2s0)
> 	last seen: 319.667s [boottime]
> 	freq: 5765
> 	beacon interval: 100 TUs
> 	signal: -42.00 dBm
> 	last seen: 350 ms ago
> 	Information elements from Probe Response frame:
> 	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
> ....
> BSS 10:c3:7b:54:74:d4(on wlp2s0)
> 	last seen: 319.662s [boottime]
> 	freq: 5765
> 	beacon interval: 100 TUs
> 	signal: -37.00 dBm
> 	last seen: 355 ms ago
> 	Information elements from Probe Response frame:
> 	SSID: myssid
> 
> Shouldn't the second scan give a single result from that one BSS?

Well, depends what the AP sends you really. The "last seen" doesn't
indicate that it's not flushed but rather that you got it again, so
without seeing the sniffer I don't think we can really say what's going
on.

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Johannes,

On 05/22/2018 03:12 AM, Johannes Berg wrote:
> Hi Denis,
> 
>> Just FYI, there's definitely something funny with the scanning code:
>>
>> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush
>> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>> 	last seen: 274.815s [boottime]
>> 	freq: 5765
>> 	beacon interval: 100 TUs
>> 	signal: -35.00 dBm
>> 	last seen: 349 ms ago
>> 	Information elements from Probe Response frame:
>> 	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
> 
> This is already rather strange to start with. Can you provide a sniffer
> capture of this situation?

Will do

> 
> Thing is - the all-zero-bytes there points to using hidden SSID with a
> length of 9 characters, BUT
>   * "myssid" is just 6 characters long - or did you edit that?

Good eyes!  Yes this was edited to protect the innocent neighbors ;)

>   * normally the zeroed-out SSID isn't transmitted in a *probe response*
>     but only in beacons

Exactly.  That's what makes this really weird.

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

Hi Denis,

Thanks for the capture file (for everyone else - Denis provided this to
Arend and myself privately).

In it, we see that there are only ever beacons with zeroed out SSID, and
probe responses with correct SSID. Nothing weird mixed.

> denkenz@iwd-test ~ $ sudo iw dev wlp2s0 scan flush
> BSS 10:c3:7b:54:74:d4(on wlp2s0)
> 	last seen: 274.815s [boottime]
> 	freq: 5765
> 	beacon interval: 100 TUs
> 	signal: -35.00 dBm
> 	last seen: 349 ms ago
> 	Information elements from Probe Response frame:
> 	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00


I think I finally figured out what's going on. It's a mix between
strange 'iw' behaviour, and strange backward-compatibility behaviour in
cfg80211.

If you do this again and give the scan dump command explicitly with -b
added, like

sudo iw dev wlp2s0 scan passive
iw dev wlp2s0 scan dump -b

then you'll likely see

BSS 10:c3:7b:54:74:d4(on wlp2s0)
 	last seen: 274.815s [boottime]
 	freq: 5765
 	beacon interval: 100 TUs
 	signal: -35.00 dBm
 	last seen: 349 ms ago
 	Information elements from Probe Response frame:
 	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
	[... ht/vht ...]
 	Information elements from Beacon frame:
 	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
	[... ht/vht ...]

So there are two things going on:

1) In iw, we limit the amount of information printed, but in that case
still print "Information elements from Probe Response frame", if beacon
IEs are also present in the BSS information.

2) Originally, the kernel didn't distinguish between probe response and
beacon IE attributes, but only had "IEs" in general. When we later did
want to distinguish, we changed nl80211 to unconditionally put some IEs
into the "IEs", if received from beacon put them into "beacon IEs", but
to avoid further duplication didn't introduce a separate "probe response
IEs" attribute. You can see this in nl80211_send_bss():

        /* this pointer prefers to be pointed to probe response data
         * but is always valid
         */
        ies = rcu_dereference(res->ies);

        if (ies) {
                if (nla_put_u64_64bit(msg, NL80211_BSS_TSF, ies->tsf,
                                      NL80211_BSS_PAD))
                        goto fail_unlock_rcu;
                if (ies->len && nla_put(msg, NL80211_BSS_INFORMATION_ELEMENTS,
                                        ies->len, ies->data))
                        goto fail_unlock_rcu;
        }



        /* and this pointer is always (unless driver didn't know) beacon data */
        ies = rcu_dereference(res->beacon_ies);
        if (ies && ies->from_beacon) {
                if (nla_put_u64_64bit(msg, NL80211_BSS_BEACON_TSF, ies->tsf,
                                      NL80211_BSS_PAD))
                        goto fail_unlock_rcu;
                if (ies->len && nla_put(msg, NL80211_BSS_BEACON_IES,
                                        ies->len, ies->data))
                        goto fail_unlock_rcu;
        }

Ultimately, this is also lossy - so later I added NL80211_BSS_PRESP_DATA
that indicates "these were *really* received by probe response.

I guess I should change iw in the following way:

https://p.sipsolutions.net/6958d16d00f955c7.txt

which will mean it will not print "probe response IEs" in a false
positive way.

Obviously on kernels that don't set NL80211_BSS_PRESP_DATA it won't
print it out even if the beacon/probe response were both received with
different content, but it can't know if the kernel doesn't tell it.

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Johannes,
> 
> 
> I think I finally figured out what's going on. It's a mix between
> strange 'iw' behaviour, and strange backward-compatibility behaviour in
> cfg80211.
> 
> If you do this again and give the scan dump command explicitly with -b
> added, like
> 
> sudo iw dev wlp2s0 scan passive
> iw dev wlp2s0 scan dump -b
> 
> then you'll likely see
> 
> BSS 10:c3:7b:54:74:d4(on wlp2s0)
>   	last seen: 274.815s [boottime]
>   	freq: 5765
>   	beacon interval: 100 TUs
>   	signal: -35.00 dBm
>   	last seen: 349 ms ago
>   	Information elements from Probe Response frame:
>   	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
> 	[... ht/vht ...]
>   	Information elements from Beacon frame:
>   	SSID: \x00\x00\x00\x00\x00\x00\x00\x00\x00
> 	[... ht/vht ...]
> 
> So there are two things going on:
> 
> 1) In iw, we limit the amount of information printed, but in that case
> still print "Information elements from Probe Response frame", if beacon
> IEs are also present in the BSS information.
> 
> 2) Originally, the kernel didn't distinguish between probe response and
> beacon IE attributes, but only had "IEs" in general. When we later did
> want to distinguish, we changed nl80211 to unconditionally put some IEs
> into the "IEs", if received from beacon put them into "beacon IEs", but
> to avoid further duplication didn't introduce a separate "probe response
> IEs" attribute. You can see this in nl80211_send_bss():
> 
>          /* this pointer prefers to be pointed to probe response data
>           * but is always valid
>           */
>          ies = rcu_dereference(res->ies);
> 
>          if (ies) {
>                  if (nla_put_u64_64bit(msg, NL80211_BSS_TSF, ies->tsf,
>                                        NL80211_BSS_PAD))
>                          goto fail_unlock_rcu;
>                  if (ies->len && nla_put(msg, NL80211_BSS_INFORMATION_ELEMENTS,
>                                          ies->len, ies->data))
>                          goto fail_unlock_rcu;
>          }
> 
> 
> 
>          /* and this pointer is always (unless driver didn't know) beacon data */
>          ies = rcu_dereference(res->beacon_ies);
>          if (ies && ies->from_beacon) {
>                  if (nla_put_u64_64bit(msg, NL80211_BSS_BEACON_TSF, ies->tsf,
>                                        NL80211_BSS_PAD))
>                          goto fail_unlock_rcu;
>                  if (ies->len && nla_put(msg, NL80211_BSS_BEACON_IES,
>                                          ies->len, ies->data))
>                          goto fail_unlock_rcu;
>          }
> 
> Ultimately, this is also lossy - so later I added NL80211_BSS_PRESP_DATA
> that indicates "these were *really* received by probe response.
> 
> I guess I should change iw in the following way:
> 
> https://p.sipsolutions.net/6958d16d00f955c7.txt
> 
> which will mean it will not print "probe response IEs" in a false
> positive way.
> 
> Obviously on kernels that don't set NL80211_BSS_PRESP_DATA it won't
> print it out even if the beacon/probe response were both received with
> different content, but it can't know if the kernel doesn't tell it.

So I need to absorb all of this some more, but I'm still wondering why 
we are seeing two separate scan entries (with hidden & plain ssid) when 
we requested a flush?  Is there a way to force the kernel to only show 
us the probe responses.

It would seem that even with the flush flag set, there could still be 
spurious beacons getting in causing these results to appear in the scan 
result set, right?

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

Hi,

> So I need to absorb all of this some more, but I'm still wondering why 
> we are seeing two separate scan entries (with hidden & plain ssid) when 
> we requested a flush?  Is there a way to force the kernel to only show 
> us the probe responses.

Oh. I didn't even think about this part, but that's just a consequence
of having a hidden SSID. We need one entry to track the beacon, and then
we add another entry for each hidden SSID it may have. Some APs
implement multiple SSIDs that way (old Cisco equipment, IIRC, though
then with zero-length SSID instead of zero-bytes), so you can't lump
them together into a single BSS entry.

> It would seem that even with the flush flag set, there could still be 
> spurious beacons getting in causing these results to appear in the scan 
> result set, right?

In general anything can, there's no filtering. So you might do a flush
scan for a specific SSID and still get 20 (different) results, if 20
beacons happened to be received while you were scanning.

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Johannes,

On 05/22/2018 03:40 PM, Johannes Berg wrote:
> Hi,
> 
>> So I need to absorb all of this some more, but I'm still wondering why
>> we are seeing two separate scan entries (with hidden & plain ssid) when
>> we requested a flush?  Is there a way to force the kernel to only show
>> us the probe responses.
> 
> Oh. I didn't even think about this part, but that's just a consequence
> of having a hidden SSID. We need one entry to track the beacon, and then
> we add another entry for each hidden SSID it may have. Some APs
> implement multiple SSIDs that way (old Cisco equipment, IIRC, though
> then with zero-length SSID instead of zero-bytes), so you can't lump
> them together into a single BSS entry.
> 
>> It would seem that even with the flush flag set, there could still be
>> spurious beacons getting in causing these results to appear in the scan
>> result set, right?
> 
> In general anything can, there's no filtering. So you might do a flush
> scan for a specific SSID and still get 20 (different) results, if 20
> beacons happened to be received while you were scanning.
> 

Okay, so we need to use NL80211_BSS_PRESP_DATA if we want to filter out 
scan results that are coming from beacons, right?

So what's the practical use of the flush flag?  Or is that something 
that was meant to be 'for-testing-only'?

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

On Tue, 2018-05-22 at 15:49 -0500, Denis Kenzior wrote:

> Okay, so we need to use NL80211_BSS_PRESP_DATA if we want to filter out 
> scan results that are coming from beacons, right?

You could do that, yes. In non-hidden cases you get the beacon/probe
response data combined, in hidden cases you may get the beacon data
separately and the (second, third, ...) entry with probe response data
will come with beacon data too.

I'm not really sure what value that would have though. In general, you
might want to not display hidden SSIDs as such or at all in the UI.

> So what's the practical use of the flush flag?  Or is that something 
> that was meant to be 'for-testing-only'?

I think you misunderstand? The value is that it ensures that nothing is
present in the list that was received *before* the scan.

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Johannes,

> On May 22, 2018, at 3:52 PM, Johannes Berg <johannes@sipsolutions.net> =
wrote:
>=20
> On Tue, 2018-05-22 at 15:49 -0500, Denis Kenzior wrote:
>=20
>> Okay, so we need to use NL80211_BSS_PRESP_DATA if we want to filter =
out=20
>> scan results that are coming from beacons, right?
>=20
> You could do that, yes. In non-hidden cases you get the beacon/probe
> response data combined, in hidden cases you may get the beacon data
> separately and the (second, third, ...) entry with probe response data
> will come with beacon data too.
>=20
> I'm not really sure what value that would have though. In general, you
> might want to not display hidden SSIDs as such or at all in the UI.

Right.  The intent here is to handle auto-connect to hidden networks.  =
So if we have provisioned a network that is hidden, and we see hidden =
networks via beacons, we want to scan for our provisioned hidden SSIDs =
and only get the probe response results.

>=20
>> So what's the practical use of the flush flag?  Or is that something=20=

>> that was meant to be 'for-testing-only'?
>=20
> I think you misunderstand? The value is that it ensures that nothing =
is
> present in the list that was received *before* the scan.
>=20

We misunderstood how it was supposed to work, but no, I get how things =
work now.  I=E2=80=99m just curious what other potential uses this flag =
might have.

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

On Tue, 2018-05-22 at 16:00 -0500, Denis Kenzior wrote:

> > > So what's the practical use of the flush flag?  Or is that something 
> > > that was meant to be 'for-testing-only'?
> > 
> > I think you misunderstand? The value is that it ensures that nothing is
> > present in the list that was received *before* the scan.
> > 
> 
> We misunderstood how it was supposed to work, but no, I get how things
> work now. 

FWIW, even what I said above was slightly lying I think (though I'd have
to go check the code). That's actually what I thought had happened
before, though the timestamps should've allowed you to realize this, and
it wasn't the case here afaict.

But in theory, I think you could've received the beacon with hidden SSID
*before* the scan, yet it might be present in the scan results if the
new scan caused the probe response to be associated with that scan.

> I’m just curious what other potential uses this flag might have.

Well, basically, ensure that your scan data is up-to-date?

I think mostly it's because there are scenarios where the AP is expected
to vary the probe response data, so you need to know if you
 * have a new probe response, or
 * didn't receive a new probe response, or
 * the AP erroneously didn't change the new probe response.

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

Hi Johannes,

> But in theory, I think you could've received the beacon with hidden SSID
> *before* the scan, yet it might be present in the scan results if the
> new scan caused the probe response to be associated with that scan.

Right, your explanation was helpful, thanks.  It still seems completely 
weird and redundant that we get two separate entries though.  The second 
entry with the probe response data still carries the beacon info (as you 
point out).  Should the pure-beacon one be filtered?

> 
>> I’m just curious what other potential uses this flag might have.
> 
> Well, basically, ensure that your scan data is up-to-date?
> 
> I think mostly it's because there are scenarios where the AP is expected
> to vary the probe response data, so you need to know if you
>   * have a new probe response, or
>   * didn't receive a new probe response, or
>   * the AP erroneously didn't change the new probe response.

Right, so thinking out loud here.  Would it be useful to tell GET SCAN 
to only return entries with actual probe response data?  Combined with 
the flush flag it seems like a much better fit for the cases you point out.

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

On Tue, 2018-05-22 at 16:25 -0500, Denis Kenzior wrote:
> Hi Johannes,
> 
> > But in theory, I think you could've received the beacon with hidden SSID
> > *before* the scan, yet it might be present in the scan results if the
> > new scan caused the probe response to be associated with that scan.
> 
> Right, your explanation was helpful, thanks.  It still seems completely 
> weird and redundant that we get two separate entries though.  The second 
> entry with the probe response data still carries the beacon info (as you 
> point out).  Should the pure-beacon one be filtered?

I'm not sure. It still indicates that a hidden SSID was found, and in
general even a real SSID on the same BSSID doesn't indicate that this
was the only hidden SSID ...

> Right, so thinking out loud here.  Would it be useful to tell GET SCAN 
> to only return entries with actual probe response data?  Combined with 
> the flush flag it seems like a much better fit for the cases you point out.

I don't really see much point in doing filtering in the kernel. It
wouldn't doesn't hurt, but just trades off more kernel code for less
transferred data - and that's mostly in this particular corner case, so
not really an efficiency problem?

And if it wasn't a hidden SSID, then you probably do want to know about
the non-hidden SSIDs that you picked up along the way. In fact, this
will become crucial with OCE, since that results in cases where you
don't even send a probe request if you've picked up certain things
during the scan passively.

johannes

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Denis Kenzior]

On 05/22/2018 04:28 PM, Johannes Berg wrote:
> On Tue, 2018-05-22 at 16:25 -0500, Denis Kenzior wrote:
>> Hi Johannes,
>>
>>> But in theory, I think you could've received the beacon with hidden SSID
>>> *before* the scan, yet it might be present in the scan results if the
>>> new scan caused the probe response to be associated with that scan.
>>
>> Right, your explanation was helpful, thanks.  It still seems completely
>> weird and redundant that we get two separate entries though.  The second
>> entry with the probe response data still carries the beacon info (as you
>> point out).  Should the pure-beacon one be filtered?
> 
> I'm not sure. It still indicates that a hidden SSID was found, and in
> general even a real SSID on the same BSSID doesn't indicate that this
> was the only hidden SSID ...

Right, but you still get that info conveyed through the Beacon IEs 
elements on the second/third/etc entry.  So it still seems redundant to 
include the pure beacon one?

Also, given that you have to ask for the SSID you want specifically, 
what practical purpose does it serve to know that this wasn't the only 
hidden SSID?  I mean you can see that hidden SSIDs are out there, run an 
active scan for the ones you can use.  If none are there, you can just 
ignore that bssid...

> 
>> Right, so thinking out loud here.  Would it be useful to tell GET SCAN
>> to only return entries with actual probe response data?  Combined with
>> the flush flag it seems like a much better fit for the cases you point out.
> 
> I don't really see much point in doing filtering in the kernel. It
> wouldn't doesn't hurt, but just trades off more kernel code for less
> transferred data - and that's mostly in this particular corner case, so
> not really an efficiency problem?

Fair enough.  It was more motivated by 'make the API a bit more readable 
/ accessible / user friendly'.

> 
> And if it wasn't a hidden SSID, then you probably do want to know about
> the non-hidden SSIDs that you picked up along the way. In fact, this
> will become crucial with OCE, since that results in cases where you
> don't even send a probe request if you've picked up certain things
> during the scan passively.

Right.  In our case we only scan passively unless we detect a hidden 
ssid and have provisioned hidden ssids.  Then we issue an active scan 
for just those...

Regards,
-Denis

Re: [PATCH] cfg80211: Fix support for flushing old scan results

[Johannes Berg]

Hi Denis,

> > I'm not sure. It still indicates that a hidden SSID was found, and in
> > general even a real SSID on the same BSSID doesn't indicate that this
> > was the only hidden SSID ...
> 
> Right, but you still get that info conveyed through the Beacon IEs 
> elements on the second/third/etc entry.  So it still seems redundant to 
> include the pure beacon one?

Yeah, it's redundant to some extent. If you're an older application not
looking at BEACON_IES attribute though, you don't see the extra beacon
data in the scan result for the hidden network that had a probe response
received.

Internally, we have to create two different BSS entries so that we can
track the BSSID/channel -> IEs mapping in one, and the potentially
multiple probe responses with different IEs in the other entries. We
ignore that relationship when sending things to userspace, but the
multiple entries are there.

> Also, given that you have to ask for the SSID you want specifically, 
> what practical purpose does it serve to know that this wasn't the only 
> hidden SSID?  I mean you can see that hidden SSIDs are out there, run an 
> active scan for the ones you can use.  If none are there, you can just 
> ignore that bssid...

It's really just the same code though, so we'd have to have extra code
to filter if it has children, and perhaps if the client said it
understood beacon IEs or something like that ... doesn't seem worth it
to save an extra entry being sent to userspace in the (hopefully rare)
case of hidden SSID.
johannes