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(-)
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..542ff6d 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);
+
+
+static void my_kfree_rcu_ies(struct cfg80211_bss_ies *ies)
+{
+ struct wifi_mem_tracker *iesm;
+ spin_lock_bh(&ies_lock);
+ list_for_each_entry(iesm, &ies_list, mylist) {
+ if (iesm->ptr == ies) {
+ list_del(&iesm->mylist);
+ kfree(iesm);
+ break;
+ }
+ }
+ spin_unlock_bh(&ies_lock);
+ atomic_sub(1, &ies_count);
+ kfree_rcu(ies, rcu_head);
+}
+
+#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;
+}
+
+static void my_kfree_bss(struct cfg80211_internal_bss *bss)
+{
+ struct wifi_mem_tracker *bssm;
+ spin_lock_bh(&bss_lock);
+ list_for_each_entry(bssm, &bss_list, mylist) {
+ if (bssm->ptr == bss) {
+ list_del(&bssm->mylist);
+ kfree(bssm);
+ break;
+ }
+ }
+ atomic_sub(1, &bss_count);
+ spin_unlock_bh(&bss_lock);
+ kfree(bss);
+}
+
+#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(ies) kfree_rcu(ies, rcu_head)
+#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);
ies = (void *)rcu_access_pointer(bss->pub.proberesp_ies);
if (ies)
- kfree_rcu(ies, rcu_head);
+ my_kfree_rcu_ies(ies);
/*
* 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,7 @@ 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);
} else if (rcu_access_pointer(tmp->pub.beacon_ies)) {
const struct cfg80211_bss_ies *old;
struct cfg80211_internal_bss *bss;
@@ -731,8 +830,7 @@ 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);
goto drop;
}
@@ -759,8 +857,7 @@ 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);
}
found->pub.beacon_interval = tmp->pub.beacon_interval;
@@ -777,15 +874,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);
ies = (void *)rcu_dereference(tmp->pub.proberesp_ies);
if (ies)
- kfree_rcu(ies, rcu_head);
+ my_kfree_rcu_ies(ies);
goto drop;
}
memcpy(new, tmp, sizeof(*new));
@@ -899,7 +996,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 +1053,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
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(-)
diff --git a/net/wireless/core.c b/net/wireless/core.c
index eb3e1de..9e05da9 100644
--- a/net/wireless/core.c
+++ b/net/wireless/core.c
@@ -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)))
--
1.7.3.4
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(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 6510790..622cdd8 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,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);
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
On Wed, 2013-06-19 at 12:49 +0200, Johannes Berg wrote:
> 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
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
On 06/19/2013 02:49 AM, Johannes Berg wrote:
> 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.
> 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
>
> johannes
>
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
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(-)
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 542ff6d..834d2f9 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -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,
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index ea2ce33..d26cd68 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -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,
--
1.7.3.4
On 06/22/2013 01:43 AM, Kalle Valo wrote:
> [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]>
>
> [...]
>
>> +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
--
Ben Greear <[email protected]>
Candela Technologies Inc http://www.candelatech.com
On Wed, 2013-06-19 at 07:22 -0700, Ben Greear wrote:
> > 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.
> I didn't actually audit the drivers since I am not using that hardware,
> by the way.
Right.
> 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
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(-)
diff --git a/net/wireless/mlme.c b/net/wireless/mlme.c
index d975510..1c60268 100644
--- a/net/wireless/mlme.c
+++ b/net/wireless/mlme.c
@@ -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;
}
--
1.7.3.4
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(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 6066720..ea2ce33 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -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,
--
1.7.3.4
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
Hi Ben,
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(-)
>
> diff --git a/net/wireless/scan.c b/net/wireless/scan.c
> index 542ff6d..834d2f9 100644
> --- a/net/wireless/scan.c
> +++ b/net/wireless/scan.c
> @@ -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/
[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]>
[...]
> +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?
--
Kalle Valo