[PATCH-WN 1/3] wireless: Make sure __cfg80211_connect_result always puts bss.

[PATCH-WN 1/3] wireless: Make sure __cfg80211_connect_result always puts bss.

[greearb]

From: Ben Greear <greearb@candelatech.com>

Otherwise, we can leak a bss reference.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

This is against wireless-next.

 net/wireless/sme.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index c0bf781..32dac8c 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -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,
-- 
1.7.3.4

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

[greearb]

From: Ben Greear <greearb@candelatech.com>

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 <greearb@candelatech.com>
---

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(-)

diff --git a/net/wireless/core.c b/net/wireless/core.c
index 9143288..042d6a3 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -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)))
-- 
1.7.3.4

[PATCH-WN 3/3] wireless: Add comments about bss refcounting.

[greearb]

From: Ben Greear <greearb@candelatech.com>

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

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

This is against wireless-next.

 net/wireless/scan.c |    4 ++++
 net/wireless/sme.c  |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index abb0399..d2f4db0 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -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,
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 32dac8c..1d3cfb1 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -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,
-- 
1.7.3.4

Re: [PATCH-WN 1/3] wireless: Make sure __cfg80211_connect_result always puts bss.

[Johannes Berg]

On Wed, 2013-06-19 at 14:06 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Otherwise, we can leak a bss reference.

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

johannes

Re: [PATCH-WN 1/3] wireless: Make sure __cfg80211_connect_result always puts bss.

[Ben Greear]

On 06/24/2013 06:56 AM, Johannes Berg wrote:
> On Wed, 2013-06-19 at 14:06 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> 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


>
> johannes
>


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