wireless: Add memory usage debugging.

PATCH In review [email protected] 4 participants 14 messages Jun 18, 2013 – Jun 22, 2013

Subsystems (2)

mac80211 802.11 (including cfg80211/NL80211)

lore

Share & tools

lore

[email protected]

author

2013-06-18 22:03 UTC #0 lore raw

PATCH 1/6 wireless: Add memory usage debugging.

From: Ben Greear <[email protected]>

The bss objects are reference counted, and the ies are also tricky to keep track of. Add option to track allocation and freeing of the ies and bss objects, and add debugfs files to show the current objects.

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

---
 net/wireless/Kconfig   |   13 +++++
 net/wireless/core.c    |    5 +-
 net/wireless/core.h    |   17 ++++++
 net/wireless/debugfs.c |  117 ++++++++++++++++++++++++++++++++++++++++++++
 net/wireless/debugfs.h |    2 +
 net/wireless/scan.c    |  127 ++++++++++++++++++++++++++++++++++++++++++------
 6 files changed, 264 insertions(+), 17 deletions(-)

net/wireless/Kconfig +13 -0

index 16d08b3..43ec2cd 100644
@@ -115,6 +115,19 @@ config CFG80211_DEBUGFS
 
 	  If unsure, say N.
 
+config CFG80211_MEM_DEBUGGING
+	bool "cfg80211 memory debugging logic"
+	default n
+	depends on CFG80211_DEBUGFS
+	---help---
+	  Enable this if you want to debug memory handling for bss and ies
+	  objects.  New debugfs files: ieee80211/all_ies and all_bss will
+	  be created to display these objects.  This has a moderate CPU cost
+	  and uses a bit more memory than normal, but otherwise is not very
+	  expensive.
+
+	  If unsure, say N.
+
 config CFG80211_INTERNAL_REGDB
 	bool "use statically compiled regulatory rules database" if EXPERT
 	default n

net/wireless/core.c +3 -2

index 9f08203..eb3e1de 100644
@@ -1123,6 +1123,7 @@ static int __init cfg80211_init(void)
 		goto out_fail_nl80211;
 
 	ieee80211_debugfs_dir = debugfs_create_dir("ieee80211", NULL);
+	ieee80211_debugfs_add_glbl(ieee80211_debugfs_dir);
 
 	err = regulatory_init();
 	if (err)

@@ -1137,7 +1138,7 @@ static int __init cfg80211_init(void)
 out_fail_wq:
 	regulatory_exit();
 out_fail_reg:
-	debugfs_remove(ieee80211_debugfs_dir);
+	debugfs_remove_recursive(ieee80211_debugfs_dir);
 out_fail_nl80211:
 	unregister_netdevice_notifier(&cfg80211_netdev_notifier);
 out_fail_notifier:

@@ -1151,7 +1152,7 @@ subsys_initcall(cfg80211_init);
 
 static void __exit cfg80211_exit(void)
 {
-	debugfs_remove(ieee80211_debugfs_dir);
+	debugfs_remove_recursive(ieee80211_debugfs_dir);
 	nl80211_exit();
 	unregister_netdevice_notifier(&cfg80211_netdev_notifier);
 	wiphy_sysfs_exit();

net/wireless/core.h +17 -0

index 71b7285..e75be56 100644
@@ -126,6 +126,23 @@ static inline void assert_cfg80211_lock(void)
 	lockdep_assert_held(&cfg80211_mutex);
 }
 
+#ifdef CONFIG_CFG80211_MEM_DEBUGGING
+
+struct wifi_mem_tracker {
+	struct list_head mylist;
+	char buf[40];
+	void *ptr;
+};
+extern struct list_head ies_list;
+extern spinlock_t ies_lock;
+extern atomic_t ies_count;
+
+extern struct list_head bss_list;
+extern spinlock_t bss_lock;
+extern atomic_t bss_count;
+
+#endif
+
 struct cfg80211_internal_bss {
 	struct list_head list;
 	struct list_head hidden_list;

net/wireless/debugfs.c +117 -0

133 lines not on this page · raw message

net/wireless/debugfs.h +2 -0

index 74fdd38..f644869 100644
@@ -3,9 +3,11 @@
 
 #ifdef CONFIG_CFG80211_DEBUGFS
 void cfg80211_debugfs_rdev_add(struct cfg80211_registered_device *rdev);
+void ieee80211_debugfs_add_glbl(struct dentry *dir);
 #else
 static inline
 void cfg80211_debugfs_rdev_add(struct cfg80211_registered_device *rdev) {}
+static inline void ieee80211_debugfs_add_glbl(struct dentry *dir) { }
 #endif
 
 #endif /* __CFG80211_DEBUGFS_H */

net/wireless/scan.c +112 ~~-15~~

188 lines not on this page · raw message

Signature · 1 line

1.7.3.4

[email protected]

author

2013-06-18 22:04 UTC #1 lore raw

PATCH 2/6 mac80211: Fix bss ref leak.

From: Ben Greear <[email protected]>

Some of the calls to ieee80211_destroy_assoc_data should be putting the bss reference and were not. Add boolean argument to tell that method whether or not it should put the reference and fix calling code appropriately.

Grab the bss reference where the pointer is assigned to make it easier to properly do reference counting.

Also add some comments to help clarify the bss ref-counting logic.

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

---
 net/mac80211/mlme.c |   33 ++++++++++++++++++++++++---------
 net/mac80211/scan.c |    6 ++++++
 2 files changed, 30 insertions(+), 9 deletions(-)

net/mac80211/mlme.c +24 -9

index 6510790..622cdd8 100644
@@ -2400,7 +2400,7 @@ static void ieee80211_get_rates(struct ieee80211_supported_band *sband,
 }
 
 static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
-					 bool assoc)
+					 bool assoc, bool put_bss)
 {
 	struct ieee80211_mgd_assoc_data *assoc_data = sdata->u.mgd.assoc_data;

@@ -2415,6 +2415,9 @@ static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
 		ieee80211_vif_release_channel(sdata);
 	}
 
+	if (put_bss)
+		cfg80211_put_bss(sdata->local->hw.wiphy, assoc_data->bss);
+
 	kfree(assoc_data);
 	sdata->u.mgd.assoc_data = NULL;
 }

@@ -2587,6 +2590,7 @@ static bool ieee80211_assoc_success(struct ieee80211_sub_if_data *sdata,
 	return true;
 }
 
+/** Calling code must dispose of bss reference if it is not NULL. */
 static enum rx_mgmt_action __must_check
 ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 			     struct ieee80211_mgmt *mgmt, size_t len,

@@ -2643,17 +2647,20 @@ ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 		return RX_MGMT_NONE;
 	}
 
+	/* bss will be passed back to calling code, and that code must
+	 * deal with properly putting the reference.
+	 */
 	*bss = assoc_data->bss;
 
 	if (status_code != WLAN_STATUS_SUCCESS) {
 		sdata_info(sdata, "%pM denied association (code=%d)\n",
 			   mgmt->sa, status_code);
-		ieee80211_destroy_assoc_data(sdata, false);
+		ieee80211_destroy_assoc_data(sdata, false, false);
 	} else {
 		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);
+			ieee80211_destroy_assoc_data(sdata, false, true);
+			*bss = NULL; /* Ensure no stale references */
 			return RX_MGMT_CFG80211_ASSOC_TIMEOUT;
 		}
 		sdata_info(sdata, "associated\n");

@@ -2663,7 +2670,7 @@ ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 		 * recalc after assoc_data is NULL but before associated
 		 * is set can cause the interface to go idle
 		 */
-		ieee80211_destroy_assoc_data(sdata, true);
+		ieee80211_destroy_assoc_data(sdata, true, false);
 	}
 
 	return RX_MGMT_CFG80211_RX_ASSOC;

@@ -3105,6 +3112,9 @@ void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
 		break;
 	case IEEE80211_STYPE_ASSOC_RESP:
 	case IEEE80211_STYPE_REASSOC_RESP:
+		/* One way or another, must eventually put bss reference
+		 * if it is not NULL.
+		 */
 		rma = ieee80211_rx_mgmt_assoc_resp(sdata, mgmt, skb->len, &bss);
 		break;
 	case IEEE80211_STYPE_ACTION:

@@ -3136,6 +3146,9 @@ void ieee80211_sta_rx_queued_mgmt(struct ieee80211_sub_if_data *sdata,
 		cfg80211_send_rx_assoc(sdata->dev, bss, (u8 *)mgmt, skb->len);
 		break;
 	case RX_MGMT_CFG80211_ASSOC_TIMEOUT:
+		/* bss reference is already put at this point, see
+		 * 'internal-error' comment in ieee80211_rx_mgmt_assoc_resp
+		 */
 		cfg80211_send_assoc_timeout(sdata->dev, mgmt->bssid);
 		break;
 	case RX_MGMT_CFG80211_TX_DEAUTH:

@@ -3385,7 +3398,7 @@ void ieee80211_sta_work(struct ieee80211_sub_if_data *sdata)
 
 			memcpy(bssid, ifmgd->assoc_data->bss->bssid, ETH_ALEN);
 
-			ieee80211_destroy_assoc_data(sdata, false);
+			ieee80211_destroy_assoc_data(sdata, false, true);
 
 			mutex_unlock(&ifmgd->mtx);
 			cfg80211_send_assoc_timeout(sdata->dev, bssid);

@@ -3935,6 +3948,7 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
 		return -ENOMEM;
 
 	auth_data->bss = req->bss;
+	cfg80211_ref_bss(local->hw.wiphy, auth_data->bss);
 
 	if (req->sae_data_len >= 4) {
 		__le16 *pos = (__le16 *) req->sae_data;

@@ -3998,8 +4012,6 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
 		goto err_clear;
 	}
 
-	/* hold our own reference */
-	cfg80211_ref_bss(local->hw.wiphy, auth_data->bss);
 	err = 0;
 	goto out_unlock;

@@ -4008,6 +4020,7 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
 	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BSSID);
 	ifmgd->auth_data = NULL;
  err_free:
+	cfg80211_put_bss(sdata->local->hw.wiphy, auth_data->bss);
 	kfree(auth_data);
  out_unlock:
 	mutex_unlock(&ifmgd->mtx);

@@ -4129,6 +4142,7 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 	}
 
 	assoc_data->bss = req->bss;
+	cfg80211_ref_bss(sdata->local->hw.wiphy, assoc_data->bss);
 
 	if (ifmgd->req_smps == IEEE80211_SMPS_AUTOMATIC) {
 		if (ifmgd->powersave)

@@ -4259,6 +4273,7 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
  err_clear:
 	memset(ifmgd->bssid, 0, ETH_ALEN);
 	ieee80211_bss_info_change_notify(sdata, BSS_CHANGED_BSSID);
+	cfg80211_put_bss(sdata->local->hw.wiphy, assoc_data->bss);
 	ifmgd->assoc_data = NULL;
  err_free:
 	kfree(assoc_data);

@@ -4364,7 +4379,7 @@ void ieee80211_mgd_stop(struct ieee80211_sub_if_data *sdata)
 
 	mutex_lock(&ifmgd->mtx);
 	if (ifmgd->assoc_data)
-		ieee80211_destroy_assoc_data(sdata, false);
+		ieee80211_destroy_assoc_data(sdata, false, true);
 	if (ifmgd->auth_data)
 		ieee80211_destroy_auth_data(sdata, false);
 	del_timer_sync(&ifmgd->timer);

net/mac80211/scan.c +6 -0

index 0b6434b..e0fcb4a 100644
@@ -55,6 +55,9 @@ static bool is_uapsd_supported(struct ieee802_11_elems *elems)
 	return qos_info & IEEE80211_WMM_IE_AP_QOSINFO_UAPSD;
 }
 
+/** Must (eventually) put the returned value to keep from leaking
+ * a reference to the bss.
+ */
 struct ieee80211_bss *
 ieee80211_bss_info_update(struct ieee80211_local *local,
 			  struct ieee80211_rx_status *rx_status,

@@ -73,6 +76,9 @@ ieee80211_bss_info_update(struct ieee80211_local *local,
 	else if (local->hw.flags & IEEE80211_HW_SIGNAL_UNSPEC)
 		signal = (rx_status->signal * 100) / local->hw.max_signal;
 
+	/* We now own a reference to cbss, have to make sure we
+	 * put it later.
+	 */
 	cbss = cfg80211_inform_bss_frame(local->hw.wiphy, channel,
 					 mgmt, len, signal, GFP_ATOMIC);
 	if (!cbss)

Signature · 1 line

1.7.3.4

Johannes Berg

maintainer

2013-06-19 10:49 UTC #2 lore raw

Re: [PATCH 2/6] mac80211: Fix bss ref leak.

I think we should do this differently and have mac80211 pass the BSS entry back to cfg80211_connect_result. That way, we can also hold it to make sure it doesn't expire while we're trying to connect, which is another corner case here ...

I'll take a look at this, but that'll be in mac80211-next.

johannes

Johannes Berg

maintainer

2013-06-19 10:51 UTC #3 lore raw

Re: [PATCH 2/6] mac80211: Fix bss ref leak.

Quoted 4 lines · Johannes Berg

I think we should do this differently and have mac80211 pass the BSS entry back to cfg80211_connect_result. That way, we can also hold it to make sure it doesn't expire while we're trying to connect, which is another corner case here ...

Oh, actually, mac80211 doesn't have this issue but all the other drivers do.

johannes

[email protected]

author

2013-06-18 22:04 UTC #4 lore raw

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

In reply to [email protected]

From: Ben Greear <[email protected]>

Otherwise, we can leak a bss reference.

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

---
 net/wireless/sme.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

net/wireless/sme.c +9 -2

index 6066720..ea2ce33 100644
@@ -420,6 +420,7 @@ void cfg80211_sme_failed_assoc(struct wireless_dev *wdev)
 	schedule_work(&rdev->conn_work);
 }
 
+/** 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,

@@ -435,11 +436,17 @@ 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)) {
+		if (bss)
+			cfg80211_put_bss(wdev->wiphy, bss);
 		return;
+	}
 
-	if (wdev->sme_state != CFG80211_SME_CONNECTING)
+	if (wdev->sme_state != CFG80211_SME_CONNECTING) {
+		if (bss)
+			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

[email protected]

author

2013-06-18 22:04 UTC #5 lore raw

PATCH 4/6 wireless: Check for dangling wdev->current_bss pointer.

In reply to [email protected]

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]>

---
 net/wireless/core.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

net/wireless/core.c +6 -0

index eb3e1de..9e05da9 100644
@@ -1063,6 +1063,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);
+			SET_BSS(wdev, NULL);
+		}
 		break;
 	case NETDEV_PRE_UP:
 		if (!(wdev->wiphy->interface_modes & BIT(wdev->iftype)))

Signature · 1 line

1.7.3.4

[email protected]

author

2013-06-18 22:04 UTC #6 lore raw

PATCH 5/6 wireless: Fix bss ref count leak in __cfg80211_mlme_assoc

In reply to [email protected]

From: Ben Greear <[email protected]>

The rdev_assoc path holds it's own reference, so the mlme_assoc must release the reference it took earlier before returning.

This actually appears to be the leak I have been seeing in my tests.

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

---
 net/wireless/mlme.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

net/wireless/mlme.c +1 -1

index d975510..1c60268 100644
@@ -432,8 +432,8 @@ out:
 	if (err) {
 		if (was_connected)
 			SET_SME_STATE(wdev, CFG80211_SME_CONNECTED);
-		cfg80211_put_bss(&rdev->wiphy, req.bss);
 	}
+	cfg80211_put_bss(&rdev->wiphy, req.bss);
 
 	return err;
 }

Signature · 1 line

1.7.3.4

[email protected]

author

2013-06-18 22:04 UTC #7 lore raw

PATCH 6/6 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]>

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

net/wireless/scan.c +4 -0

index 542ff6d..834d2f9 100644
@@ -622,6 +622,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,

@@ -777,6 +778,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)

@@ -962,6 +964,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,

@@ -1019,6 +1022,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 ea2ce33..d26cd68 100644
@@ -263,6 +263,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);

@@ -579,6 +580,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,

@@ -662,6 +664,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

Julian Calaby

2013-06-19 02:52 UTC #8 lore raw

Re: [PATCH 6/6] wireless: Add comments about bss refcounting.

Hi Ben,

Quoted 16 lines · On Wed, Jun 19, 2013 at 8:03 AM, <[email protected]> wrote:

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]>
---
 net/wireless/scan.c |    4 ++++
 net/wireless/sme.c  |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)
net/wireless/scan.c +0 -0
index 542ff6d..834d2f9 100644

net/wireless/scan.c static int cmp_bss(struct cfg80211_bss *a, · line 625

Quoted 4 lines

net/wireless/scan.c +1 -0

@@ -622,6 +622,7 @@ static int cmp_bss(struct cfg80211_bss *a,
        }
 }
 
+/** Returned bss is reference counted and must be cleaned up appropriately. */

/** is for kernel doc, I'm not sure that is valid kernel doc.

Thanks,

--
Julian Calaby

Email: [email protected]
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Johannes Berg

maintainer

2013-06-19 09:49 UTC #9 lore raw

In reply to [email protected]

Quoted 6 lines · On Tue, 2013-06-18 at 15:03 -0700, [email protected] wrote:

From: Ben Greear <[email protected]>
The bss objects are reference counted, and the ies are also tricky to keep track of. Add option to track allocation and freeing of the ies and bss objects, and add debugfs files to show the current objects.

I'm not really sure this is worth it -- you found the bug with kmemleak() after all. Having very specific things for all of this doesn't really seem worth it.

Either way though, I can't apply the patches since they won't apply. I realize that you're working on 3.9, but you're going to have to send me patches I can apply to one of my trees (and right now, not really mac80211 but mac80211-next)

johannes

Ben Greear

author

2013-06-19 14:22 UTC #10 lore raw

Quoted 11 lines · Johannes Berg

Quoted 6 lines · On Tue, 2013-06-18 at 15:03 -0700, [email protected] wrote:
From: Ben Greear <[email protected]>
The bss objects are reference counted, and the ies are also tricky to keep track of. Add option to track allocation and freeing of the ies and bss objects, and add debugfs files to show the current objects.
I'm not really sure this is worth it -- you found the bug with kmemleak() after all. Having very specific things for all of this doesn't really seem worth it.

kmemleak didn't show the bss leaking though, so I spent several days thinking ies pointers were being corrupted somehow. I think the code is not that intrusive, and since drivers also take bss refs, the problems could come back in the future.

I didn't actually audit the drivers since I am not using that hardware, by the way.

Quoted 4 lines

Either way though, I can't apply the patches since they won't apply. I realize that you're working on 3.9, but you're going to have to send me patches I can apply to one of my trees (and right now, not really mac80211 but mac80211-next)

I started porting some patches to wireless-next yesterday..think that would be a good enough tree to patch against?

Thanks,
Ben

Quoted 3 lines

johannes

Signature · 2 lines

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

Johannes Berg

maintainer

2013-06-19 14:27 UTC #11 lore raw

Quoted 8 lines · Ben Greear

Quoted 3 lines
I'm not really sure this is worth it -- you found the bug with kmemleak() after all. Having very specific things for all of this doesn't really seem worth it.
kmemleak didn't show the bss leaking though, so I spent several days thinking ies pointers were being corrupted somehow. I think the code is not that intrusive, and since drivers also take bss refs, the problems could come back in the future.

Fair enough, I'll think about it some more. I don't think drivers are really that much of a problem though.

Quoted 2 lines

I didn't actually audit the drivers since I am not using that hardware, by the way.

Right.

Quoted 2 lines

I started porting some patches to wireless-next yesterday..think that would be a good enough tree to patch against?

Probably OK, I only have few patches right now. I also just posted two patches to address parts of this problem though, can you review those too?

johannes

Kalle Valo

2013-06-22 08:43 UTC #12 lore raw

In reply to [email protected]

Quoted 8 lines · [email protected] writes:

From: Ben Greear <[email protected]>
The bss objects are reference counted, and the ies are also tricky to keep track of. Add option to track allocation and freeing of the ies and bss objects, and add debugfs files to show the current objects.
Signed-off-by Ben Greear <[email protected]>

[...]

net/wireless/Kconfig config CFG80211_DEBUGFS · line 129

Quoted 12 lines

net/wireless/Kconfig +12 -0

+config CFG80211_MEM_DEBUGGING
+	bool "cfg80211 memory debugging logic"
+	default n
+	depends on CFG80211_DEBUGFS
+	---help---
+	  Enable this if you want to debug memory handling for bss and ies
+	  objects.  New debugfs files: ieee80211/all_ies and all_bss will
+	  be created to display these objects.  This has a moderate CPU cost
+	  and uses a bit more memory than normal, but otherwise is not very
+	  expensive.
+
+	  If unsure, say N.

IMHO having a new Kconfig option is overkill. Can we use something more generic, like CFG80211_DEVELOPER_WARNINGS, for this?

Signature · 1 line

Kalle Valo

Ben Greear

author

2013-06-22 15:24 UTC #13 lore raw

Quoted 13 lines · Kalle Valo

Quoted 8 lines · [email protected] writes:
From: Ben Greear <[email protected]>
The bss objects are reference counted, and the ies are also tricky to keep track of. Add option to track allocation and freeing of the ies and bss objects, and add debugfs files to show the current objects.
Signed-off-by Ben Greear <[email protected]>
[...]

Quoted 15 lines

net/wireless/Kconfig config CFG80211_DEBUGFS · line 129
Quoted 12 lines
net/wireless/Kconfig +12 -0
+config CFG80211_MEM_DEBUGGING
+	bool "cfg80211 memory debugging logic"
+	default n
+	depends on CFG80211_DEBUGFS
+	---help---
+	  Enable this if you want to debug memory handling for bss and ies
+	  objects.  New debugfs files: ieee80211/all_ies and all_bss will
+	  be created to display these objects.  This has a moderate CPU cost
+	  and uses a bit more memory than normal, but otherwise is not very
+	  expensive.
+
+	  If unsure, say N.
IMHO having a new Kconfig option is overkill. Can we use something more generic, like CFG80211_DEVELOPER_WARNINGS, for this?

That would be fine with me. It does cause some extra list walkings and memory usage, so I wasn't sure it should go in by default.

Unless we are actually leaking things, the lists should remain relatively short, and probably not walked all *that* often, so maybe it would be OK....

Thanks,
Ben

Signature · 2 lines

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