[PATCH 0/6] sta-info fixes, especially destroy()

[PATCH 0/6] sta-info fixes, especially destroy()

[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.)

johannes

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

[Johannes Berg]

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 <johannes@sipsolutions.net>
---
 net/mac80211/ieee80211_sta.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

--- everything.orig/net/mac80211/ieee80211_sta.c	2008-03-31 18:21:58.000000000 +0200
+++ everything/net/mac80211/ieee80211_sta.c	2008-03-31 18:22:23.000000000 +0200
@@ -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;
 

-- 

[PATCH 2/6] mac80211: fix sta_info_destroy(NULL)

[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 <johannes@sipsolutions.net>
---
 net/mac80211/sta_info.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

--- everything.orig/net/mac80211/sta_info.c	2008-03-31 18:21:58.000000000 +0200
+++ everything/net/mac80211/sta_info.c	2008-03-31 18:22:24.000000000 +0200
@@ -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);

-- 

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

[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 <johannes@sipsolutions.net>
---
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(-)

--- everything.orig/net/mac80211/cfg.c	2008-03-31 18:21:57.000000000 +0200
+++ everything/net/mac80211/cfg.c	2008-03-31 18:22:24.000000000 +0200
@@ -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;
 	}
--- everything.orig/net/mac80211/ieee80211.c	2008-03-31 18:21:57.000000000 +0200
+++ everything/net/mac80211/ieee80211.c	2008-03-31 18:22:24.000000000 +0200
@@ -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;
--- everything.orig/net/mac80211/ieee80211_sta.c	2008-03-31 18:22:23.000000000 +0200
+++ everything/net/mac80211/ieee80211_sta.c	2008-03-31 18:22:24.000000000 +0200
@@ -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;
 }
--- everything.orig/net/mac80211/mesh_plink.c	2008-03-31 18:21:57.000000000 +0200
+++ everything/net/mac80211/mesh_plink.c	2008-03-31 18:22:24.000000000 +0200
@@ -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;
 		}
--- everything.orig/net/mac80211/sta_info.c	2008-03-31 18:22:24.000000000 +0200
+++ everything/net/mac80211/sta_info.c	2008-03-31 18:22:24.000000000 +0200
@@ -36,16 +36,23 @@
  * (which is pretty useless) or insert it into the hash table using
  * sta_info_insert() which demotes the reference from ownership to a regular
  * RCU-protected reference; if the function is called without protection by an
- * RCU critical section the reference is instantly invalidated.
+ * RCU critical section the reference is instantly invalidated. Note that the
+ * caller may not do much with the STA info before inserting it, in particular,
+ * it may not start any mesh peer link management or add encryption keys.
+ *
+ * When the insertion fails (sta_info_insert()) returns non-zero), the
+ * structure will have been freed by sta_info_insert()!
  *
  * Because there are debugfs entries for each station, and adding those
  * must be able to sleep, it is also possible to "pin" a station entry,
  * that means it can be removed from the hash table but not be freed.
- * See the comment in __sta_info_unlink() for more information.
+ * See the comment in __sta_info_unlink() for more information, this is
+ * an internal capability only.
  *
  * In order to remove a STA info structure, the caller needs to first
  * unlink it (sta_info_unlink()) from the list and hash tables and
- * then wait for an RCU synchronisation before it can be freed. Due to
+ * then destroy it while holding the RTNL; sta_info_destroy() will wait
+ * for an RCU grace period to elapse before actually freeing it. Due to
  * the pinning and the possibility of multiple callers trying to remove
  * the same STA info at the same time, sta_info_unlink() can clear the
  * STA info pointer it is passed to indicate that the STA info is owned
@@ -127,12 +134,35 @@ struct sta_info *sta_info_get_by_idx(str
 	return NULL;
 }
 
+/**
+ * __sta_info_free - internal STA free helper
+ *
+ * @sta: STA info to free
+ *
+ * This function must undo everything done by sta_info_alloc()
+ * that may happen before sta_info_insert().
+ */
+static void __sta_info_free(struct ieee80211_local *local,
+			    struct sta_info *sta)
+{
+	DECLARE_MAC_BUF(mbuf);
+
+	rate_control_free_sta(sta->rate_ctrl, sta->rate_ctrl_priv);
+	rate_control_put(sta->rate_ctrl);
+
+#ifdef CONFIG_MAC80211_VERBOSE_DEBUG
+	printk(KERN_DEBUG "%s: Destroyed STA %s\n",
+	       wiphy_name(local->hw.wiphy), print_mac(mbuf, sta->addr));
+#endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
+
+	kfree(sta);
+}
+
 void sta_info_destroy(struct sta_info *sta)
 {
 	struct ieee80211_local *local;
 	struct sk_buff *skb;
 	int i;
-	DECLARE_MAC_BUF(mbuf);
 
 	ASSERT_RTNL();
 	might_sleep();
@@ -175,15 +205,8 @@ void sta_info_destroy(struct sta_info *s
 		del_timer_sync(&sta->ampdu_mlme.tid_rx[i].session_timer);
 		del_timer_sync(&sta->ampdu_mlme.tid_tx[i].addba_resp_timer);
 	}
-	rate_control_free_sta(sta->rate_ctrl, sta->rate_ctrl_priv);
-	rate_control_put(sta->rate_ctrl);
-
-#ifdef CONFIG_MAC80211_VERBOSE_DEBUG
-	printk(KERN_DEBUG "%s: Destroyed STA %s\n",
-	       wiphy_name(local->hw.wiphy), print_mac(mbuf, sta->addr));
-#endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
 
-	kfree(sta);
+	__sta_info_free(local, sta);
 }
 
 
@@ -264,6 +287,7 @@ int sta_info_insert(struct sta_info *sta
 	struct ieee80211_local *local = sta->local;
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 	unsigned long flags;
+	int err = 0;
 	DECLARE_MAC_BUF(mac);
 
 	/*
@@ -271,20 +295,23 @@ int sta_info_insert(struct sta_info *sta
 	 * something inserts a STA (on one CPU) without holding the RTNL
 	 * and another CPU turns off the net device.
 	 */
-	if (unlikely(!netif_running(sdata->dev)))
-		return -ENETDOWN;
-
-	if (WARN_ON(compare_ether_addr(sta->addr, sdata->dev->dev_addr) == 0))
-		return -EINVAL;
+	if (unlikely(!netif_running(sdata->dev))) {
+		err = -ENETDOWN;
+		goto out_free;
+	}
 
-	if (WARN_ON(is_multicast_ether_addr(sta->addr)))
-		return -EINVAL;
+	if (WARN_ON(compare_ether_addr(sta->addr, sdata->dev->dev_addr) == 0 ||
+	            is_multicast_ether_addr(sta->addr))) {
+		err = -EINVAL;
+		goto out_free;
+	}
 
 	spin_lock_irqsave(&local->sta_lock, flags);
 	/* check if STA exists already */
 	if (__sta_info_find(local, sta->addr)) {
 		spin_unlock_irqrestore(&local->sta_lock, flags);
-		return -EEXIST;
+		err = -EEXIST;
+		goto out_free;
 	}
 	list_add(&sta->list, &local->sta_list);
 	local->num_sta++;
@@ -307,9 +334,13 @@ int sta_info_insert(struct sta_info *sta
 	spin_unlock_irqrestore(&local->sta_lock, flags);
 
 #ifdef CONFIG_MAC80211_DEBUGFS
-	/* debugfs entry adding might sleep, so schedule process
+	/*
+	 * Debugfs entry adding might sleep, so schedule process
 	 * context task for adding entry for STAs that do not yet
-	 * have one. */
+	 * have one.
+	 * NOTE: due to auto-freeing semantics this may only be done
+	 *       if the insertion is successful!
+	 */
 	queue_work(local->hw.workqueue, &local->sta_debugfs_add);
 #endif
 
@@ -317,6 +348,10 @@ int sta_info_insert(struct sta_info *sta
 		mesh_accept_plinks_update(sdata);
 
 	return 0;
+ out_free:
+	BUG_ON(!err);
+ 	__sta_info_free(local, sta);
+	return err;
 }
 
 static inline void __bss_tim_set(struct ieee80211_if_ap *bss, u16 aid)

-- 

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

[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 <johannes@sipsolutions.net>
---
 net/mac80211/cfg.c           |    6 +-----
 net/mac80211/ieee80211_sta.c |    1 -
 net/mac80211/sta_info.c      |   33 +++++++++++++++++++--------------
 3 files changed, 20 insertions(+), 20 deletions(-)

--- everything.orig/net/mac80211/cfg.c	2008-03-31 18:22:24.000000000 +0200
+++ everything/net/mac80211/cfg.c	2008-03-31 18:22:25.000000000 +0200
@@ -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);
 
--- everything.orig/net/mac80211/sta_info.c	2008-03-31 18:22:24.000000000 +0200
+++ everything/net/mac80211/sta_info.c	2008-03-31 18:22:25.000000000 +0200
@@ -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);
 
--- everything.orig/net/mac80211/ieee80211_sta.c	2008-03-31 18:22:24.000000000 +0200
+++ everything/net/mac80211/ieee80211_sta.c	2008-03-31 18:22:25.000000000 +0200
@@ -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();

-- 

[PATCH 5/6] mac80211: sta_info_flush() fixes

[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 <johannes@sipsolutions.net>
---
 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(-)

--- everything.orig/net/mac80211/ieee80211_i.h	2008-03-31 18:25:36.000000000 +0200
+++ everything/net/mac80211/ieee80211_i.h	2008-03-31 19:13:37.000000000 +0200
@@ -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;
 
--- everything.orig/net/mac80211/sta_info.c	2008-03-31 18:25:59.000000000 +0200
+++ everything/net/mac80211/sta_info.c	2008-03-31 19:19:28.000000000 +0200
@@ -643,10 +643,41 @@ static void sta_info_debugfs_add_work(st
 }
 #endif
 
+void __ieee80211_run_pending_flush(struct ieee80211_local *local)
+{
+	struct sta_info *sta;
+	unsigned long flags;
+
+	ASSERT_RTNL();
+
+	spin_lock_irqsave(&local->sta_lock, flags);
+	while (!list_empty(&local->sta_flush_list)) {
+		sta = list_first_entry(&local->sta_flush_list,
+				       struct sta_info, list);
+		list_del(&sta->list);
+		spin_unlock_irqrestore(&local->sta_lock, flags);
+		sta_info_destroy(sta);
+		spin_lock_irqsave(&local->sta_lock, flags);
+	}
+	spin_unlock_irqrestore(&local->sta_lock, flags);
+}
+
+static void ieee80211_sta_flush_work(struct work_struct *work)
+{
+	struct ieee80211_local *local =
+		container_of(work, struct ieee80211_local, sta_flush_work);
+
+	rtnl_lock();
+	__ieee80211_run_pending_flush(local);
+	rtnl_unlock();
+}
+
 void sta_info_init(struct ieee80211_local *local)
 {
 	spin_lock_init(&local->sta_lock);
 	INIT_LIST_HEAD(&local->sta_list);
+	INIT_LIST_HEAD(&local->sta_flush_list);
+	INIT_WORK(&local->sta_flush_work, ieee80211_sta_flush_work);
 
 	setup_timer(&local->sta_cleanup, sta_info_cleanup,
 		    (unsigned long)local);
@@ -667,7 +698,12 @@ int sta_info_start(struct ieee80211_loca
 void sta_info_stop(struct ieee80211_local *local)
 {
 	del_timer(&local->sta_cleanup);
+	cancel_work_sync(&local->sta_flush_work);
+
+	rtnl_lock();
 	sta_info_flush(local, NULL);
+	__ieee80211_run_pending_flush(local);
+	rtnl_unlock();
 }
 
 /**
@@ -687,6 +723,7 @@ int sta_info_flush(struct ieee80211_loca
 	unsigned long flags;
 
 	might_sleep();
+	ASSERT_RTNL();
 
 	spin_lock_irqsave(&local->sta_lock, flags);
 	list_for_each_entry_safe(sta, tmp, &local->sta_list, list) {
@@ -705,3 +742,36 @@ int sta_info_flush(struct ieee80211_loca
 
 	return ret;
 }
+
+/**
+ * sta_info_flush_delayed - flush matching STA entries from the STA table
+ *
+ * This function unlinks all stations for a given interface and queues
+ * them for freeing. Note that the workqueue function scheduled here has
+ * to run before any new keys can be added to the system to avoid set_key()
+ * callback ordering issues.
+ *
+ * @sdata: the interface
+ */
+void sta_info_flush_delayed(struct ieee80211_sub_if_data *sdata)
+{
+	struct ieee80211_local *local = sdata->local;
+	struct sta_info *sta, *tmp;
+	unsigned long flags;
+	bool work = false;
+
+	spin_lock_irqsave(&local->sta_lock, flags);
+	list_for_each_entry_safe(sta, tmp, &local->sta_list, list) {
+		if (sdata == sta->sdata) {
+			__sta_info_unlink(&sta);
+			if (sta) {
+				list_add_tail(&sta->list,
+					      &local->sta_flush_list);
+				work = true;
+			}
+		}
+	}
+	if (work)
+		schedule_work(&local->sta_flush_work);
+	spin_unlock_irqrestore(&local->sta_lock, flags);
+}
--- everything.orig/net/mac80211/sta_info.h	2008-03-31 18:25:35.000000000 +0200
+++ everything/net/mac80211/sta_info.h	2008-03-31 19:19:48.000000000 +0200
@@ -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 */
--- everything.orig/net/mac80211/ieee80211_sta.c	2008-03-31 18:25:59.000000000 +0200
+++ everything/net/mac80211/ieee80211_sta.c	2008-03-31 19:13:38.000000000 +0200
@@ -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. */
--- everything.orig/net/mac80211/key.c	2008-03-31 19:18:04.000000000 +0200
+++ everything/net/mac80211/key.c	2008-03-31 19:21:10.000000000 +0200
@@ -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,

-- 

[PATCH 6/6] mac80211: fix sparse complaint in ieee80211_sta_def_wmm_params

[Johannes Berg]

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

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
Cc: Vladimir Koutny <vlado@work.ksp.sk>
---
 net/mac80211/ieee80211_sta.c |    1 -
 1 file changed, 1 deletion(-)

--- everything.orig/net/mac80211/ieee80211_sta.c	2008-03-31 18:22:26.000000000 +0200
+++ everything/net/mac80211/ieee80211_sta.c	2008-03-31 18:22:26.000000000 +0200
@@ -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));
 

-- 

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

[Johannes Berg]

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 <johannes@sipsolutions.net>
---
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(-)

--- everything.orig/net/mac80211/cfg.c	2008-04-01 15:17:15.000000000 +0200
+++ everything/net/mac80211/cfg.c	2008-04-01 15:19:23.000000000 +0200
@@ -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;
 	}
--- everything.orig/net/mac80211/ieee80211.c	2008-04-01 15:17:34.000000000 +0200
+++ everything/net/mac80211/ieee80211.c	2008-04-01 15:19:23.000000000 +0200
@@ -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;
--- everything.orig/net/mac80211/ieee80211_sta.c	2008-04-01 15:17:51.000000000 +0200
+++ everything/net/mac80211/ieee80211_sta.c	2008-04-01 15:19:23.000000000 +0200
@@ -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;
 }
--- everything.orig/net/mac80211/mesh_plink.c	2008-04-01 15:17:15.000000000 +0200
+++ everything/net/mac80211/mesh_plink.c	2008-04-01 15:19:23.000000000 +0200
@@ -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;
 		}
--- everything.orig/net/mac80211/sta_info.c	2008-04-01 15:17:52.000000000 +0200
+++ everything/net/mac80211/sta_info.c	2008-04-01 15:20:38.000000000 +0200
@@ -36,16 +36,23 @@
  * (which is pretty useless) or insert it into the hash table using
  * sta_info_insert() which demotes the reference from ownership to a regular
  * RCU-protected reference; if the function is called without protection by an
- * RCU critical section the reference is instantly invalidated.
+ * RCU critical section the reference is instantly invalidated. Note that the
+ * caller may not do much with the STA info before inserting it, in particular,
+ * it may not start any mesh peer link management or add encryption keys.
+ *
+ * When the insertion fails (sta_info_insert()) returns non-zero), the
+ * structure will have been freed by sta_info_insert()!
  *
  * Because there are debugfs entries for each station, and adding those
  * must be able to sleep, it is also possible to "pin" a station entry,
  * that means it can be removed from the hash table but not be freed.
- * See the comment in __sta_info_unlink() for more information.
+ * See the comment in __sta_info_unlink() for more information, this is
+ * an internal capability only.
  *
  * In order to remove a STA info structure, the caller needs to first
  * unlink it (sta_info_unlink()) from the list and hash tables and
- * then wait for an RCU synchronisation before it can be freed. Due to
+ * then destroy it while holding the RTNL; sta_info_destroy() will wait
+ * for an RCU grace period to elapse before actually freeing it. Due to
  * the pinning and the possibility of multiple callers trying to remove
  * the same STA info at the same time, sta_info_unlink() can clear the
  * STA info pointer it is passed to indicate that the STA info is owned
@@ -127,12 +134,35 @@ struct sta_info *sta_info_get_by_idx(str
 	return NULL;
 }
 
+/**
+ * __sta_info_free - internal STA free helper
+ *
+ * @sta: STA info to free
+ *
+ * This function must undo everything done by sta_info_alloc()
+ * that may happen before sta_info_insert().
+ */
+static void __sta_info_free(struct ieee80211_local *local,
+			    struct sta_info *sta)
+{
+	DECLARE_MAC_BUF(mbuf);
+
+	rate_control_free_sta(sta->rate_ctrl, sta->rate_ctrl_priv);
+	rate_control_put(sta->rate_ctrl);
+
+#ifdef CONFIG_MAC80211_VERBOSE_DEBUG
+	printk(KERN_DEBUG "%s: Destroyed STA %s\n",
+	       wiphy_name(local->hw.wiphy), print_mac(mbuf, sta->addr));
+#endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
+
+	kfree(sta);
+}
+
 void sta_info_destroy(struct sta_info *sta)
 {
 	struct ieee80211_local *local;
 	struct sk_buff *skb;
 	int i;
-	DECLARE_MAC_BUF(mbuf);
 
 	ASSERT_RTNL();
 	might_sleep();
@@ -182,15 +212,7 @@ void sta_info_destroy(struct sta_info *s
 		spin_unlock_bh(&sta->ampdu_mlme.ampdu_tx);
 	}
 
-	rate_control_free_sta(sta->rate_ctrl, sta->rate_ctrl_priv);
-	rate_control_put(sta->rate_ctrl);
-
-#ifdef CONFIG_MAC80211_VERBOSE_DEBUG
-	printk(KERN_DEBUG "%s: Destroyed STA %s\n",
-	       wiphy_name(local->hw.wiphy), print_mac(mbuf, sta->addr));
-#endif /* CONFIG_MAC80211_VERBOSE_DEBUG */
-
-	kfree(sta);
+	__sta_info_free(local, sta);
 }
 
 
@@ -266,6 +288,7 @@ int sta_info_insert(struct sta_info *sta
 	struct ieee80211_local *local = sta->local;
 	struct ieee80211_sub_if_data *sdata = sta->sdata;
 	unsigned long flags;
+	int err = 0;
 	DECLARE_MAC_BUF(mac);
 
 	/*
@@ -273,20 +296,23 @@ int sta_info_insert(struct sta_info *sta
 	 * something inserts a STA (on one CPU) without holding the RTNL
 	 * and another CPU turns off the net device.
 	 */
-	if (unlikely(!netif_running(sdata->dev)))
-		return -ENETDOWN;
-
-	if (WARN_ON(compare_ether_addr(sta->addr, sdata->dev->dev_addr) == 0))
-		return -EINVAL;
+	if (unlikely(!netif_running(sdata->dev))) {
+		err = -ENETDOWN;
+		goto out_free;
+	}
 
-	if (WARN_ON(is_multicast_ether_addr(sta->addr)))
-		return -EINVAL;
+	if (WARN_ON(compare_ether_addr(sta->addr, sdata->dev->dev_addr) == 0 ||
+	            is_multicast_ether_addr(sta->addr))) {
+		err = -EINVAL;
+		goto out_free;
+	}
 
 	spin_lock_irqsave(&local->sta_lock, flags);
 	/* check if STA exists already */
 	if (__sta_info_find(local, sta->addr)) {
 		spin_unlock_irqrestore(&local->sta_lock, flags);
-		return -EEXIST;
+		err = -EEXIST;
+		goto out_free;
 	}
 	list_add(&sta->list, &local->sta_list);
 	local->num_sta++;
@@ -309,9 +335,13 @@ int sta_info_insert(struct sta_info *sta
 	spin_unlock_irqrestore(&local->sta_lock, flags);
 
 #ifdef CONFIG_MAC80211_DEBUGFS
-	/* debugfs entry adding might sleep, so schedule process
+	/*
+	 * Debugfs entry adding might sleep, so schedule process
 	 * context task for adding entry for STAs that do not yet
-	 * have one. */
+	 * have one.
+	 * NOTE: due to auto-freeing semantics this may only be done
+	 *       if the insertion is successful!
+	 */
 	queue_work(local->hw.workqueue, &local->sta_debugfs_add);
 #endif
 
@@ -319,6 +349,10 @@ int sta_info_insert(struct sta_info *sta
 		mesh_accept_plinks_update(sdata);
 
 	return 0;
+ out_free:
+	BUG_ON(!err);
+ 	__sta_info_free(local, sta);
+	return err;
 }
 
 static inline void __bss_tim_set(struct ieee80211_if_ap *bss, u16 aid)

Re: [PATCH 0/6] sta-info fixes, especially destroy()

[Tomas Winkler]

On Mon, Mar 31, 2008 at 8:22 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> 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