[PATCH] mac80211: check for existance sta before adding it

[PATCH] mac80211: check for existance sta before adding it

[Luis R. Rodriguez]

Lets just check for the STA before its addition through
sta_info_insert() and make this mandatory. It simpifies
code.

In our mac80211 cfg80211 callback for device addition we
also can simplify the code by first checking for the STA
before trying to add it and then checking for -EEXIST which
we were not doing. If that actualy would happen we could
end up potentially with a stale sta and the rate info was
never updated. We should now be updating accordingly.

Since we are making part of the API to check for the STA
prior to addition lets warn in sta_info_insert() if the
sta does already exist instead of passing an -EEXIST.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---

johill, this is what I meant. Still not the right approach?

 net/mac80211/cfg.c      |   33 ++++++++++++++++++---------------
 net/mac80211/mlme.c     |    4 ++++
 net/mac80211/sta_info.c |   18 ++++++++++++++----
 3 files changed, 36 insertions(+), 19 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 77e9ff5..657e6b4 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -717,6 +717,7 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
 	struct ieee80211_sub_if_data *sdata;
 	int err;
 	int layer2_update;
+	bool new_sta = true;
 
 	if (params->vlan) {
 		sdata = IEEE80211_DEV_TO_SUB_IF(params->vlan);
@@ -733,9 +734,18 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
 	if (is_multicast_ether_addr(mac))
 		return -EINVAL;
 
-	sta = sta_info_alloc(sdata, mac, GFP_KERNEL);
-	if (!sta)
-		return -ENOMEM;
+	rcu_read_lock();
+
+	sta = sta_info_get(local, mac);
+
+	if (!sta) {
+		sta = sta_info_alloc(sdata, mac, GFP_KERNEL);
+		if (!sta) {
+			rcu_read_unlock();
+			return -ENOMEM;
+		}
+	} else
+		new_sta = false;
 
 	sta->flags = WLAN_STA_AUTH | WLAN_STA_ASSOC;
 
@@ -746,19 +756,12 @@ static int ieee80211_add_station(struct wiphy *wiphy, struct net_device *dev,
 	layer2_update = sdata->vif.type == NL80211_IFTYPE_AP_VLAN ||
 		sdata->vif.type == NL80211_IFTYPE_AP;
 
-	rcu_read_lock();
-
-	err = sta_info_insert(sta);
-	if (err) {
-		/* STA has been freed */
-		if (err == -EEXIST && layer2_update) {
-			/* Need to update layer 2 devices on reassociation */
-			sta = sta_info_get(local, mac);
-			if (sta)
-				ieee80211_send_layer2_update(sta);
+	if (new_sta) {
+		err = sta_info_insert(sta);
+		if (err) {
+			rcu_read_unlock();
+			return err;
 		}
-		rcu_read_unlock();
-		return err;
 	}
 
 	if (layer2_update)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 509469c..ef79823 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -1782,6 +1782,10 @@ static void ieee80211_rx_mgmt_assoc_resp(struct ieee80211_sub_if_data *sdata,
 
 	if (newsta) {
 		int err = sta_info_insert(sta);
+		/*
+		 * Since we checked earlier for this STA and we're under RCU
+		 * this should not happen.
+		 */
 		if (err) {
 			printk(KERN_DEBUG "%s: failed to insert STA entry for"
 			       " the AP (error %d)\n", sdata->dev->name, err);
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index d5611d8..c64a277 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -42,7 +42,9 @@
  * 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()!
+ * structure will have been freed by sta_info_insert()! You _must_
+ * call sta_info_insert() only prior to first checking if the sta already
+ * exists with sta_info_get().
  *
  * 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,
@@ -309,6 +311,11 @@ struct sta_info *sta_info_alloc(struct ieee80211_sub_if_data *sdata,
 	return sta;
 }
 
+/*
+ * Must be called with rcu_read_lock() held, you should also
+ * first check if the STA exists first prior to calling
+ * this routine to add it.
+ */
 int sta_info_insert(struct sta_info *sta)
 {
 	struct ieee80211_local *local = sta->local;
@@ -333,10 +340,13 @@ int sta_info_insert(struct sta_info *sta)
 	}
 
 	spin_lock_irqsave(&local->sta_lock, flags);
-	/* check if STA exists already */
-	if (sta_info_get(local, sta->sta.addr)) {
+	/*
+	 * check if STA exists already - callers should have
+	 * already done this while holding the RCU lock
+	 */
+	if (WARN_ON(sta_info_get(local, sta->sta.addr))) {
 		spin_unlock_irqrestore(&local->sta_lock, flags);
-		err = -EEXIST;
+		err = -EIO;
 		goto out_free;
 	}
 	list_add(&sta->list, &local->sta_list);
-- 
1.6.0.6

Re: [PATCH] mac80211: check for existance sta before adding it

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 1119 bytes --]

On Tue, 2009-06-02 at 16:30 -0400, Luis R. Rodriguez wrote:
> Lets just check for the STA before its addition through
> sta_info_insert() and make this mandatory. It simpifies
> code.
> 
> In our mac80211 cfg80211 callback for device addition we
> also can simplify the code by first checking for the STA
> before trying to add it and then checking for -EEXIST which
> we were not doing. If that actualy would happen we could
> end up potentially with a stale sta and the rate info was
> never updated. We should now be updating accordingly.
> 
> Since we are making part of the API to check for the STA
> prior to addition lets warn in sta_info_insert() if the
> sta does already exist instead of passing an -EEXIST.
> 
> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
> ---
> 
> johill, this is what I meant. Still not the right approach?

This may sound flippy, but you do need to understand RCU first. This is
simply not correct, even with rcu_read_lock() the sta info might be
unlinked from the list, your pointer will just point to something no
longer on the list.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] mac80211: check for existance sta before adding it

[Luis R. Rodriguez]

On Tue, Jun 2, 2009 at 1:35 PM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Tue, 2009-06-02 at 16:30 -0400, Luis R. Rodriguez wrote:
>> Lets just check for the STA before its addition through
>> sta_info_insert() and make this mandatory. It simpifies
>> code.
>>
>> In our mac80211 cfg80211 callback for device addition we
>> also can simplify the code by first checking for the STA
>> before trying to add it and then checking for -EEXIST which
>> we were not doing. If that actualy would happen we could
>> end up potentially with a stale sta and the rate info was
>> never updated. We should now be updating accordingly.
>>
>> Since we are making part of the API to check for the STA
>> prior to addition lets warn in sta_info_insert() if the
>> sta does already exist instead of passing an -EEXIST.
>>
>> Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
>> ---
>>
>> johill, this is what I meant. Still not the right approach?
>
> This may sound flippy, but you do need to understand RCU first. This is
> simply not correct, even with rcu_read_lock() the sta info might be
> unlinked from the list, your pointer will just point to something no
> longer on the list.

Thanks, yeah I see where I was making an incorrect assumption here.

There still is a case here where userpsace can try to add a sta twice
and the second parameters passed would be ignored.

  Luis

Re: [PATCH] mac80211: check for existance sta before adding it

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

On Tue, 2009-06-02 at 14:01 -0700, Luis R. Rodriguez wrote:

> Thanks, yeah I see where I was making an incorrect assumption here.
> 
> There still is a case here where userpsace can try to add a sta twice
> and the second parameters passed would be ignored.

Don't we show that -EEXIST to userspace then? I thought we did.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] mac80211: check for existance sta before adding it

[Luis R. Rodriguez]

On Wed, Jun 03, 2009 at 12:00:50AM -0700, Johannes Berg wrote:
> On Tue, 2009-06-02 at 14:01 -0700, Luis R. Rodriguez wrote:
> 
> > Thanks, yeah I see where I was making an incorrect assumption here.
> > 
> > There still is a case here where userpsace can try to add a sta twice
> > and the second parameters passed would be ignored.
> 
> Don't we show that -EEXIST to userspace then? I thought we did.

We do, but why don't we *first* try to check for its existence?

  Luis

Re: [PATCH] mac80211: check for existance sta before adding it

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

On Wed, 2009-06-03 at 11:08 -0700, Luis R. Rodriguez wrote:
> On Wed, Jun 03, 2009 at 12:00:50AM -0700, Johannes Berg wrote:
> > On Tue, 2009-06-02 at 14:01 -0700, Luis R. Rodriguez wrote:
> > 
> > > Thanks, yeah I see where I was making an incorrect assumption here.
> > > 
> > > There still is a case here where userpsace can try to add a sta twice
> > > and the second parameters passed would be ignored.
> > 
> > Don't we show that -EEXIST to userspace then? I thought we did.
> 
> We do, but why don't we *first* try to check for its existence?

Because that's racy -- after the check completes and before we go try to
insert a station might have come to life. Well, it might actually be ok
due to external factors (AP mode _only_ gets stations from userspace)
but it seems better to not rely on that.

What problem were you trying to solve btw?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH] mac80211: check for existance sta before adding it

[Luis R. Rodriguez]

On Wed, Jun 03, 2009 at 11:23:43AM -0700, Johannes Berg wrote:
> On Wed, 2009-06-03 at 11:08 -0700, Luis R. Rodriguez wrote:
> > On Wed, Jun 03, 2009 at 12:00:50AM -0700, Johannes Berg wrote:
> > > On Tue, 2009-06-02 at 14:01 -0700, Luis R. Rodriguez wrote:
> > > 
> > > > Thanks, yeah I see where I was making an incorrect assumption here.
> > > > 
> > > > There still is a case here where userpsace can try to add a sta twice
> > > > and the second parameters passed would be ignored.
> > > 
> > > Don't we show that -EEXIST to userspace then? I thought we did.
> > 
> > We do, but why don't we *first* try to check for its existence?
> 
> Because that's racy -- after the check completes and before we go try to
> insert a station might have come to life. Well, it might actually be ok
> due to external factors (AP mode _only_ gets stations from userspace)
> but it seems better to not rely on that.

I see.

> What problem were you trying to solve btw?

Nothing in particular, I was just reviewing sta code path.

  Luis