Skip to contentSkip to conversation

wireless: Make sure __cfg80211_connect_result always puts bss.

PATCH Applied [email protected] 2 participants 5 messages Jun 19, 2013 – Jun 24, 2013 802.11 (including cfg80211/NL80211)

lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/3 wireless: Make sure __cfg80211_connect_result always puts bss.

From: Ben Greear <[email protected]>

Otherwise, we can leak a bss reference.

Signed-off-by Ben Greear <[email protected]>

---

This is against wireless-next.

 net/wireless/sme.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
net/wireless/sme.c +4 -1
index c0bf781..32dac8c 100644
@@ -557,6 +557,7 @@ static DECLARE_WORK(cfg80211_disconnect_work, disconnect_work);
  * SME event handling
  */
 
+/* This method must consume bss one way or another */
 void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 			       const u8 *req_ie, size_t req_ie_len,
 			       const u8 *resp_ie, size_t resp_ie_len,

@@ -572,8 +573,10 @@ void __cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 	ASSERT_WDEV_LOCK(wdev);
 
 	if (WARN_ON(wdev->iftype != NL80211_IFTYPE_STATION &&
-		    wdev->iftype != NL80211_IFTYPE_P2P_CLIENT))
+		    wdev->iftype != NL80211_IFTYPE_P2P_CLIENT)) {
+		cfg80211_put_bss(wdev->wiphy, bss);
 		return;
+	}
 
 	nl80211_send_connect_result(wiphy_to_dev(wdev->wiphy), dev,
 				    bssid, req_ie, req_ie_len,
Signature · 1 line
1.7.3.4
author
#1 lore raw

PATCH 2/3 wireless: Check for dangling wdev->current_bss pointer.

From: Ben Greear <[email protected]>

If it *is* still set when the netdev is being deleted, then we are about to leak a pointer. Warn and clean up in that case.

Signed-off-by Ben Greear <[email protected]>

---

This is against wireless-next. I've never seen this hit, though possibly it would catch some future bugs. If you think it's not useful, I won't argue :)

 net/wireless/core.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
net/wireless/core.c +6 -0
index 9143288..042d6a3 100644
@@ -934,6 +934,12 @@ static int cfg80211_netdev_notifier_call(struct notifier_block *nb,
 		 * freed.
 		 */
 		cfg80211_process_wdev_events(wdev);
+
+		if (WARN_ON(wdev->current_bss)) {
+			cfg80211_unhold_bss(wdev->current_bss);
+			cfg80211_put_bss(wdev->wiphy, &wdev->current_bss->pub);
+			wdev->current_bss = NULL;
+		}
 		break;
 	case NETDEV_PRE_UP:
 		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype)))
Signature · 1 line
1.7.3.4
author
#2 lore raw

PATCH 3/3 wireless: Add comments about bss refcounting.

In reply to [email protected]

From: Ben Greear <[email protected]>

Should help the next person that tries to understand the bss refcounting logic.

Signed-off-by Ben Greear <[email protected]>

---

This is against wireless-next.

 net/wireless/scan.c |    4 ++++
 net/wireless/sme.c  |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)
net/wireless/scan.c +4 -0
index abb0399..d2f4db0 100644
@@ -623,6 +623,7 @@ static int cmp_bss(struct cfg80211_bss *a,
 	}
 }
 
+/* Returned bss is reference counted and must be cleaned up appropriately. */
 struct cfg80211_bss *cfg80211_get_bss(struct wiphy *wiphy,
 				      struct ieee80211_channel *channel,
 				      const u8 *bssid,

@@ -778,6 +779,7 @@ static bool cfg80211_combine_bsses(struct cfg80211_registered_device *dev,
 	return true;
 }
 
+/* Returned bss is reference counted and must be cleaned up appropriately. */
 static struct cfg80211_internal_bss *
 cfg80211_bss_update(struct cfg80211_registered_device *dev,
 		    struct cfg80211_internal_bss *tmp)

@@ -963,6 +965,7 @@ cfg80211_get_bss_channel(struct wiphy *wiphy, const u8 *ie, size_t ielen,
 	return channel;
 }
 
+/* Returned bss is reference counted and must be cleaned up appropriately. */
 struct cfg80211_bss*
 cfg80211_inform_bss(struct wiphy *wiphy,
 		    struct ieee80211_channel *channel,

@@ -1020,6 +1023,7 @@ cfg80211_inform_bss(struct wiphy *wiphy,
 }
 EXPORT_SYMBOL(cfg80211_inform_bss);
 
+/* Returned bss is reference counted and must be cleaned up appropriately. */
 struct cfg80211_bss *
 cfg80211_inform_bss_frame(struct wiphy *wiphy,
 			  struct ieee80211_channel *channel,
net/wireless/sme.c +3 -0
index 32dac8c..1d3cfb1 100644
@@ -239,6 +239,7 @@ void cfg80211_conn_work(struct work_struct *work)
 	rtnl_unlock();
 }
 
+/* Returned bss is reference counted and must be cleaned up appropriately. */
 static struct cfg80211_bss *cfg80211_get_conn_bss(struct wireless_dev *wdev)
 {
 	struct cfg80211_registered_device *rdev = wiphy_to_dev(wdev->wiphy);

@@ -699,6 +700,7 @@ void cfg80211_connect_result(struct net_device *dev, const u8 *bssid,
 }
 EXPORT_SYMBOL(cfg80211_connect_result);
 
+/* Consumes bss object one way or another */
 void __cfg80211_roamed(struct wireless_dev *wdev,
 		       struct cfg80211_bss *bss,
 		       const u8 *req_ie, size_t req_ie_len,

@@ -775,6 +777,7 @@ void cfg80211_roamed(struct net_device *dev,
 }
 EXPORT_SYMBOL(cfg80211_roamed);
 
+/* Consumes bss object one way or another */
 void cfg80211_roamed_bss(struct net_device *dev,
 			 struct cfg80211_bss *bss, const u8 *req_ie,
 			 size_t req_ie_len, const u8 *resp_ie,
Signature · 1 line
1.7.3.4
author
#4 lore raw
Quoted 7 lines · Johannes Berg
Quoted 3 lines · On Wed, 2013-06-19 at 14:06 -0700, [email protected] wrote:

From: Ben Greear <[email protected]>

Otherwise, we can leak a bss reference.

Applied all 3, still trying to make up my mind about the debugfs one patch :)

With regard to that one, how about I allow it to be enabled based on cfg80211 module option (and only when cfg80211 debugfs is enabled).

That way, no new kernel option, and no significant hit to any runtime performance unless users specifically enable the feature.

Thanks for applying the other three.

Thanks,
Ben

Quoted 3 lines

johannes

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com