[PATCH 1/2] cfg80211: require passing BSS struct back to cfg80211_assoc_timeout

[PATCH 1/2] cfg80211: require passing BSS struct back to cfg80211_assoc_timeout

[Johannes Berg]

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

Doing so will allow us to hold the BSS (not just ref it) over the
association process, thus ensuring that it doesn't time out and
gets invisible to the user (e.g. in 'iw wlan0 link'.)

This also fixes a leak in mac80211 where it doesn't always release
the BSS struct properly in all cases where calling this function.
This leak was reported by Ben Greear.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/cfg80211.h | 16 ++++++++--------
 net/mac80211/mlme.c    | 15 +++++++--------
 net/wireless/mlme.c    |  8 +++++---
 3 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index e3a39fc..7b0730a 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1459,7 +1459,8 @@ const u8 *ieee80211_bss_get_ie(struct cfg80211_bss *bss, u8 ie);
  * This structure provides information needed to complete IEEE 802.11
  * authentication.
  *
- * @bss: The BSS to authenticate with.
+ * @bss: The BSS to authenticate with, the callee must obtain a reference
+ *	to it if it needs to keep it.
  * @auth_type: Authentication type (algorithm)
  * @ie: Extra IEs to add to Authentication frame or %NULL
  * @ie_len: Length of ie buffer in octets
@@ -1497,11 +1498,10 @@ enum cfg80211_assoc_req_flags {
  *
  * This structure provides information needed to complete IEEE 802.11
  * (re)association.
- * @bss: The BSS to associate with. If the call is successful the driver
- *	is given a reference that it must release, normally via a call to
- *	cfg80211_send_rx_assoc(), or, if association timed out, with a
- *	call to cfg80211_put_bss() (in addition to calling
- *	cfg80211_send_assoc_timeout())
+ * @bss: The BSS to associate with. If the call is successful the driver is
+ *	given a reference that it must give back to cfg80211_send_rx_assoc()
+ *	or to cfg80211_assoc_timeout(). To ensure proper refcounting, new
+ *	association requests while already associating must be rejected.
  * @ie: Extra IEs to add to (Re)Association Request frame or %NULL
  * @ie_len: Length of ie buffer in octets
  * @use_mfp: Use management frame protection (IEEE 802.11w) in this association
@@ -3522,11 +3522,11 @@ void cfg80211_rx_assoc_resp(struct net_device *dev,
 /**
  * cfg80211_assoc_timeout - notification of timed out association
  * @dev: network device
- * @addr: The MAC address of the device with which the association timed out
+ * @bss: The BSS entry with which association timed out.
  *
  * This function may sleep. The caller must hold the corresponding wdev's mutex.
  */
-void cfg80211_assoc_timeout(struct net_device *dev, const u8 *addr);
+void cfg80211_assoc_timeout(struct net_device *dev, struct cfg80211_bss *bss);
 
 /**
  * cfg80211_tx_mlme_mgmt - notification of transmitted deauth/disassoc frame
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index e0939eb..08b3e71 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -2722,8 +2722,7 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 		if (!ieee80211_assoc_success(sdata, bss, mgmt, len)) {
 			/* oops -- internal error -- send timeout for now */
 			ieee80211_destroy_assoc_data(sdata, false);
-			cfg80211_put_bss(sdata->local->hw.wiphy, bss);
-			cfg80211_assoc_timeout(sdata->dev, mgmt->bssid);
+			cfg80211_assoc_timeout(sdata->dev, bss);
 			return;
 		}
 		sdata_info(sdata, "associated\n");
@@ -3440,13 +3439,10 @@ void ieee80211_sta_work(struct ieee80211_sub_if_data *sdata)
 	    time_after(jiffies, ifmgd->assoc_data->timeout)) {
 		if ((ifmgd->assoc_data->need_beacon && !ifmgd->have_beacon) ||
 		    ieee80211_do_assoc(sdata)) {
-			u8 bssid[ETH_ALEN];
-
-			memcpy(bssid, ifmgd->assoc_data->bss->bssid, ETH_ALEN);
+			struct cfg80211_bss *bss = ifmgd->assoc_data->bss;
 
 			ieee80211_destroy_assoc_data(sdata, false);
-
-			cfg80211_assoc_timeout(sdata->dev, bssid);
+			cfg80211_assoc_timeout(sdata->dev, bss);
 		}
 	} else if (ifmgd->assoc_data && ifmgd->assoc_data->timeout_started)
 		run_again(sdata, ifmgd->assoc_data->timeout);
@@ -4372,8 +4368,11 @@ void ieee80211_mgd_stop(struct ieee80211_sub_if_data *sdata)
 	cancel_work_sync(&ifmgd->chswitch_work);
 
 	sdata_lock(sdata);
-	if (ifmgd->assoc_data)
+	if (ifmgd->assoc_data) {
+		struct cfg80211_bss *bss = ifmgd->assoc_data->bss;
 		ieee80211_destroy_assoc_data(sdata, false);
+		cfg80211_assoc_timeout(sdata->dev, bss);
+	}
 	if (ifmgd->auth_data)
 		ieee80211_destroy_auth_data(sdata, false);
 	del_timer_sync(&ifmgd->timer);
diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index a61a44b..dd6f79d 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -131,16 +131,18 @@ void cfg80211_auth_timeout(struct net_device *dev, const u8 *addr)
 }
 EXPORT_SYMBOL(cfg80211_auth_timeout);
 
-void cfg80211_assoc_timeout(struct net_device *dev, const u8 *addr)
+void cfg80211_assoc_timeout(struct net_device *dev, struct cfg80211_bss *bss)
 {
 	struct wireless_dev *wdev = dev->ieee80211_ptr;
 	struct wiphy *wiphy = wdev->wiphy;
 	struct cfg80211_registered_device *rdev = wiphy_to_dev(wiphy);
 
-	trace_cfg80211_send_assoc_timeout(dev, addr);
+	trace_cfg80211_send_assoc_timeout(dev, bss->bssid);
 
-	nl80211_send_assoc_timeout(rdev, dev, addr, GFP_KERNEL);
+	nl80211_send_assoc_timeout(rdev, dev, bss->bssid, GFP_KERNEL);
 	cfg80211_sme_assoc_timeout(wdev);
+
+	cfg80211_put_bss(wiphy, bss);
 }
 EXPORT_SYMBOL(cfg80211_assoc_timeout);
 
-- 
1.8.0

[PATCH 2/2] cfg80211: hold BSS over association process

[Johannes Berg]

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

This fixes the potential issue that the BSS struct that we use
and later assign to wdev->current_bss is removed from the scan
list while associating.

Also warn when we don't have a BSS struct in connect_result
unless it's from a driver that only has the connect() API.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 net/wireless/mlme.c |  4 ++++
 net/wireless/sme.c  | 15 ++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index dd6f79d..bfac5e1 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -38,6 +38,7 @@ void cfg80211_rx_assoc_resp(struct net_device *dev, struct cfg80211_bss *bss,
 	 * frame instead of reassoc.
 	 */
 	if (cfg80211_sme_rx_assoc_resp(wdev, status_code)) {
+		cfg80211_unhold_bss(bss_from_pub(bss));
 		cfg80211_put_bss(wiphy, bss);
 		return;
 	}
@@ -142,6 +143,7 @@ void cfg80211_assoc_timeout(struct net_device *dev, struct cfg80211_bss *bss)
 	nl80211_send_assoc_timeout(rdev, dev, bss->bssid, GFP_KERNEL);
 	cfg80211_sme_assoc_timeout(wdev);
 
+	cfg80211_unhold_bss(bss_from_pub(bss));
 	cfg80211_put_bss(wiphy, bss);
 }
 EXPORT_SYMBOL(cfg80211_assoc_timeout);
@@ -309,6 +311,8 @@ int cfg80211_mlme_assoc(struct cfg80211_registered_device *rdev,
 		goto out;
 
 	err = rdev_assoc(rdev, dev, req);
+	if (!err)
+		cfg80211_hold_bss(bss_from_pub(req->bss));
 
 out:
 	if (err)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index ae7e2cb..c0bf781 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -615,19 +615,24 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 		kfree(wdev->connect_keys);
 		wdev->connect_keys = NULL;
 		wdev->ssid_len = 0;
-		cfg80211_put_bss(wdev->wiphy, bss);
+		if (bss) {
+			cfg80211_unhold_bss(bss_from_pub(bss));
+			cfg80211_put_bss(wdev->wiphy, bss);
+		}
 		return;
 	}
 
-	if (!bss)
+	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;
+		if (WARN_ON(!bss))
+			return;
+		cfg80211_hold_bss(bss_from_pub(bss));
+	}
 
-	cfg80211_hold_bss(bss_from_pub(bss));
 	wdev->current_bss = bss_from_pub(bss);
 
 	cfg80211_upload_connect_keys(wdev);
-- 
1.8.0

Re: [PATCH 2/2] cfg80211: hold BSS over association process

[Ben Greear]

On 06/19/2013 06:53 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> This fixes the potential issue that the BSS struct that we use
> and later assign to wdev->current_bss is removed from the scan
> list while associating.
>
> Also warn when we don't have a BSS struct in connect_result
> unless it's from a driver that only has the connect() API.

This also looks OK to me, but I have not tested it.

Thanks,
Ben

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

Re: [PATCH 2/2] cfg80211: hold BSS over association process

[Ben Greear]

On 06/19/2013 06:53 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> This fixes the potential issue that the BSS struct that we use
> and later assign to wdev->current_bss is removed from the scan
> list while associating.
>
> Also warn when we don't have a BSS struct in connect_result
> unless it's from a driver that only has the connect() API.

I ran these two patches, plus my memory debugging patch
on top of wireless-testing (in relatively light testing).

It appears to fix the leaks I was seeing.

Just to make sure I am not missing something:

mac80211 should never take an additional reference
on the bss related to auth_data now?

Thanks,
Ben

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

Re: [PATCH 2/2] cfg80211: hold BSS over association process

[Johannes Berg]

On Wed, 2013-06-19 at 13:28 -0700, Ben Greear wrote:
> On 06/19/2013 06:53 AM, Johannes Berg wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > This fixes the potential issue that the BSS struct that we use
> > and later assign to wdev->current_bss is removed from the scan
> > list while associating.
> >
> > Also warn when we don't have a BSS struct in connect_result
> > unless it's from a driver that only has the connect() API.
> 
> I ran these two patches, plus my memory debugging patch
> on top of wireless-testing (in relatively light testing).
> 
> It appears to fix the leaks I was seeing.
> 
> Just to make sure I am not missing something:
> 
> mac80211 should never take an additional reference
> on the bss related to auth_data now?

On *auth_data* it takes and (unconditionally) puts one. On *assoc_data*
the reference is only temporarily owned by mac80211 until given back to
cfg80211, and unless mac80211 wanted to have a separate one afterwards
it doesn't need one (it would probably make sense to take one for
"ifmgd->associated" though, but currently it relies on that being the
same as "wdev->current_bss")

johannes

Re: [PATCH 1/2] cfg80211: require passing BSS struct back to cfg80211_assoc_timeout

[Ben Greear]

On 06/19/2013 06:53 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Doing so will allow us to hold the BSS (not just ref it) over the
> association process, thus ensuring that it doesn't time out and
> gets invisible to the user (e.g. in 'iw wlan0 link'.)
>
> This also fixes a leak in mac80211 where it doesn't always release
> the BSS struct properly in all cases where calling this function.
> This leak was reported by Ben Greear.

I have not yet tested this, but I read it closely and
it appears to fix the same problem I tried to fix in
the 2/6 "Fix bss leak." patch.

Thanks,
Ben

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

Re: [PATCH 1/2] cfg80211: require passing BSS struct back to cfg80211_assoc_timeout

[Johannes Berg]

On Wed, 2013-06-19 at 15:53 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> Doing so will allow us to hold the BSS (not just ref it) over the
> association process, thus ensuring that it doesn't time out and
> gets invisible to the user (e.g. in 'iw wlan0 link'.)
> 
> This also fixes a leak in mac80211 where it doesn't always release
> the BSS struct properly in all cases where calling this function.
> This leak was reported by Ben Greear.

Applied both.

johannes