* [PATCH 6/6] wireless: Add comments about bss refcounting.
From: greearb @ 2013-06-18 22:03 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
In-Reply-To: <1371593017-10985-1-git-send-email-greearb@candelatech.com>
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>
---
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
^ permalink raw reply related
* [PATCH 2/6] mac80211: Fix bss ref leak.
From: greearb @ 2013-06-18 22:03 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
In-Reply-To: <1371593017-10985-1-git-send-email-greearb@candelatech.com>
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 | 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
^ permalink raw reply related
* [PATCH 1/6] wireless: Add memory usage debugging.
From: greearb @ 2013-06-18 22:03 UTC (permalink / raw)
To: linux-wireless; +Cc: Ben Greear
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 | 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
^ permalink raw reply related
* Re: Lots of confusion on bss refcounting.
From: Ben Greear @ 2013-06-18 21:36 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <1371570723.22256.0.camel@jlt4.sipsolutions.net>
On 06/18/2013 08:52 AM, Johannes Berg wrote:
>
>>> You mean ->current_bss? That should be handled in all the callbacks in
>>> sme.c or so
>>
>> Looks like much of the action happens on work-queues. I'm wondering if
>> we managed to delete wdev objects before we have completely cleaned up
>> in some cases...
>
> Don't we flush work structs appropriately?
I'm still seeing leaks after all those patches I posted...so I'm still
looking. The leaked bss objects have a quite large refcount, in the
hundreds after an hour or so of running, so this is probably more than
a strange race somewhere.
This code looks questionable in wireless/mlme.c
int __cfg80211_mlme_assoc()
.....
It grabs a reference using cfg80211_get_bss, but it only
does a put on the reference if there was an error code.
The __cfg80211_mlme_auth a bit above always does a put on
the reference.
I'm thinking mlme_assoc should also always do put. Any
reason you can think of otherwise?
Thanks,
Ben
>
> johannes
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: pull request: TI wireless drivers 2013-06-18 (for 3.11)
From: John W. Linville @ 2013-06-18 18:45 UTC (permalink / raw)
To: Luciano Coelho; +Cc: linux-wireless
In-Reply-To: <1371552937.10702.15.camel@cumari.coelho.fi>
On Tue, Jun 18, 2013 at 01:55:37PM +0300, Luciano Coelho wrote:
> Hi John,
>
> Here are a few patches intended for 3.11. There's nothing major. A
> bunch of bug fixes here and there, a hardware bug workaround and some
> small clean-ups.
>
> Please let me know if you have any problems with it!
>
> The following changes since commit 1105a13bb8ad29cf83d46989ee462d196038be87:
>
> orinoco_usb: fix memory leak in ezusb_access_ltv() when device disconnected (2013-06-14 13:37:16 -0400)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/luca/wl12xx.git for-linville
>
> for you to fetch changes up to 789e90e567d7d93edc1b2001d17209c21cc37850:
>
> net: wireless: wl1251: Fix commenting style (2013-06-17 12:24:09 +0300)
Pulling now...
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* Re: pull-request: iwlwifi-fixes 2013-06-14
From: John W. Linville @ 2013-06-18 18:44 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1371206567.8278.3.camel@jlt4.sipsolutions.net>
On Fri, Jun 14, 2013 at 12:42:47PM +0200, Johannes Berg wrote:
> John,
>
> I have two more fixes that I think would be worthwhile for 3.10 but
> admittedly the scenario is somewhat unlikely, so if you want to hold
> them for 3.11 I can live with that. In that case, I can put them into my
> -next tree, or you can pull this into -next (but if you don't pull
> wireless.git first you'd get some more fixes you already have.)
>
> These two patches fix two issues with using rfkill randomly during
> traffic, which would then cause our driver to stop working and not be
> able to recover at all.
>
> johannes
>
>
> The following changes since commit 622ebe994f6866b8d46ee5d3bcc329ed65d3722d:
>
> iwlwifi: fix rate control regression (2013-06-12 14:13:39 +0200)
>
> are available in the git repository at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/iwlwifi/iwlwifi-fixes.git for-john
>
> for you to fetch changes up to 8a487b1a7432b20ff3f82387a8ce7555a964b44e:
>
> iwlwifi: pcie: wake the queue if stopped when being unmapped (2013-06-13 16:44:04 +0200)
Pulling now (to wireless-next)...
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply
* [PATCH V2] rtlwifi: rtl8192cu: Fix duplicate if test
From: Larry Finger @ 2013-06-18 18:25 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, Larry Finger, netdev, stable
A typo causes routine rtl92cu_phy_rf6052_set_cck_txpower() to test the
same condition twice. The problem was found using cppcheck-1.49, and the
proper fix was verified against the pre-mac80211 version of the code.
This patch was originally included as commit 1288aa4, but was accidentally
reverted in a later patch.
Reported-by: David Binderman <dcb314@hotmail.com> [original report]
Reported-by: Andrea Morello <andrea.merello@gmail.com> [report of accidental reversion]
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: stable <stable@vger.kernel.org> [back to 2.6.39]
---
John,
Although this patch is trivial and could be included in the 3.10 stream,
I think we should not burden DaveM nor Linus with it this late in the 3.10
cycle. Please push it into the 3.11 stream.
Thanks,
Larry
---
V2 - Fix error in stable mailing address
---
drivers/net/wireless/rtlwifi/rtl8192cu/rf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/rtlwifi/rtl8192cu/rf.c b/drivers/net/wireless/rtlwifi/rtl8192cu/rf.c
index 953f1a0..2119313 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192cu/rf.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192cu/rf.c
@@ -104,7 +104,7 @@ void rtl92cu_phy_rf6052_set_cck_txpower(struct ieee80211_hw *hw,
tx_agc[RF90_PATH_A] = 0x10101010;
tx_agc[RF90_PATH_B] = 0x10101010;
} else if (rtlpriv->dm.dynamic_txhighpower_lvl ==
- TXHIGHPWRLEVEL_LEVEL1) {
+ TXHIGHPWRLEVEL_LEVEL2) {
tx_agc[RF90_PATH_A] = 0x00000000;
tx_agc[RF90_PATH_B] = 0x00000000;
} else{
--
1.8.1.4
^ permalink raw reply related
* Re: [PATCH] rtlwifi: rtl8192cu: Fix duplicate if test
From: Sergei Shtylyov @ 2013-06-18 18:24 UTC (permalink / raw)
To: Larry Finger; +Cc: linville, linux-wireless, netdev, stable
In-Reply-To: <1371579497-5575-1-git-send-email-Larry.Finger@lwfinger.net>
Hello.
On 06/18/2013 10:18 PM, Larry Finger wrote:
> A typo causes routine rtl92cu_phy_rf6052_set_cck_txpower() to test the
> same condition twice. The problem was found using cppcheck-1.49, and the
> proper fix was verified against the pre-mac80211 version of the code.
> This patch was originally included as commit 1288aa4, but was accidentally
You probably forgot to specify the summary line of that commit.
> reverted in a later patch.
> Reported-by: David Binderman <dcb314@hotmail.com> [original report]
> Reported-by: Andrea Morello <andrea.merello@gmail.com> [report of accidental reversion]
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> Cc: stable <stable@kernel.org> [back to 2.6.39]
WBR, Sergei
^ permalink raw reply
* [PATCH] rtlwifi: rtl8192cu: Fix duplicate if test
From: Larry Finger @ 2013-06-18 18:18 UTC (permalink / raw)
To: linville; +Cc: linux-wireless, Larry Finger, netdev, stable
A typo causes routine rtl92cu_phy_rf6052_set_cck_txpower() to test the
same condition twice. The problem was found using cppcheck-1.49, and the
proper fix was verified against the pre-mac80211 version of the code.
This patch was originally included as commit 1288aa4, but was accidentally
reverted in a later patch.
Reported-by: David Binderman <dcb314@hotmail.com> [original report]
Reported-by: Andrea Morello <andrea.merello@gmail.com> [report of accidental reversion]
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: stable <stable@kernel.org> [back to 2.6.39]
---
John,
Although this patch is trivial and could be included in the 3.10 stream,
I think we should not burden DaveM nor Linus with it this late in the 3.10
cycle. Please push it into the 3.11 stream.
Thanks,
Larry
---
drivers/net/wireless/rtlwifi/rtl8192cu/rf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/rtlwifi/rtl8192cu/rf.c b/drivers/net/wireless/rtlwifi/rtl8192cu/rf.c
index 953f1a0..2119313 100644
--- a/drivers/net/wireless/rtlwifi/rtl8192cu/rf.c
+++ b/drivers/net/wireless/rtlwifi/rtl8192cu/rf.c
@@ -104,7 +104,7 @@ void rtl92cu_phy_rf6052_set_cck_txpower(struct ieee80211_hw *hw,
tx_agc[RF90_PATH_A] = 0x10101010;
tx_agc[RF90_PATH_B] = 0x10101010;
} else if (rtlpriv->dm.dynamic_txhighpower_lvl ==
- TXHIGHPWRLEVEL_LEVEL1) {
+ TXHIGHPWRLEVEL_LEVEL2) {
tx_agc[RF90_PATH_A] = 0x00000000;
tx_agc[RF90_PATH_B] = 0x00000000;
} else{
--
1.8.1.4
^ permalink raw reply related
* Re: [PATCHv2 4/5] mac80211: add channel switch command and beacon callbacks
From: Simon Wunderlich @ 2013-06-18 17:27 UTC (permalink / raw)
To: Johannes Berg
Cc: Simon Wunderlich, linux-wireless, Simon Wunderlich,
Mathias Kretschmer
In-Reply-To: <1371567628.8318.43.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 3817 bytes --]
Hey Johannes,
I'm skipping the "style" comments (you were right most of the time anyway) and will
only comment on the design stuff, see below:
On Tue, Jun 18, 2013 at 05:00:28PM +0200, Johannes Berg wrote:
> On Fri, 2013-06-14 at 14:15 +0200, Simon Wunderlich wrote:
> > @@ -2818,6 +2830,8 @@ struct ieee80211_ops {
> > struct ieee80211_vif *vif,
> > struct inet6_dev *idev);
> > #endif
> > + void (*channel_switch_beacon)(struct ieee80211_hw *hw,
> > + struct ieee80211_vif *vif);
>
> What about channel contexts? Actually I don't really understand this?
> Shouldn't it say which channel to switch to?
>
My first implementation (ath9k) does rely on mac80211 to complete the
channel switch, so it does not even need to know which channel is switched
to. I can add that as we can assume other drivers will behave differently ...
> > /**
> > + * ieee80211_csa_finish - notify mac80211 about channel switch
> > + * @vif: &struct ieee80211_vif pointer from the add_interface callback.
> > + *
> > + * After a channel switch announcement was scheduled and the counter in this
> > + * announcement hit zero, this function must be called by the driver to
> > + * notify mac80211 that the channel can be changed.
> > + */
> > +void ieee80211_csa_finish(struct ieee80211_vif *vif);
>
> If there are multiple interfaces, should it be called multiple times?
> etc. Maybe it should be on a channel context instead?
>
Multiple interfaces are not supported - and I don't know how this should be
handled anyway. CSAs are triggered on a per-interface base from userspace,
and multiple CSAs would clash with each other (could be different channels,
different counters, etc ...).
Or would you have a suggestion how to handle this differently?
> > + netif_carrier_off(sdata->dev);
> > + err = ieee80211_vif_use_channel(sdata, &local->csa_chandef,
> > + IEEE80211_CHANCTX_SHARED);
> > + netif_carrier_on(sdata->dev);
>
> That seems like a really bad idea, deleting a channel context might tear
> down all kinds of device state and might require deleting the interface
> first ... I think the chan context API needs to be extended to switch
> instead.
>
Hm, yeah I can do that.
> > + if (WARN_ON(err < 0))
> > + return;
>
> This can fail _easily_ too, e.g. if some other vif stays on the channel
> and you're now using too many channel contexts.
>
> > + /* don't handle if chanctx is used */
> > + if (local->use_chanctx)
> > + return -EBUSY;
>
> Still don't really like the way you've implemented it :-)
>
Why not? :)
> > + vif->csa_active = 0;
>
> is that a counter, or should it be a bool?
It should be bool.
> > +static void ieee80211_update_csa(struct ieee80211_sub_if_data *sdata,
> > + struct beacon_data *beacon)
> > +{
> > + struct probe_resp *resp;
> > + int counter_beacon = sdata->csa_counter_offset_beacon;
> > + int counter_presp = sdata->csa_counter_offset_presp;
> > +
> > + if (WARN_ON(counter_beacon > beacon->tail_len))
> > + return;
> > +
> > + if (WARN_ON(((u8 *)beacon->tail)[counter_beacon] == 0))
> > + return;
>
> How can these happen?
>
Maybe when the beacon is re-assigned - although add a check for that
and remove these warnings ...
> > + ((u8 *)beacon->tail)[counter_beacon]--;
> > +
> > + if (counter_presp && sdata->vif.type == NL80211_IFTYPE_AP) {
> > + resp = rcu_dereference(sdata->u.ap.probe_resp);
>
> Who guarantees RCU protection?
>
Hmm ... should add that.
> > + if (WARN_ON(!resp))
> > + return;
>
> That can legimitately happen, no? At least userspace is allowed to not
> set probe_resp now, if you want to change that ...
>
If there is no presp then also counter_presp should not be set.
Cheers,
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: 3.7.8/amd64 full interrupt hangs due to iwlwifi under big nfs copies out
From: Johannes Berg @ 2013-06-18 17:04 UTC (permalink / raw)
To: Eric Dumazet
Cc: Marc MERLIN, David Miller, Larry.Finger, bhutchings,
linux-wireless, netdev, sgruszka
In-Reply-To: <1371574361.3252.259.camel@edumazet-glaptop>
On Tue, 2013-06-18 at 09:52 -0700, Eric Dumazet wrote:
> > We'll be submitting a patch to make single pages default.
>
> Do you think the same change would be possible for
> drivers/net/wireless/iwlegacy/4965-mac.c ?
Yes, Stanislaw?
johannes
^ permalink raw reply
* Re: 3.7.8/amd64 full interrupt hangs due to iwlwifi under big nfs copies out
From: Eric Dumazet @ 2013-06-18 16:52 UTC (permalink / raw)
To: Johannes Berg
Cc: Marc MERLIN, David Miller, Larry.Finger, bhutchings,
linux-wireless, netdev
In-Reply-To: <1361912099.8440.21.camel@jlt4.sipsolutions.net>
On Tue, 2013-02-26 at 21:54 +0100, Johannes Berg wrote:
> On Fri, 2013-02-22 at 22:14 -0800, Marc MERLIN wrote:
> > On Wed, Feb 20, 2013 at 10:15:03AM +0100, Johannes Berg wrote:
> > > On Wed, 2013-02-20 at 10:12 +0100, Johannes Berg wrote:
> > > > On Tue, 2013-02-19 at 08:21 -0800, Eric Dumazet wrote:
> > > > > On Tue, 2013-02-19 at 11:03 +0100, Johannes Berg wrote:
> > > > > > On Mon, 2013-02-18 at 21:17 -0800, Eric Dumazet wrote:
> > > > > >
> > > > > > > > chrome: page allocation failure: order:1, mode:0x4020
> > > > > > > > Pid: 8730, comm: chrome Tainted: G O 3.7.8-amd64-preempt-20121226-fixwd #1
> > > > > > > > Call Trace:
> > > > > > > > <IRQ> [<ffffffff810d5f38>] warn_alloc_failed+0x117/0x12c
> > > > > >
> > > > > > > You could try to load iwlwifi with amsdu_size_8K set to 0 (disable)
> > > > > > >
> > > > > > > It should hopefully use order-0 pages
> > > > > >
> > > > > > It will, do that then, unfortunately it can't switch at runtime because
> > > > > > it advertised this support to the access point or clients.
> > > > >
> > > > > What are the drawbacks of setting amsdu_size_8K to 0 by default ?
> > > >
> > > > We're discussing this now, the only downside would be that we couldn't
> > > > receive 8k A-MSDUs. Thing is, practically nobody uses A-MSDU anyway, and
> > > > even when I suspect the difference between 4k and 8k won't be huge.
> > >
> > > OTOH, this affects the protocol, and when you really can't allocate any
> > > order-1 pages you pointed out yourself that many other things also won't
> > > work, so I'm not really sure it makes a big difference if we change the
> > > driver?
> >
> > That as an unscientific test, but when I did the NFS eats all my pages
> > test using ethernet, my system didn't hang like it did with iwlagn.
> >
> > So while the NFS code is definitely doing something wrong when it uses
> > its default huge buffers, the e1000e code deals with it without hanging
> > my system.
> >
> > So thanks for trying to improve the iwlagn code to avoid those system
> > lockups.
>
> We'll be submitting a patch to make single pages default.
Do you think the same change would be possible for
drivers/net/wireless/iwlegacy/4965-mac.c ?
Thanks !
^ permalink raw reply
* ath9k_htc: station unable to authenticate
From: Ignacy Gawedzki @ 2013-06-18 16:48 UTC (permalink / raw)
To: linux-wireless
Hi,
After a lot of git-bisecting and other testing, it appears that commit
382a103b2b528a3085cde4ac56fc69d92a828b72 is the culprit.
To reproduce the problem, use backports-20130607, and the ath9k defconfig
(with no change otherwise).
The station is unable to authenticate with WPA2 to the AP. Interestingly
enough, if a monitor vif is created and upped beforehand, the authentication
succeeds.
After reverting the commit, authentication succeeds very quickly (without the
need to up a monitor vif), just as expected.
Note that I have kernel 3.8.0 (as per Ubuntu 13.04) and that original
ath9k_htc driver works as expected. This has been also tested on Debian with
driver from original kernel 3.9 (i.e. without backports).
For the moment I can live with the commit reverted, but I suppose it's been
there to fix something else, so this should probably be looked into by someone
more knowledgeable than me.
Ignacy
--
A person is shit's way of making more shit.
-- S. Barnett, anthropologist.
^ permalink raw reply
* Re: [RFC 2/2] mac80211: Fix bss ref leak.
From: Ben Greear @ 2013-06-18 16:33 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1371569205.8318.47.camel@jlt4.sipsolutions.net>
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
^ permalink raw reply
* Re: [PATCHv3 05/18] mac80211: fix timing for 5 MHz and 10 MHz channels
From: Simon Wunderlich @ 2013-06-18 16:27 UTC (permalink / raw)
To: Johannes Berg
Cc: Simon Wunderlich, linux-wireless, Mathias Kretschmer,
Simon Wunderlich
In-Reply-To: <1371565283.8318.21.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 791 bytes --]
On Tue, Jun 18, 2013 at 04:21:23PM +0200, Johannes Berg wrote:
>
> > +static inline int
> > +ieee80211_hw_get_divisor(struct ieee80211_hw *hw)
> > +{
> > + switch (hw->conf.chandef.width) {
> > + case NL80211_CHAN_WIDTH_5:
> > + return 4;
> > + case NL80211_CHAN_WIDTH_10:
> > + return 2;
> > + default:
> > + return 1;
> > + }
> > +}
>
> Using this function will break with drivers that use channel contexts,
> you really shouldn't do that.
Hmm ... yeah, you are right. I've used this a lot in the following patches, argh.
Will find another solution.
>
> Also, you're disabling HT so why bother changing minstrel_ht?
Mhm, yeah that is probably not neccesary. I'll remove that, however we remember
it once HT support should be added. ;)
Thanks,
Simon
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: Lots of confusion on bss refcounting.
From: Ben Greear @ 2013-06-18 15:59 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <1371570723.22256.0.camel@jlt4.sipsolutions.net>
On 06/18/2013 08:52 AM, Johannes Berg wrote:
>
>>> You mean ->current_bss? That should be handled in all the callbacks in
>>> sme.c or so
>>
>> Looks like much of the action happens on work-queues. I'm wondering if
>> we managed to delete wdev objects before we have completely cleaned up
>> in some cases...
>
> Don't we flush work structs appropriately?
Looks like it, from core.c in the netdev event handler:
/*
* Ensure that all events have been processed and
* freed.
*/
cfg80211_process_wdev_events(wdev);
/* I just added this to see if it helps... */
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;
Some of the unregister and similar sme.c calls that should be cleaning up
the current_bss have some early returns if state does not match expected
value. If the warning above hits, then probably we are hitting those
somehow.
If not, then I'll keep looking :)
Thanks,
Ben
>
> johannes
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCHv3 07/18] mac80211: add radiotap flag and handling for 5/10 MHz
From: Johannes Berg @ 2013-06-18 15:53 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Mathias Kretschmer, Simon Wunderlich
In-Reply-To: <20130618155051.GD27492@pandem0nium>
On Tue, 2013-06-18 at 17:50 +0200, Simon Wunderlich wrote:
> Hmm, if this [1] is the official standard, then no, it is not defined. Don't
> know when this was added in wireshark, and it was only added implicitly in
> the code (no header files etc). But since it was there, I figured it would
> be a good idea to re-use it (unaware that there was some "standard").
>
> How shall we proceed, propose that on some radiotap mailing list officially?
Yes, see http://www.radiotap.org/Standardisation
> > > + RX_FLAG_10MHZ = BIT(26),
> > > + RX_FLAG_5MHZ = BIT(27),
> >
> > Does that make sense? We know what kind of channel we're on? Though it
> > might be easier for the driver, so it may make sense I guess.
>
> Yeah, it's a little easier for the driver, and I thought it would be
> cleaner to get this reported through a flag.
Yeah I tend to agree.
johannes
^ permalink raw reply
* Re: Lots of confusion on bss refcounting.
From: Johannes Berg @ 2013-06-18 15:52 UTC (permalink / raw)
To: Ben Greear; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <51C08119.3000407@candelatech.com>
> > You mean ->current_bss? That should be handled in all the callbacks in
> > sme.c or so
>
> Looks like much of the action happens on work-queues. I'm wondering if
> we managed to delete wdev objects before we have completely cleaned up
> in some cases...
Don't we flush work structs appropriately?
johannes
^ permalink raw reply
* Re: [PATCHv3 07/18] mac80211: add radiotap flag and handling for 5/10 MHz
From: Simon Wunderlich @ 2013-06-18 15:50 UTC (permalink / raw)
To: Johannes Berg
Cc: Simon Wunderlich, linux-wireless, Mathias Kretschmer,
Simon Wunderlich
In-Reply-To: <1371565626.8318.26.camel@jlt4.sipsolutions.net>
[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]
On Tue, Jun 18, 2013 at 04:27:06PM +0200, Johannes Berg wrote:
> On Thu, 2013-05-16 at 13:00 +0200, Simon Wunderlich wrote:
>
> > Wireshark
>
> doesn't really matter all that much, but I guess the radiotap standard
> also defines it.
>
Hmm, if this [1] is the official standard, then no, it is not defined. Don't
know when this was added in wireshark, and it was only added implicitly in
the code (no header files etc). But since it was there, I figured it would
be a good idea to re-use it (unaware that there was some "standard").
How shall we proceed, propose that on some radiotap mailing list officially?
>
> > + RX_FLAG_10MHZ = BIT(26),
> > + RX_FLAG_5MHZ = BIT(27),
>
> Does that make sense? We know what kind of channel we're on? Though it
> might be easier for the driver, so it may make sense I guess.
Yeah, it's a little easier for the driver, and I thought it would be
cleaner to get this reported through a flag.
Cheers,
Simon
[1] http://www.radiotap.org/defined-fields/Channel
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: Lots of confusion on bss refcounting.
From: Ben Greear @ 2013-06-18 15:47 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless@vger.kernel.org
In-Reply-To: <1371559771.8318.12.camel@jlt4.sipsolutions.net>
On 06/18/2013 05:49 AM, Johannes Berg wrote:
> On Mon, 2013-06-17 at 17:30 -0700, Ben Greear wrote:
>> On 06/17/2013 02:31 PM, Ben Greear wrote:
>>> On 06/17/2013 12:09 PM, Ben Greear wrote:
>>>> On 06/17/2013 12:02 PM, Johannes Berg wrote:
>>>
>>>> The bss reference is passed back, and through luck or careful programming,
>>>> it *seems* that all paths related to calling ieee80211_rx_mgmt_assoc_resp
>>>> managed to consume the bss.
>>>>
>>>> I haven't figured out yet why this is not an erroneous put since I didn't
>>>> find the reference taken in the first place.
>>>>
>>>> I'm going to work on making some changes to the ref counting scheme
>>>> a bit. I'd rather have the code perhaps take and put a few refs
>>>> it might otherwise skip to keep the ownership cleaner and make
>>>> the code easier to debug and understand...
>>>>
>>>> I'll post some for RFC when I make some progress.
>>>
>>> I think I found at least some of the leaks.
>>>
>>> In places like ieee80211_mgd_stop, we were calling ieee80211_destroy_assoc_data,
>>> but it was not putting the bss reference.
>>>
>>> I'll post some RFC patches in a minute or two...first is debugging
>>> logic, second attempts to fix bss ref counting. This needs more
>>> testing before it is applied...we will continue testing it....
>>
>> It seems that the wdev objects (struct wireless_dev) can also
>> hold a reference to the bss.
>>
>> Do you happen to know what code is responsible for destructing
>> those objects? I want to check to make sure it properly puts
>> its reference.
>
> You mean ->current_bss? That should be handled in all the callbacks in
> sme.c or so
Looks like much of the action happens on work-queues. I'm wondering if
we managed to delete wdev objects before we have completely cleaned up
in some cases...
I was planning to add code to explicitly clean up current_bss if it
is not NULL in whatever code actually does the final cleanup for
wdev objects.
I didn't see an obvious cleanup method in sme.c, but will go look around
some more...
Thanks,
Ben
>
> johannes
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [RFC 2/2] mac80211: Fix bss ref leak.
From: Ben Greear @ 2013-06-18 15:38 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless
In-Reply-To: <1371569205.8318.47.camel@jlt4.sipsolutions.net>
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
^ permalink raw reply
* Re: [RFC 2/2] mac80211: Fix bss ref leak.
From: Johannes Berg @ 2013-06-18 15:26 UTC (permalink / raw)
To: greearb; +Cc: linux-wireless
In-Reply-To: <1371504746-8476-2-git-send-email-greearb@candelatech.com>
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
^ permalink raw reply
* Re: Connection not established with Realtek RTL8188CUS based USB device (EDIMAX)
From: Larry Finger @ 2013-06-18 15:26 UTC (permalink / raw)
To: Paul Menzel; +Cc: linux-wireless
In-Reply-To: <1371542085.7163.15.camel@mattotaupa>
On 06/18/2013 02:54 AM, Paul Menzel wrote:
> Three more questions regarding this patch.
>
> 1. Could you add `CC: stable@vger.kernel.org` to the patch so it gets
> backported to the stable Linux kernel releases.
I thought I had done that. In fact, the patch was Cc'd to stable, but I missed
the entry in the commit message. I have written to Stable to see what caqn be done.
> 2. Does this fix a regression or did it never work with WEP/WPA(1)
> networks before?
It does fix a regression, but I have no idea when it happened.
> 3. If wpa_supplicant only prints WPA to `/var/log/syslog`, can I be sure
> this is *no* WPA2 network? Because I always thought this is a WPA2
> network.
The data in an "iwlist scan" will state the version of WPA in use. If it is
WPA2, you get a line with
IE: IEEE 802.11i/WPA2 Version 1
For WPA1, it is
IE: WPA Version 1
Larry
^ permalink raw reply
* Re: [PATCHv2 2/5] nl80211/cfg80211: add channel switch command
From: Johannes Berg @ 2013-06-18 15:21 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <20130618151424.GA27492@pandem0nium>
> > If you're going to use nested IEs, shouldn't you define a separate
> > namespace to be used within the nesting?
>
> The idea was to recycle the nl80211_parse_beacon() function for the CSA IEs,
> but if you prefer I can define an own namespace and create a separate function
> to check.
Hmm, ok. Not sure. This just seems like an awful lot of stack space,
increasing with each new attribute ...
johannes
^ permalink raw reply
* Re: [PATCHv2 2/5] nl80211/cfg80211: add channel switch command
From: Johannes Berg @ 2013-06-18 15:20 UTC (permalink / raw)
To: Simon Wunderlich; +Cc: linux-wireless, Simon Wunderlich, Mathias Kretschmer
In-Reply-To: <20130618151806.GB27492@pandem0nium>
On Tue, 2013-06-18 at 17:18 +0200, Simon Wunderlich wrote:
> On Tue, Jun 18, 2013 at 04:50:30PM +0200, Johannes Berg wrote:
> >
> > > + * @NL80211_CMD_CHANNEL_SWITCH: Perform a channel switch by announcing the
> > > + * the new channel information (Channel Switch Announcement - CSA)
> > > + * in the beacon for some time (as defined in the
> > > + * %NL80211_ATTR_CH_SWITCH_COUNT parameter) and then change to the
> > > + * new channel. Userspace provides the new channel information (using
> > > + * %NL80211_ATTR_WIPHY_FREQ and the attributes determining channel
> > > + * width). %NL80211_ATTR_CH_SWITCH_BLOCK_TX may be supplied to inform
> > > + * other station that transmission must be blocked until the channel
> > > + * switch is complete.
> >
> > We probably need a flag to indicate that the command is available, since
> > mac80211 might implement it but not all drivers will?
>
> Yeah ... shall we announce it as CMD() in nl80211_send_wiphy()? Or feature
> flag?
CMD() works (make sure to only put it if split), but need a wiphy flag
since mac80211 always implements it, I think.
johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox