[RFC 1/2] wireless: Add memory usage debugging.

[RFC 1/2] wireless: Add memory usage debugging.

[greearb]

From: Ben Greear <greearb@candelatech.com>

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 <greearb@candelatech.com>
---
 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    |  130 ++++++++++++++++++++++++++++++++++++++++++------
 6 files changed, 267 insertions(+), 17 deletions(-)

diff --git a/net/wireless/Kconfig b/net/wireless/Kconfig
index 16d08b3..43ec2cd 100644
--- a/net/wireless/Kconfig
+++ b/net/wireless/Kconfig
@@ -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
diff --git a/net/wireless/core.c b/net/wireless/core.c
index 9f08203..eb3e1de 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -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();
diff --git a/net/wireless/core.h b/net/wireless/core.h
index 71b7285..e75be56 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -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;
diff --git a/net/wireless/debugfs.c b/net/wireless/debugfs.c
index 920cabe..96dc757 100644
--- a/net/wireless/debugfs.c
+++ b/net/wireless/debugfs.c
@@ -31,6 +31,110 @@ static const struct file_operations name## _ops = {			\
 	.llseek = generic_file_llseek,					\
 };
 
+#define DEBUGFS_READONLY_FILE_OPS(name) \
+static const struct file_operations name## _ops = {			\
+	.read = name## _read,						\
+	.open = simple_open,						\
+	.llseek = generic_file_llseek,					\
+};
+
+#ifdef CONFIG_CFG80211_MEM_DEBUGGING
+static ssize_t all_ies_read(struct file *file, char __user *user_buf,
+				size_t count, loff_t *ppos)
+{
+	int mxln = 31500;
+	char *buf = kzalloc(mxln, GFP_KERNEL);
+	int q, res = 0;
+	struct wifi_mem_tracker *iesm;
+
+	if (!buf)
+		return 0;
+
+	spin_lock_bh(&ies_lock);
+	res += sprintf(buf + res, "Total: %i\n", atomic_read(&ies_count));
+	list_for_each_entry(iesm, &ies_list, mylist) {
+		res += sprintf(buf + res, "%p: %s\n",
+			       iesm->ptr, iesm->buf);
+		if (res >= mxln) {
+			res = mxln;
+			break;
+		}
+	}
+	spin_unlock_bh(&ies_lock);
+
+	q = simple_read_from_buffer(user_buf, count, ppos, buf, res);
+	kfree(buf);
+	return q;
+}
+
+static ssize_t all_bss_read(struct file *file, char __user *user_buf,
+			    size_t count, loff_t *ppos)
+{
+	int mxln = 31500;
+	char *buf = kzalloc(mxln, GFP_KERNEL);
+	int q, res = 0;
+	struct wifi_mem_tracker *bssm;
+
+	if (!buf)
+		return 0;
+
+	spin_lock_bh(&bss_lock);
+	res += sprintf(buf + res, "Total: %i\n", atomic_read(&bss_count));
+	list_for_each_entry(bssm, &bss_list, mylist) {
+		struct cfg80211_internal_bss *bss;
+		bss = (struct cfg80211_internal_bss *)(bssm->ptr);
+		res += sprintf(buf + res, "%p: #%lu %s\n",
+			       bssm->ptr, bss->refcount, bssm->buf);
+		if (res >= mxln) {
+			res = mxln;
+			break;
+		}
+	}
+	spin_unlock_bh(&bss_lock);
+
+	q = simple_read_from_buffer(user_buf, count, ppos, buf, res);
+	kfree(buf);
+	return q;
+}
+
+DEBUGFS_READONLY_FILE_OPS(all_ies);
+DEBUGFS_READONLY_FILE_OPS(all_bss);
+
+#endif
+
+static ssize_t bss_read(struct file *file, char __user *user_buf,
+			size_t count, loff_t *ppos)
+{
+	struct wiphy *wiphy = file->private_data;
+	struct cfg80211_registered_device *dev = wiphy_to_dev(wiphy);
+	int mxln = 31500;
+	char *buf = kzalloc(mxln, GFP_KERNEL);
+	int q, res = 0;
+	struct cfg80211_internal_bss *bss;
+
+	if (!buf)
+		return 0;
+
+	spin_lock_bh(&dev->bss_lock);
+	list_for_each_entry(bss, &dev->bss_list, list) {
+		res += sprintf(buf + res,
+			       "%p: #%lu  bcn: %p  pr: %p  hidden: %p\n",
+			       bss, bss->refcount,
+			       rcu_access_pointer(bss->pub.beacon_ies),
+			       rcu_access_pointer(bss->pub.proberesp_ies),
+			       bss->pub.hidden_beacon_bss);
+		if (res >= mxln) {
+			res = mxln;
+			break;
+		}
+	}
+	spin_unlock_bh(&dev->bss_lock);
+
+	q = simple_read_from_buffer(user_buf, count, ppos, buf, res);
+	kfree(buf);
+	return q;
+}
+
 DEBUGFS_READONLY_FILE(rts_threshold, 20, "%d",
 		      wiphy->rts_threshold)
 DEBUGFS_READONLY_FILE(fragmentation_threshold, 20, "%d",
@@ -39,6 +143,7 @@ DEBUGFS_READONLY_FILE(short_retry_limit, 20, "%d",
 		      wiphy->retry_short)
 DEBUGFS_READONLY_FILE(long_retry_limit, 20, "%d",
 		      wiphy->retry_long);
+DEBUGFS_READONLY_FILE_OPS(bss);
 
 static int ht_print_chan(struct ieee80211_channel *chan,
 			 char *buf, int buf_size, int offset)
@@ -112,4 +217,16 @@ void cfg80211_debugfs_rdev_add(struct cfg80211_registered_device *rdev)
 	DEBUGFS_ADD(short_retry_limit);
 	DEBUGFS_ADD(long_retry_limit);
 	DEBUGFS_ADD(ht40allow_map);
+	DEBUGFS_ADD(bss);
+}
+
+#define DEBUGFS_ADD_GLBL(name)						\
+	debugfs_create_file(#name, S_IRUGO, dir, NULL, &name## _ops);
+
+void ieee80211_debugfs_add_glbl(struct dentry *dir)
+{
+#ifdef CONFIG_CFG80211_MEM_DEBUGGING
+	DEBUGFS_ADD_GLBL(all_ies);
+	DEBUGFS_ADD_GLBL(all_bss);
+#endif
 }
diff --git a/net/wireless/debugfs.h b/net/wireless/debugfs.h
index 74fdd38..f644869 100644
--- a/net/wireless/debugfs.h
+++ b/net/wireless/debugfs.h
@@ -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 */
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index fd99ea4..64cfc1b 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -57,6 +57,106 @@
 
 #define IEEE80211_SCAN_RESULT_EXPIRE	(30 * HZ)
 
+#ifdef CONFIG_CFG80211_MEM_DEBUGGING
+
+LIST_HEAD(ies_list);
+DEFINE_SPINLOCK(ies_lock);
+atomic_t ies_count = ATOMIC_INIT(0);
+
+LIST_HEAD(bss_list);
+DEFINE_SPINLOCK(bss_lock);
+atomic_t bss_count = ATOMIC_INIT(0);
+
+
+#define my_kfree_rcu_ies(a, b, bss)					\
+	do {								\
+		struct wifi_mem_tracker *iesm;				\
+		spin_lock_bh(&ies_lock);				\
+		list_for_each_entry(iesm, &ies_list, mylist) {		\
+			if (iesm->ptr == a) {				\
+				list_del(&iesm->mylist);		\
+				kfree(iesm);				\
+				break;					\
+			}						\
+		}							\
+		spin_unlock_bh(&ies_lock);				\
+		atomic_sub(1, &ies_count);				\
+		kfree_rcu(a, b);					\
+	} while (0)
+
+#define my_kmalloc_ies(s, g)				\
+	_my_kmalloc_ies(s, g, __LINE__);
+
+static void* _my_kmalloc_ies(size_t s, gfp_t gfp, int l)
+{
+	void *rv = kmalloc(s, gfp);
+	if (rv) {
+		struct wifi_mem_tracker *iesm = kmalloc(sizeof(*iesm), gfp);
+		atomic_add(1, &ies_count);
+		if (iesm) {
+			snprintf(iesm->buf, sizeof(iesm->buf), "%i", l);
+			iesm->buf[sizeof(iesm->buf)-1] = 0;
+			iesm->ptr = rv;
+			INIT_LIST_HEAD(&iesm->mylist);
+			spin_lock_bh(&ies_lock);
+			list_add(&iesm->mylist, &ies_list);
+			spin_unlock_bh(&ies_lock);
+		} else {
+			pr_err("ERROR:  Could not allocate iesm.\n");
+		}
+	}
+	return rv;
+}
+
+#define my_kfree_bss(a)					\
+	do {								\
+		struct wifi_mem_tracker *iesm;				\
+		spin_lock_bh(&bss_lock);				\
+		list_for_each_entry(iesm, &bss_list, mylist) {		\
+			if (iesm->ptr == a) {				\
+				list_del(&iesm->mylist);		\
+				kfree(iesm);				\
+				break;					\
+			}						\
+		}							\
+		atomic_sub(1, &bss_count);				\
+		spin_unlock_bh(&bss_lock);				\
+		kfree(a);						\
+	} while (0)
+
+#define my_kzalloc_bss(s, g)				\
+	_my_kzalloc_bss(s, g, __LINE__);
+
+static void* _my_kzalloc_bss(size_t s, gfp_t gfp, int l)
+{
+	void *rv = kmalloc(s, gfp);
+	if (rv) {
+		struct wifi_mem_tracker *bssm = kmalloc(sizeof(*bssm), gfp);
+		atomic_add(1, &bss_count);
+		if (bssm) {
+			snprintf(bssm->buf, sizeof(bssm->buf), "%i", l);
+			bssm->buf[sizeof(bssm->buf)-1] = 0;
+			bssm->ptr = rv;
+			INIT_LIST_HEAD(&bssm->mylist);
+			spin_lock_bh(&bss_lock);
+			list_add(&bssm->mylist, &bss_list);
+			spin_unlock_bh(&bss_lock);
+		} else {
+			pr_err("ERROR:  Could not allocate bssm for bss.\n");
+		}
+	}
+	return rv;
+}
+
+#else
+
+#define my_kfree_rcu_ies(a, b, bss) kfree_rcu(a, b)
+#define my_kmalloc_ies(s, g) kmalloc(s, g)
+#define my_kfree_bss(a) kfree(a)
+#define my_kzalloc_bss(s, g) kzalloc(s, g)
+
+#endif
+
 static void bss_free(struct cfg80211_internal_bss *bss)
 {
 	struct cfg80211_bss_ies *ies;
@@ -66,10 +166,10 @@ static void bss_free(struct cfg80211_internal_bss *bss)
 
 	ies = (void *)rcu_access_pointer(bss->pub.beacon_ies);
 	if (ies && !bss->pub.hidden_beacon_bss)
-		kfree_rcu(ies, rcu_head);
+		my_kfree_rcu_ies(ies, rcu_head, bss);
 	ies = (void *)rcu_access_pointer(bss->pub.proberesp_ies);
 	if (ies)
-		kfree_rcu(ies, rcu_head);
+		my_kfree_rcu_ies(ies, rcu_head, bss);
 
 	/*
 	 * This happens when the module is removed, it doesn't
@@ -78,7 +178,7 @@ static void bss_free(struct cfg80211_internal_bss *bss)
 	if (!list_empty(&bss->hidden_list))
 		list_del(&bss->hidden_list);
 
-	kfree(bss);
+	my_kfree_bss(bss);
 }
 
 static inline void bss_ref_get(struct cfg80211_registered_device *dev,
@@ -710,8 +810,8 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 			rcu_assign_pointer(found->pub.ies,
 					   tmp->pub.proberesp_ies);
 			if (old)
-				kfree_rcu((struct cfg80211_bss_ies *)old,
-					  rcu_head);
+				my_kfree_rcu_ies((struct cfg80211_bss_ies *)old,
+						 rcu_head, found);
 		} else if (rcu_access_pointer(tmp->pub.beacon_ies)) {
 			const struct cfg80211_bss_ies *old;
 			struct cfg80211_internal_bss *bss;
@@ -731,8 +831,8 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 				 */
 
 				f = rcu_access_pointer(tmp->pub.beacon_ies);
-				kfree_rcu((struct cfg80211_bss_ies *)f,
-					  rcu_head);
+				my_kfree_rcu_ies((struct cfg80211_bss_ies *)f,
+						 rcu_head, NULL);
 				goto drop;
 			}
 
@@ -759,8 +859,8 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 			}
 
 			if (old)
-				kfree_rcu((struct cfg80211_bss_ies *)old,
-					  rcu_head);
+				my_kfree_rcu_ies((struct cfg80211_bss_ies *)old,
+						 rcu_head, found);
 		}
 
 		found->pub.beacon_interval = tmp->pub.beacon_interval;
@@ -777,15 +877,15 @@ cfg80211_bss_update(struct cfg80211_registered_device *dev,
 		 * is allocated on the stack since it's not needed in the
 		 * more common case of an update
 		 */
-		new = kzalloc(sizeof(*new) + dev->wiphy.bss_priv_size,
-			      GFP_ATOMIC);
+		new = my_kzalloc_bss(sizeof(*new) + dev->wiphy.bss_priv_size,
+				     GFP_ATOMIC);
 		if (!new) {
 			ies = (void *)rcu_dereference(tmp->pub.beacon_ies);
 			if (ies)
-				kfree_rcu(ies, rcu_head);
+				my_kfree_rcu_ies(ies, rcu_head, NULL);
 			ies = (void *)rcu_dereference(tmp->pub.proberesp_ies);
 			if (ies)
-				kfree_rcu(ies, rcu_head);
+				my_kfree_rcu_ies(ies, rcu_head, NULL);
 			goto drop;
 		}
 		memcpy(new, tmp, sizeof(*new));
@@ -899,7 +999,7 @@ cfg80211_inform_bss(struct wiphy *wiphy,
 	 * override the IEs pointer should we have received an earlier
 	 * indication of Probe Response data.
 	 */
-	ies = kmalloc(sizeof(*ies) + ielen, gfp);
+	ies = my_kmalloc_ies(sizeof(*ies) + ielen, gfp);
 	if (!ies)
 		return NULL;
 	ies->len = ielen;
@@ -956,7 +1056,7 @@ cfg80211_inform_bss_frame(struct wiphy *wiphy,
 	if (!channel)
 		return NULL;
 
-	ies = kmalloc(sizeof(*ies) + ielen, gfp);
+	ies = my_kmalloc_ies(sizeof(*ies) + ielen, gfp);
 	if (!ies)
 		return NULL;
 	ies->len = ielen;
-- 
1.7.3.4

[RFC 2/2] mac80211: Fix bss ref leak.

[greearb]

From: Ben Greear <greearb@candelatech.com>

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 <greearb@candelatech.com>
---
 net/mac80211/mlme.c |   34 +++++++++++++++++++++++++---------
 net/mac80211/scan.c |    6 ++++++
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 6510790..732eda0 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -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,10 @@ 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 +2591,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 +2648,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 +2671,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 +3113,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 +3147,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 +3399,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 +3949,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 +4013,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 +4021,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 +4143,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 +4274,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 +4380,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);
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 0b6434b..e0fcb4a 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -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)
-- 
1.7.3.4

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

[Johannes Berg]

On Mon, 2013-06-17 at 14:32 -0700, greearb@candelatech.com wrote:

>  static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
> -					 bool assoc)
> +					 bool assoc, bool put_bss)

Do we _really_ need another argument? Shouldn't it always be put in the
non-assoc case anyway, at least if non-NULL?

> @@ -2415,6 +2415,10 @@ 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);
> +
> +

just one blank line

> +	/** bss will be passed back to calling code, and that code must
> +	 * deal with properly putting the reference.
> +	 */

/** is for kernel-doc only

>  	return RX_MGMT_CFG80211_RX_ASSOC;

You're working on some pretty old code here ... If you want this to be
in stable the patch really needs to be smaller, I think. And for
mac80211-next this can't apply.

johannes

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

[Ben Greear]

On 06/18/2013 08:26 AM, Johannes Berg wrote:
> On Mon, 2013-06-17 at 14:32 -0700, greearb@candelatech.com wrote:
>
>>   static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
>> -					 bool assoc)
>> +					 bool assoc, bool put_bss)
>
> Do we _really_ need another argument? Shouldn't it always be put in the
> non-assoc case anyway, at least if non-NULL?

I can check if that is the case...

>
>> @@ -2415,6 +2415,10 @@ 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);
>> +
>> +
>
> just one blank line
>
>> +	/** bss will be passed back to calling code, and that code must
>> +	 * deal with properly putting the reference.
>> +	 */
>
> /** is for kernel-doc only
>
>>   	return RX_MGMT_CFG80211_RX_ASSOC;
>
> You're working on some pretty old code here ... If you want this to be
> in stable the patch really needs to be smaller, I think. And for
> mac80211-next this can't apply.

I'm working on 3.9.6.  When I get the problems fixed here, I can help
port this to whatever is the upstream kernel.

I'm not certain this is worth bothering with for stable anyway.
It seems the leaks are not too bad in general use cases,
and piddling around with code this tricky code could
introduce worse bugs that we may not immediately notice.

Thanks,
Ben

>
> johannes
>


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

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

[Ben Greear]

On 06/18/2013 08:26 AM, Johannes Berg wrote:
> On Mon, 2013-06-17 at 14:32 -0700, greearb@candelatech.com wrote:
>
>>   static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
>> -					 bool assoc)
>> +					 bool assoc, bool put_bss)
>
> Do we _really_ need another argument? Shouldn't it always be put in the
> non-assoc case anyway, at least if non-NULL?

I don't think so.  Check out the ieee80211_rx_mgmt_assoc_resp method.

  	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, false);

This passes in false as 'assoc', but we should not free bss here because
it is being passed back to the calling method, and the return
code of RX_MGMT_CFG80211_RX_ASSOC means bss should eventually
be consumed by the cfg80211 logic.

Of course, this is all 'as far as I can tell'.

I sort of like the second boolean because it forces the caller to
think about whether bss should be freed or not...

Thanks,
Ben


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

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

[Johannes Berg]

On Tue, 2013-06-18 at 09:33 -0700, Ben Greear wrote:
> On 06/18/2013 08:26 AM, Johannes Berg wrote:
> > On Mon, 2013-06-17 at 14:32 -0700, greearb@candelatech.com wrote:
> >
> >>   static void ieee80211_destroy_assoc_data(struct ieee80211_sub_if_data *sdata,
> >> -					 bool assoc)
> >> +					 bool assoc, bool put_bss)
> >
> > Do we _really_ need another argument? Shouldn't it always be put in the
> > non-assoc case anyway, at least if non-NULL?
> 
> I don't think so.  Check out the ieee80211_rx_mgmt_assoc_resp method.
> 
>   	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, false);
> 
> This passes in false as 'assoc', but we should not free bss here because
> it is being passed back to the calling method, and the return
> code of RX_MGMT_CFG80211_RX_ASSOC means bss should eventually
> be consumed by the cfg80211 logic.
> 
> Of course, this is all 'as far as I can tell'.
> 
> I sort of like the second boolean because it forces the caller to
> think about whether bss should be freed or not...

I just posted a patch to hand it back and never free it in mac80211 at
all, that also allows cfg80211_hold_bss() it across the assoc process
which fixes a (possibly more theoretical) expiry issue.

johannes