[PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Ujjal Roy]

cfg80211 allows re-association in managed mode and if a user
wants to re-associate to the same AP network after the time
period of IEEE80211_SCAN_RESULT_EXPIRE, cfg80211 warns with
the following message on receiving the connect result event.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 13984 at net/wireless/sme.c:658
         __cfg80211_connect_result+0x3a6/0x3e0 [cfg80211]()
Call Trace:
 [<ffffffff81747a41>] dump_stack+0x46/0x58
 [<ffffffff81045847>] warn_slowpath_common+0x87/0xb0
 [<ffffffff81045885>] warn_slowpath_null+0x15/0x20
 [<ffffffffa05345f6>] __cfg80211_connect_result+0x3a6/0x3e0 [cfg80211]
 [<ffffffff8107168b>] ? update_rq_clock+0x2b/0x50
 [<ffffffff81078c01>] ? update_curr+0x1/0x160
 [<ffffffffa05133d2>] cfg80211_process_wdev_events+0xb2/0x1c0 [cfg80211]
 [<ffffffff81079303>] ? pick_next_task_fair+0x63/0x170
 [<ffffffffa0513518>] cfg80211_process_rdev_events+0x38/0x90 [cfg80211]
 [<ffffffffa050f03d>] cfg80211_event_work+0x1d/0x30 [cfg80211]
 [<ffffffff8105f21f>] process_one_work+0x17f/0x420
 [<ffffffff8105f90a>] worker_thread+0x11a/0x370
 [<ffffffff8105f7f0>] ? rescuer_thread+0x2f0/0x2f0
 [<ffffffff8106638b>] kthread+0xbb/0xc0
 [<ffffffff810662d0>] ? kthread_create_on_node+0x120/0x120
 [<ffffffff817574bc>] ret_from_fork+0x7c/0xb0
 [<ffffffff810662d0>] ? kthread_create_on_node+0x120/0x120
---[ end trace 61f3bddc9c4981f7 ]---

The reason is that, in connect result event cfg80211 unholds
the BSS to which the device is associated (and was held so
far). So, for the event with status successful, when cfg80211
wants to get that BSS from the device's BSS list it gets a
NULL BSS because the BSS has been expired and unheld already.

Fix this by searching and saving the BSS before it gets unhold
by the cfg80211. Use that saved BSS and hold it again when the
assiciation is successful.

Signed-off-by: Ujjal Roy <royujjal@gmail.com>
---
 net/wireless/sme.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 65f8008..5eeb8e0 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -589,6 +589,7 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 			       struct cfg80211_bss *bss)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	struct cfg80211_bss *save_bss = NULL;
 	const u8 *country_ie;
 #ifdef CONFIG_CFG80211_WEXT
 	union iwreq_data wrqu;
@@ -632,6 +633,11 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 	}
 #endif
 
+	if (!bss)
+		save_bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
+					    wdev->ssid, wdev->ssid_len,
+					    WLAN_CAPABILITY_ESS,
+					    WLAN_CAPABILITY_ESS);
 	if (wdev->current_bss) {
 		cfg80211_unhold_bss(wdev->current_bss);
 		cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
@@ -651,10 +657,7 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 
 	if (!bss) {
 		WARN_ON_ONCE(!wiphy_to_dev(wdev->wiphy)->ops->connect);
-		bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
-				       wdev->ssid, wdev->ssid_len,
-				       WLAN_CAPABILITY_ESS,
-				       WLAN_CAPABILITY_ESS);
+		bss = save_bss;
 		if (WARN_ON(!bss))
 			return;
 		cfg80211_hold_bss(bss_from_pub(bss));
-- 
1.8.3.1

[PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Ujjal Roy]

cfg80211 allows re-association in managed mode and if a user
wants to re-associate to the same AP network after the time
period of IEEE80211_SCAN_RESULT_EXPIRE, cfg80211 warns with
the following message on receiving the connect result event.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 13984 at net/wireless/sme.c:658
         __cfg80211_connect_result+0x3a6/0x3e0 [cfg80211]()
Call Trace:
 [<ffffffff81747a41>] dump_stack+0x46/0x58
 [<ffffffff81045847>] warn_slowpath_common+0x87/0xb0
 [<ffffffff81045885>] warn_slowpath_null+0x15/0x20
 [<ffffffffa05345f6>] __cfg80211_connect_result+0x3a6/0x3e0 [cfg80211]
 [<ffffffff8107168b>] ? update_rq_clock+0x2b/0x50
 [<ffffffff81078c01>] ? update_curr+0x1/0x160
 [<ffffffffa05133d2>] cfg80211_process_wdev_events+0xb2/0x1c0 [cfg80211]
 [<ffffffff81079303>] ? pick_next_task_fair+0x63/0x170
 [<ffffffffa0513518>] cfg80211_process_rdev_events+0x38/0x90 [cfg80211]
 [<ffffffffa050f03d>] cfg80211_event_work+0x1d/0x30 [cfg80211]
 [<ffffffff8105f21f>] process_one_work+0x17f/0x420
 [<ffffffff8105f90a>] worker_thread+0x11a/0x370
 [<ffffffff8105f7f0>] ? rescuer_thread+0x2f0/0x2f0
 [<ffffffff8106638b>] kthread+0xbb/0xc0
 [<ffffffff810662d0>] ? kthread_create_on_node+0x120/0x120
 [<ffffffff817574bc>] ret_from_fork+0x7c/0xb0
 [<ffffffff810662d0>] ? kthread_create_on_node+0x120/0x120
---[ end trace 61f3bddc9c4981f7 ]---

The reason is that, in connect result event cfg80211 unholds
the BSS to which the device is associated (and was held so
far). So, for the event with status successful, when cfg80211
wants to get that BSS from the device's BSS list it gets a
NULL BSS because the BSS has been expired and unheld already.

Fix this by searching and saving the BSS before it gets unhold
by the cfg80211. Use that saved BSS and hold it again when the
assiciation is successful.

Signed-off-by: Ujjal Roy <royujjal@gmail.com>
---
 net/wireless/sme.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 65f8008..5eeb8e0 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -589,6 +589,7 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 			       struct cfg80211_bss *bss)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
+	struct cfg80211_bss *save_bss = NULL;
 	const u8 *country_ie;
 #ifdef CONFIG_CFG80211_WEXT
 	union iwreq_data wrqu;
@@ -632,6 +633,11 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 	}
 #endif
 
+	if (!bss)
+		save_bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
+					    wdev->ssid, wdev->ssid_len,
+					    WLAN_CAPABILITY_ESS,
+					    WLAN_CAPABILITY_ESS);
 	if (wdev->current_bss) {
 		cfg80211_unhold_bss(wdev->current_bss);
 		cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
@@ -651,10 +657,7 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 
 	if (!bss) {
 		WARN_ON_ONCE(!wiphy_to_dev(wdev->wiphy)->ops->connect);
-		bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
-				       wdev->ssid, wdev->ssid_len,
-				       WLAN_CAPABILITY_ESS,
-				       WLAN_CAPABILITY_ESS);
+		bss = save_bss;
 		if (WARN_ON(!bss))
 			return;
 		cfg80211_hold_bss(bss_from_pub(bss));
-- 
1.8.3.1

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Johannes Berg]

On Sun, 2013-12-01 at 13:01 +0530, Ujjal Roy wrote:
> cfg80211 allows re-association in managed mode and if a user
> wants to re-associate to the same AP network after the time
> period of IEEE80211_SCAN_RESULT_EXPIRE, cfg80211 warns with
> the following message on receiving the connect result event.

This behaviour is intentional to point out driver bugs. I'm not applying
this.

johannes

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Ujjal Roy]

So, can you please tell me what type of bugs or what bugs the cfg80211
going to catch? As I see cfg80211 allows re-assoc command and the
driver is able to get a BSS using the function cfg80211_get_bss(). So,
the driver is going to re-associate and the result is a Kernel warning.
At this point driver's internal association is successful though.

On Sun, Dec 1, 2013 at 3:02 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2013-12-01 at 13:01 +0530, Ujjal Roy wrote:
>> cfg80211 allows re-association in managed mode and if a user
>> wants to re-associate to the same AP network after the time
>> period of IEEE80211_SCAN_RESULT_EXPIRE, cfg80211 warns with
>> the following message on receiving the connect result event.
>
> This behaviour is intentional to point out driver bugs. I'm not applying
> this.
>
> johannes
>

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Ujjal Roy]

Hi Johannes,

Are there any update about my arguments?

Thanks,
UjjaL Roy


On Mon, Dec 2, 2013 at 10:49 AM, Ujjal Roy <royujjal@gmail.com> wrote:
> So, can you please tell me what type of bugs or what bugs the cfg80211
> going to catch? As I see cfg80211 allows re-assoc command and the
> driver is able to get a BSS using the function cfg80211_get_bss(). So,
> the driver is going to re-associate and the result is a Kernel warning.
> At this point driver's internal association is successful though.
>
> On Sun, Dec 1, 2013 at 3:02 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
>> On Sun, 2013-12-01 at 13:01 +0530, Ujjal Roy wrote:
>>> cfg80211 allows re-association in managed mode and if a user
>>> wants to re-associate to the same AP network after the time
>>> period of IEEE80211_SCAN_RESULT_EXPIRE, cfg80211 warns with
>>> the following message on receiving the connect result event.
>>
>> This behaviour is intentional to point out driver bugs. I'm not applying
>> this.
>>
>> johannes
>>

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Johannes Berg]

On Tue, 2013-12-03 at 11:54 +0530, Ujjal Roy wrote:
> Hi Johannes,
> 
> Are there any update about my arguments?

Oh please. You didn't even give me 24 hours to reply. I'm not sitting
here just waiting for your email 24 hours a day!

Please increase your minimum waiting time to *at least* a few days,
preferably a week.

johannes

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Johannes Berg]

On Mon, 2013-12-02 at 10:49 +0530, Ujjal Roy wrote:
> So, can you please tell me what type of bugs or what bugs the cfg80211
> going to catch? As I see cfg80211 allows re-assoc command and the
> driver is able to get a BSS using the function cfg80211_get_bss(). So,
> the driver is going to re-associate and the result is a Kernel warning.
> At this point driver's internal association is successful though.

Yeah, I think you're right - I was confusing this with some Broadcom
thing (the joys of everyone using gmail ...)

Why would you ever reassoc to the exact same BSS? But anyway ... I'll
take a closer look at your patch/the code.

johannes

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Ujjal Roy]

> Why would you ever reassoc to the exact same BSS? But anyway ... I'll
> take a closer look at your patch/the code.
>
> johannes
>

cfg80211 allows to do that. So, its the responsibility of cfg80211
not to have any issues if anyone uses this type of reassoc.

-- 
Thanks,
UjjaL Roy

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Johannes Berg]

On Sun, 2013-12-01 at 13:01 +0530, Ujjal Roy wrote:

> +	if (!bss)
> +		save_bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
> +					    wdev->ssid, wdev->ssid_len,
> +					    WLAN_CAPABILITY_ESS,
> +					    WLAN_CAPABILITY_ESS);
>  	if (wdev->current_bss) {
>  		cfg80211_unhold_bss(wdev->current_bss);
>  		cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
> @@ -651,10 +657,7 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
>  
>  	if (!bss) {
>  		WARN_ON_ONCE(!wiphy_to_dev(wdev->wiphy)->ops->connect);
> -		bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
> -				       wdev->ssid, wdev->ssid_len,
> -				       WLAN_CAPABILITY_ESS,
> -				       WLAN_CAPABILITY_ESS);
> +		bss = save_bss;

In an unsuccessful result case, this leaks the BSS struct. I'd prefer to
reshuffle the code a bit to not need a separate save_bss variable, but I
don't immediately see an easy way to do that.

johannes

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Ujjal Roy]

On Tue, Dec 3, 2013 at 8:00 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Sun, 2013-12-01 at 13:01 +0530, Ujjal Roy wrote:
>
>> +     if (!bss)
>> +             save_bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
>> +                                         wdev->ssid, wdev->ssid_len,
>> +                                         WLAN_CAPABILITY_ESS,
>> +                                         WLAN_CAPABILITY_ESS);
>>       if (wdev->current_bss) {
>>               cfg80211_unhold_bss(wdev->current_bss);
>>               cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
>> @@ -651,10 +657,7 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
>>
>>       if (!bss) {
>>               WARN_ON_ONCE(!wiphy_to_dev(wdev->wiphy)->ops->connect);
>> -             bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
>> -                                    wdev->ssid, wdev->ssid_len,
>> -                                    WLAN_CAPABILITY_ESS,
>> -                                    WLAN_CAPABILITY_ESS);
>> +             bss = save_bss;
>
> In an unsuccessful result case, this leaks the BSS struct. I'd prefer to
> reshuffle the code a bit to not need a separate save_bss variable, but I
> don't immediately see an easy way to do that.
>
> johannes
>

Thanks for pointing out the leak of BSS. I have reshuffled the code to
avoid an extra
variable. Please have a look at this code which I am sending through
git send-mail.

-- 
Thanks,
UjjaL Roy

[PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Ujjal Roy]

cfg80211 allows re-association in managed mode and if a user
wants to re-associate to the same AP network after the time
period of IEEE80211_SCAN_RESULT_EXPIRE, cfg80211 warns with
the following message on receiving the connect result event.

------------[ cut here ]------------
WARNING: CPU: 0 PID: 13984 at net/wireless/sme.c:658
         __cfg80211_connect_result+0x3a6/0x3e0 [cfg80211]()
Call Trace:
 [<ffffffff81747a41>] dump_stack+0x46/0x58
 [<ffffffff81045847>] warn_slowpath_common+0x87/0xb0
 [<ffffffff81045885>] warn_slowpath_null+0x15/0x20
 [<ffffffffa05345f6>] __cfg80211_connect_result+0x3a6/0x3e0 [cfg80211]
 [<ffffffff8107168b>] ? update_rq_clock+0x2b/0x50
 [<ffffffff81078c01>] ? update_curr+0x1/0x160
 [<ffffffffa05133d2>] cfg80211_process_wdev_events+0xb2/0x1c0 [cfg80211]
 [<ffffffff81079303>] ? pick_next_task_fair+0x63/0x170
 [<ffffffffa0513518>] cfg80211_process_rdev_events+0x38/0x90 [cfg80211]
 [<ffffffffa050f03d>] cfg80211_event_work+0x1d/0x30 [cfg80211]
 [<ffffffff8105f21f>] process_one_work+0x17f/0x420
 [<ffffffff8105f90a>] worker_thread+0x11a/0x370
 [<ffffffff8105f7f0>] ? rescuer_thread+0x2f0/0x2f0
 [<ffffffff8106638b>] kthread+0xbb/0xc0
 [<ffffffff810662d0>] ? kthread_create_on_node+0x120/0x120
 [<ffffffff817574bc>] ret_from_fork+0x7c/0xb0
 [<ffffffff810662d0>] ? kthread_create_on_node+0x120/0x120
---[ end trace 61f3bddc9c4981f7 ]---

The reason is that, in connect result event cfg80211 unholds
the BSS to which the device is associated (and was held so
far). So, for the event with status successful, when cfg80211
wants to get that BSS from the device's BSS list it gets a
NULL BSS because the BSS has been expired and unheld already.

Fix it by reshuffling the code.

Signed-off-by: Ujjal Roy <royujjal@gmail.com>
---
 net/wireless/sme.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 65f8008..d3c5bd7 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -632,6 +632,16 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 	}
 #endif
 
+	if (!bss && (status == WLAN_STATUS_SUCCESS)) {
+		WARN_ON_ONCE(!wiphy_to_dev(wdev->wiphy)->ops->connect);
+		bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
+				       wdev->ssid, wdev->ssid_len,
+				       WLAN_CAPABILITY_ESS,
+				       WLAN_CAPABILITY_ESS);
+		if (bss)
+			cfg80211_hold_bss(bss_from_pub(bss));
+	}
+
 	if (wdev->current_bss) {
 		cfg80211_unhold_bss(wdev->current_bss);
 		cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
@@ -649,16 +659,8 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 		return;
 	}
 
-	if (!bss) {
-		WARN_ON_ONCE(!wiphy_to_dev(wdev->wiphy)->ops->connect);
-		bss = cfg80211_get_bss(wdev->wiphy, NULL, bssid,
-				       wdev->ssid, wdev->ssid_len,
-				       WLAN_CAPABILITY_ESS,
-				       WLAN_CAPABILITY_ESS);
-		if (WARN_ON(!bss))
-			return;
-		cfg80211_hold_bss(bss_from_pub(bss));
-	}
+	if (WARN_ON(!bss))
+		return;
 
 	wdev->current_bss = bss_from_pub(bss);
 
-- 
1.8.1.4

Re: [PATCH] cfg80211: fix WARN_ON for re-association to the expired BSS

[Johannes Berg]

On Wed, 2013-12-04 at 17:27 +0530, Ujjal Roy wrote:
> cfg80211 allows re-association in managed mode and if a user
> wants to re-associate to the same AP network after the time
> period of IEEE80211_SCAN_RESULT_EXPIRE, cfg80211 warns with
> the following message on receiving the connect result event.

Applied.

johannes