Skip to contentSkip to conversation

sta-info fixes, especially destroy()

PATCH In review Johannes Berg 2 participants 9 messages Apr 1, 2008 mac80211

lore
Share & tools
lore

Johannes Berg

author maintainer
#0 lore raw

cover sta-info fixes, especially destroy()

This series should fix the problems with sta_info_destroy() that Tomas pointed out. Tomas, please give the patches a try, I don't currently have a suitable IBSS test environment. Especially, it would be worth trying out making an IBSS mode virtual interface re-join an IBSS (to test the flush_delayed stuff.)

johannes

author maintainer
#1 lore raw

PATCH 1/6 mac80211 ibss: flush only stations belonging to current interface

When joining a new IBSS, all old stations are flushed, but currently all stations belonging to all virtual interfaces are flushed, which is wrong. This patch fixes it.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/ieee80211_sta.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
net/mac80211/ieee80211_sta.c +3 -2
@@ -2231,8 +2231,10 @@ static int ieee80211_sta_join_ibss(struc
 
 	sband = local->hw.wiphy->bands[local->hw.conf.channel->band];
 
+	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
+
 	/* Remove possible STA entries from other IBSS networks. */
-	sta_info_flush(local, NULL);
+	sta_info_flush(local, sdata);
 
 	if (local->ops->reset_tsf) {
 		/* Reset own TSF to allow time synchronization work. */

@@ -2245,7 +2247,6 @@ static int ieee80211_sta_join_ibss(struc
 
 	local->hw.conf.beacon_int = bss->beacon_int >= 10 ? bss->beacon_int : 10;
 
-	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 	sdata->drop_unencrypted = bss->capability &
 		WLAN_CAPABILITY_PRIVACY ? 1 : 0;
author maintainer
#2 lore raw

PATCH 2/6 mac80211: fix sta_info_destroy(NULL)

In reply to Johannes Berg

sta_info_destroy(NULL) should be valid, but currently isn't because the argument is dereferenced before the NULL check. There are no users that currently pass in NULL, i.e. all check before calling the function, but I want to change that.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/sta_info.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
net/mac80211/sta_info.c +5 -3
@@ -129,16 +129,18 @@ struct sta_info *sta_info_get_by_idx(str
 
 void sta_info_destroy(struct sta_info *sta)
 {
-	struct ieee80211_local *local = sta->local;
+	struct ieee80211_local *local;
 	struct sk_buff *skb;
 	int i;
 	DECLARE_MAC_BUF(mbuf);
 
+	ASSERT_RTNL();
+	might_sleep();
+
 	if (!sta)
 		return;
 
-	ASSERT_RTNL();
-	might_sleep();
+	local = sta->local;
 
 	rate_control_remove_sta_debugfs(sta);
 	ieee80211_sta_debugfs_remove(sta);
author maintainer
#3 lore raw

PATCH 3/6 mac80211: automatically free sta struct when insertion fails

In reply to Johannes Berg

When STA structure insertion fails, it has been allocated but isn't really alive yet, it isn't reachable by any other code and also can't yet have much configured. This patch changes the code so that when the insertion fails, the resulting STA pointer is no longer valid because it is freed.

Signed-off-by Johannes Berg <[email protected]>

---
I'm open to not doing that and instead showing sta_info_free() in the API but I fear that would be more prone to mis-use than this clearly documented restriction.

 net/mac80211/cfg.c           |    2 -
 net/mac80211/ieee80211.c     |    2 -
 net/mac80211/ieee80211_sta.c |    5 --
 net/mac80211/mesh_plink.c    |    6 ++-
 net/mac80211/sta_info.c      |   79 +++++++++++++++++++++++++++++++------------
 5 files changed, 64 insertions(+), 30 deletions(-)
net/mac80211/cfg.c +1 -1
@@ -672,7 +672,7 @@ static int ieee80211_add_station(struct 
 
 	err = sta_info_insert(sta);
 	if (err) {
-		sta_info_destroy(sta);
+		/* STA has been freed */
 		rcu_read_unlock();
 		return err;
 	}
net/mac80211/ieee80211.c +1 -1
@@ -268,7 +268,7 @@ static int ieee80211_open(struct net_dev
 
 		res = sta_info_insert(sta);
 		if (res) {
-			sta_info_destroy(sta);
+			/* STA has been freed */
 			return res;
 		}
 		break;
net/mac80211/ieee80211_sta.c +1 -4
@@ -1920,7 +1920,6 @@ static void ieee80211_rx_mgmt_assoc_resp
 		if (err) {
 			printk(KERN_DEBUG "%s: failed to insert STA entry for"
 			       " the AP (error %d)\n", dev->name, err);
-			sta_info_destroy(sta);
 			rcu_read_unlock();
 			return;
 		}

@@ -4150,10 +4149,8 @@ struct sta_info * ieee80211_ibss_add_sta
 
 	rate_control_rate_init(sta, local);
 
-	if (sta_info_insert(sta)) {
-		sta_info_destroy(sta);
+	if (sta_info_insert(sta))
 		return NULL;
-	}
 
 	return sta;
 }
net/mac80211/mesh_plink.c +4 -2
@@ -89,6 +89,10 @@ static inline void mesh_plink_fsm_restar
 	sta->plink_retries = 0;
 }
 
+/*
+ * NOTE: This is just an alias for sta_info_alloc(), see notes
+ *       on it in the lifecycle management section!
+ */
 static struct sta_info *mesh_plink_alloc(struct ieee80211_sub_if_data *sdata,
 					 u8 *hw_addr, u64 rates)
 {

@@ -235,7 +239,6 @@ void mesh_neighbour_update(u8 *hw_addr, 
 			return;
 		}
 		if (sta_info_insert(sta)) {
-			sta_info_destroy(sta);
 			rcu_read_unlock();
 			return;
 		}

@@ -506,7 +509,6 @@ void mesh_rx_plink_frame(struct net_devi
 			return;
 		}
 		if (sta_info_insert(sta)) {
-			sta_info_destroy(sta);
 			rcu_read_unlock();
 			return;
 		}
net/mac80211/sta_info.c +57 -22

141 lines not on this page · raw message

author maintainer
#4 lore raw

PATCH v2 3/6 mac80211: automatically free sta struct when insertion fails

Subject: mac80211: automatically free sta struct when insertion fails

When STA structure insertion fails, it has been allocated but isn't really alive yet, it isn't reachable by any other code and also can't yet have much configured. This patch changes the code so that when the insertion fails, the resulting STA pointer is no longer valid because it is freed.

Signed-off-by Johannes Berg <[email protected]>

---
I'm open to not doing that and instead showing sta_info_free() in the API but I fear that would be more prone to mis-use than this clearly documented restriction.

v2: minor reject fixed due to sta-info splitting.

 net/mac80211/cfg.c           |    2 -
 net/mac80211/ieee80211.c     |    2 -
 net/mac80211/ieee80211_sta.c |    5 --
 net/mac80211/mesh_plink.c    |    6 ++-
 net/mac80211/sta_info.c      |   80 ++++++++++++++++++++++++++++++-------------
 5 files changed, 64 insertions(+), 31 deletions(-)
net/mac80211/cfg.c +1 -1
@@ -672,7 +672,7 @@ static int ieee80211_add_station(struct 
 
 	err = sta_info_insert(sta);
 	if (err) {
-		sta_info_destroy(sta);
+		/* STA has been freed */
 		rcu_read_unlock();
 		return err;
 	}
net/mac80211/ieee80211.c +1 -1
@@ -268,7 +268,7 @@ static int ieee80211_open(struct net_dev
 
 		res = sta_info_insert(sta);
 		if (res) {
-			sta_info_destroy(sta);
+			/* STA has been freed */
 			return res;
 		}
 		break;
net/mac80211/ieee80211_sta.c +1 -4
@@ -1942,7 +1942,6 @@ static void ieee80211_rx_mgmt_assoc_resp
 		if (err) {
 			printk(KERN_DEBUG "%s: failed to insert STA entry for"
 			       " the AP (error %d)\n", dev->name, err);
-			sta_info_destroy(sta);
 			rcu_read_unlock();
 			return;
 		}

@@ -4172,10 +4171,8 @@ struct sta_info * ieee80211_ibss_add_sta
 
 	rate_control_rate_init(sta, local);
 
-	if (sta_info_insert(sta)) {
-		sta_info_destroy(sta);
+	if (sta_info_insert(sta))
 		return NULL;
-	}
 
 	return sta;
 }
net/mac80211/mesh_plink.c +4 -2
@@ -89,6 +89,10 @@ static inline void mesh_plink_fsm_restar
 	sta->plink_retries = 0;
 }
 
+/*
+ * NOTE: This is just an alias for sta_info_alloc(), see notes
+ *       on it in the lifecycle management section!
+ */
 static struct sta_info *mesh_plink_alloc(struct ieee80211_sub_if_data *sdata,
 					 u8 *hw_addr, u64 rates)
 {

@@ -235,7 +239,6 @@ void mesh_neighbour_update(u8 *hw_addr, 
 			return;
 		}
 		if (sta_info_insert(sta)) {
-			sta_info_destroy(sta);
 			rcu_read_unlock();
 			return;
 		}

@@ -506,7 +509,6 @@ void mesh_rx_plink_frame(struct net_devi
 			return;
 		}
 		if (sta_info_insert(sta)) {
-			sta_info_destroy(sta);
 			rcu_read_unlock();
 			return;
 		}
net/mac80211/sta_info.c +57 -23

141 lines not on this page · raw message

author maintainer
#5 lore raw

PATCH 4/6 mac80211: clean up sta_info_destroy() users wrt. RCU/locking

In reply to Johannes Berg

Calling sta_info_destroy() doesn't require RCU-synchronisation before-hand because it does that internally. However, it does require rtnl-locking so insert that where necessary.

Also clean up the code doing it internally to be a bit clearer and not synchronize twice if keys are configured.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/cfg.c           |    6 +-----
 net/mac80211/ieee80211_sta.c |    1 -
 net/mac80211/sta_info.c      |   33 +++++++++++++++++++--------------
 3 files changed, 20 insertions(+), 20 deletions(-)
net/mac80211/cfg.c +1 -5
@@ -700,11 +700,7 @@ static int ieee80211_del_station(struct 
 			return -ENOENT;
 
 		sta_info_unlink(&sta);
-
-		if (sta) {
-			synchronize_rcu();
-			sta_info_destroy(sta);
-		}
+		sta_info_destroy(sta);
 	} else
 		sta_info_flush(local, sdata);
net/mac80211/sta_info.c +19 -14
@@ -180,13 +180,22 @@ void sta_info_destroy(struct sta_info *s
 		mesh_plink_deactivate(sta);
 #endif
 
-	/*
-	 * NOTE: This will call synchronize_rcu() internally to
-	 * make sure no key references can be in use. We rely on
-	 * that here for the mesh code!
-	 */
-	ieee80211_key_free(sta->key);
-	WARN_ON(sta->key);
+	if (sta->key) {
+		/*
+		 * NOTE: This will call synchronize_rcu() internally to
+		 * make sure no key references can be in use. We rely on
+		 * that when we take this branch to make sure nobody can
+		 * reference this STA struct any longer!
+		 */
+		ieee80211_key_free(sta->key);
+		WARN_ON(sta->key);
+	} else {
+		/*
+		 * Make sure that nobody can reference this STA struct
+		 * any longer.
+		 */
+		synchronize_rcu();
+	}
 
 #ifdef CONFIG_MAC80211_MESH
 	if (ieee80211_vif_is_mesh(&sta->sdata->vif))

@@ -627,11 +636,9 @@ static void sta_info_debugfs_add_work(st
 		rate_control_add_sta_debugfs(sta);
 
 		sta = __sta_info_unpin(sta);
-
-		if (sta) {
-			synchronize_rcu();
-			sta_info_destroy(sta);
-		}
+		rtnl_lock();
+		sta_info_destroy(sta);
+		rtnl_unlock();
 	}
 }
 #endif

@@ -693,8 +700,6 @@ int sta_info_flush(struct ieee80211_loca
 	}
 	spin_unlock_irqrestore(&local->sta_lock, flags);
 
-	synchronize_rcu();
-
 	list_for_each_entry_safe(sta, tmp, &tmp_list, list)
 		sta_info_destroy(sta);
net/mac80211/ieee80211_sta.c +0 -1
@@ -939,7 +939,6 @@ static void ieee80211_associated(struct 
 	rcu_read_unlock();
 
 	if (disassoc && sta) {
-		synchronize_rcu();
 		rtnl_lock();
 		sta_info_destroy(sta);
 		rtnl_unlock();
author maintainer
#6 lore raw

PATCH 5/6 mac80211: sta_info_flush() fixes

In reply to Johannes Berg

When the IBSS code tries to flush the STA list, it does so in an atomic context. Flushing isn't safe there, however, and requires the RTNL, so we need to defer it to a workqueue.

Signed-off-by Johannes Berg <[email protected]>
---
 net/mac80211/ieee80211_i.h   |    2 +
 net/mac80211/ieee80211_sta.c |    2 -
 net/mac80211/key.c           |    9 +++++
 net/mac80211/sta_info.c      |   70 +++++++++++++++++++++++++++++++++++++++++++
 net/mac80211/sta_info.h      |    2 +
 5 files changed, 84 insertions(+), 1 deletion(-)
net/mac80211/ieee80211_i.h +2 -0
@@ -602,6 +602,8 @@ struct ieee80211_local {
 	spinlock_t sta_lock;
 	unsigned long num_sta;
 	struct list_head sta_list;
+	struct list_head sta_flush_list;
+	struct work_struct sta_flush_work;
 	struct sta_info *sta_hash[STA_HASH_SIZE];
 	struct timer_list sta_cleanup;
net/mac80211/sta_info.c +70 -0

96 lines not on this page · raw message

net/mac80211/sta_info.h +2 -0
@@ -354,5 +354,7 @@ int sta_info_start(struct ieee80211_loca
 void sta_info_stop(struct ieee80211_local *local);
 int sta_info_flush(struct ieee80211_local *local,
 		    struct ieee80211_sub_if_data *sdata);
+void sta_info_flush_delayed(struct ieee80211_sub_if_data *sdata);
+void __ieee80211_run_pending_flush(struct ieee80211_local *local);
 
 #endif /* STA_INFO_H */
net/mac80211/ieee80211_sta.c +1 -1
@@ -2232,7 +2232,7 @@ static int ieee80211_sta_join_ibss(struc
 	sdata = IEEE80211_DEV_TO_SUB_IF(dev);
 
 	/* Remove possible STA entries from other IBSS networks. */
-	sta_info_flush(local, sdata);
+	sta_info_flush_delayed(sdata);
 
 	if (local->ops->reset_tsf) {
 		/* Reset own TSF to allow time synchronization work. */
net/mac80211/key.c +9 -0
@@ -73,6 +73,15 @@ static void ieee80211_key_enable_hw_acce
 	if (!key->local->ops->set_key)
 		return;
 
+	/*
+	 * This makes sure that all pending flushes have
+	 * actually completed prior to uploading new key
+	 * material to the hardware. That is necessary to
+	 * avoid races between flushing STAs and adding
+	 * new keys for them.
+	 */
+	__ieee80211_run_pending_flush(key->local);
+
 	addr = get_mac_for_key(key);
 
 	ret = key->local->ops->set_key(local_to_hw(key->local), SET_KEY,
author maintainer
#7 lore raw

PATCH 6/6 mac80211: fix sparse complaint in ieee80211_sta_def_wmm_params

In reply to Johannes Berg

A variable 'i' is being shadowed by another one, but the second one can just be removed.

Signed-off-by Johannes Berg <[email protected]>
Cc Vladimir Koutny <[email protected]>
---
 net/mac80211/ieee80211_sta.c |    1 -
 1 file changed, 1 deletion(-)
net/mac80211/ieee80211_sta.c +0 -1
@@ -244,7 +244,6 @@ static void ieee80211_sta_def_wmm_params
 
 	if (local->ops->conf_tx) {
 		struct ieee80211_tx_queue_params qparam;
-		int i;
 
 		memset(&qparam, 0, sizeof(qparam));
#8 lore raw
In reply to Johannes Berg
Quoted 6 lines · Johannes Berg

This series should fix the problems with sta_info_destroy() that
Tomas pointed out. Tomas, please give the patches a try, I don't currently have a suitable IBSS test environment. Especially, it would be worth trying out making an IBSS mode virtual interface re-join an IBSS (to test the flush_delayed stuff.)

Thanks. I will test the IBSS with these patches, not sure it will happen this week.

Tomas