d80211: Fix inconsistent sta_lock usage

PATCH In review Jan Kiszka 5 participants 12 messages Jan 1, 2007 – Jan 10, 2007

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Jan Kiszka

author

2007-01-01 20:49 UTC #0 lore raw

PATCH 1/1 d80211: Fix inconsistent sta_lock usage

Hacking a bit on rt2x00 to make it work in master and ad-hoc mode, lockdep popped up on some hostapd ioctls, pointing out remaining inconsistencies related to sta_lock:

1. sta_lock holders must always be protected against softirq
2. bss_tim_set/clear must not be called with sta_lock held, rather an unprotected variant
3. ieee80211_ioctl_remove_sta is not already holding the lock when calling sta_info_free

As I was not sure if sta_info_remove_aid_ptr needs lock protection or not, I played safe and moved it always under the lock. Please correct me if this is overkill.

Signed-off-by Jan Kiszka <[email protected]>

[Sorry, patch is against rt2x00 CVS. I'm lacking time and bandwidth to pull the d80211 git repos and rebase.]

---
 ieee80211/ieee80211_i.h     |   24 ++++++++++++++++++------
 ieee80211/ieee80211_ioctl.c |    4 +++-
 ieee80211/sta_info.c        |    2 +-
 3 files changed, 22 insertions(+), 8 deletions(-)

ieee80211/ieee80211_ioctl.c +3 -1

Index: rt2x00/ieee80211/ieee80211_ioctl.c
===================================================================
@@ -286,7 +286,9 @@ static int ieee80211_ioctl_add_sta(struc
 	if (sta->dev != dev) {
 		/* Binding STA to a new interface, so remove all references to
 		 * the old BSS. */
+		spin_lock_bh(&local->sta_lock);
 		sta_info_remove_aid_ptr(sta);
+		spin_unlock_bh(&local->sta_lock);
 	}
 
         /* TODO

@@ -360,7 +362,7 @@ static int ieee80211_ioctl_remove_sta(st
 	sta = sta_info_get(local, param->sta_addr);
 	if (sta) {
 		sta_info_put(sta);
-		sta_info_free(sta, 1);
+		sta_info_free(sta, 0);
 	}
 
 	return sta ? 0 : -ENOENT;

ieee80211/ieee80211_i.h +18 -6

Index: rt2x00/ieee80211/ieee80211_i.h
===================================================================
@@ -565,20 +565,32 @@ struct sta_attribute {
 	ssize_t (*store)(struct sta_info *, const char *buf, size_t count);
 };
 
+static inline void __bss_tim_set(struct ieee80211_local *local,
+				 struct ieee80211_if_ap *bss, int aid)
+{
+	bss->tim[(aid)/8] |= 1<<((aid) % 8);
+}
+
 static inline void bss_tim_set(struct ieee80211_local *local,
 			       struct ieee80211_if_ap *bss, int aid)
 {
-	spin_lock(&local->sta_lock);
-	bss->tim[(aid)/8] |= 1<<((aid) % 8);
-	spin_unlock(&local->sta_lock);
+	spin_lock_bh(&local->sta_lock);
+	__bss_tim_set(local, bss, aid);
+	spin_unlock_bh(&local->sta_lock);
+}
+
+static inline void __bss_tim_clear(struct ieee80211_local *local,
+				   struct ieee80211_if_ap *bss, int aid)
+{
+	bss->tim[(aid)/8] &= !(1<<((aid) % 8));
 }
 
 static inline void bss_tim_clear(struct ieee80211_local *local,
 				 struct ieee80211_if_ap *bss, int aid)
 {
-	spin_lock(&local->sta_lock);
-	bss->tim[(aid)/8] &= !(1<<((aid) % 8));
-	spin_unlock(&local->sta_lock);
+	spin_lock_bh(&local->sta_lock);
+	__bss_tim_clear(local, bss, aid);
+	spin_unlock_bh(&local->sta_lock);
 }
 
 /* ieee80211.c */

ieee80211/sta_info.c +1 -1

Index: rt2x00/ieee80211/sta_info.c
===================================================================
@@ -439,7 +439,7 @@ void sta_info_remove_aid_ptr(struct sta_
 		sdata->local->ops->set_tim(local_to_hw(sdata->local),
 					  sta->aid, 0);
 	if (sdata->bss)
-		bss_tim_clear(sdata->local, sdata->bss, sta->aid);
+		__bss_tim_clear(sdata->local, sdata->bss, sta->aid);
 }

Ivo Van Doorn

2007-01-02 15:30 UTC #1 lore raw

Quoted 10 lines · Jan Kiszka

Hacking a bit on rt2x00 to make it work in master and ad-hoc mode, lockdep popped up on some hostapd ioctls, pointing out remaining inconsistencies related to sta_lock:
1. sta_lock holders must always be protected against softirq
2. bss_tim_set/clear must not be called with sta_lock held, rather an unprotected variant
3. ieee80211_ioctl_remove_sta is not already holding the lock when calling sta_info_free

Quoted 8 lines

As I was not sure if sta_info_remove_aid_ptr needs lock protection or not, I played safe and moved it always under the lock. Please correct me if this is overkill.
Signed-off-by Jan Kiszka <[email protected]>
[Sorry, patch is against rt2x00 CVS. I'm lacking time and bandwidth to pull the d80211 git repos and rebase.]

To make it easier for everybody, here is the same patch only this time applied to the dscape git tree. ;)

Signed-off-by Jan Kiszka <[email protected]>

Signed-off-by Ivo van Doorn <[email protected]>

---

net/d80211/ieee80211_i.h +0 -0

index ef303da..b132ae0 100644
@@ -558,20 +558,32 @@ struct sta_attribute {

	ssize_t (*store)(struct sta_info *, const char *buf, size_t count);
};

+static inline void __bss_tim_set(struct ieee80211_local *local,
+ struct ieee80211_if_ap *bss, int aid)
+{
+ bss->tim[(aid)/8] |= 1<<((aid) % 8);
+}
+
static inline void bss_tim_set(struct ieee80211_local *local,

			       struct ieee80211_if_ap *bss, int aid)

{
- spin_lock(&local->sta_lock);
- bss->tim[(aid)/8] |= 1<<((aid) % 8);
- spin_unlock(&local->sta_lock);
+ spin_lock_bh(&local->sta_lock);
+ __bss_tim_set(local, bss, aid);
+ spin_unlock_bh(&local->sta_lock);
+}
+
+static inline void __bss_tim_clear(struct ieee80211_local *local,
+ struct ieee80211_if_ap *bss, int aid)
+{
+ bss->tim[(aid)/8] &= !(1<<((aid) % 8));
}

static inline void bss_tim_clear(struct ieee80211_local *local,

				 struct ieee80211_if_ap *bss, int aid)

{
- spin_lock(&local->sta_lock);
- bss->tim[(aid)/8] &= !(1<<((aid) % 8));
- spin_unlock(&local->sta_lock);
+ spin_lock_bh(&local->sta_lock);
+ __bss_tim_clear(local, bss, aid);
+ spin_unlock_bh(&local->sta_lock);
}

/* ieee80211.c */

net/d80211/ieee80211_ioctl.c +0 -0

index c74b431..1363a01 100644
@@ -285,7 +285,9 @@ static int ieee80211_ioctl_add_sta(struct net_device *dev,

	if (sta->dev != dev) {
		/* Binding STA to a new interface, so remove all references to
		 * the old BSS. */
+		spin_lock_bh(&local->sta_lock);
		sta_info_remove_aid_ptr(sta);
+		spin_unlock_bh(&local->sta_lock);
	}

        /* TODO
@@ -359,7 +361,7 @@ static int ieee80211_ioctl_remove_sta(struct
net_device *dev,
	sta = sta_info_get(local, param->sta_addr);
	if (sta) {
		sta_info_put(sta);
-		sta_info_free(sta, 1);
+		sta_info_free(sta, 0);
	}

	return sta ? 0 : -ENOENT;

net/d80211/sta_info.c +0 -0

index 0c42ae8..e120a4f 100644
@@ -439,7 +439,7 @@ void sta_info_remove_aid_ptr(struct sta_info *sta)

		sdata->local->ops->set_tim(local_to_hw(sdata->local),
					  sta->aid, 0);
	if (sdata->bss)

- bss_tim_clear(sdata->local, sdata->bss, sta->aid);
+ __bss_tim_clear(sdata->local, sdata->bss, sta->aid);
}

Christoph Hellwig

2007-01-02 16:22 UTC #2 lore raw

Review

Quoted 5 lines · Ivo Van Doorn

(quoted diff) +5 -0

+static inline void __bss_tim_set(struct ieee80211_local *local,
+				 struct ieee80211_if_ap *bss, int aid)
+{
+	bss->tim[(aid)/8] |= 1<<((aid) % 8);
+}

This really screams to be converted to __set_bit. Also the local argument is entirely unused. I'd probaby not even add a helper for this but just opencode it as:

	__set_bit(&bss->time, aid);

Review

Quoted 4 lines

(quoted diff) +4 -0

+static inline void __bss_tim_clear(struct ieee80211_local *local,
+				   struct ieee80211_if_ap *bss, int aid)
+{
+	bss->tim[(aid)/8] &= !(1<<((aid) % 8));

Similarly this should just be __clear_bit

Ivo van Doorn

2007-01-05 20:09 UTC #3 lore raw

Quoted 11 lines · Christoph Hellwig

Review
Quoted 5 lines · Ivo Van Doorn
(quoted diff) +5 -0
+static inline void __bss_tim_set(struct ieee80211_local *local,
+				 struct ieee80211_if_ap *bss, int aid)
+{
+	bss->tim[(aid)/8] |= 1<<((aid) % 8);
+}
This really screams to be converted to __set_bit. Also the local argument is entirely unused. I'd probaby not even add a helper for this but just opencode it as:

Quoted 8 lines

	__set_bit(&bss->time, aid);

Review

Quoted 4 lines

(quoted diff) +4 -0

+static inline void __bss_tim_clear(struct ieee80211_local *local,
+				   struct ieee80211_if_ap *bss, int aid)
+{
+	bss->tim[(aid)/8] &= !(1<<((aid) % 8));

Similarly this should just be __clear_bit

This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused.

Signed-off-by Jan Kiszka <[email protected]>

Signed-off-by Ivo van Doorn <[email protected]>

---

net/d80211/ieee80211_i.h +8 -4

index 6ba6a61..c5c04d2 100644
@@ -560,19 +560,23 @@ struct sta_attribute {
 	ssize_t (*store)(struct sta_info *, const char *buf, size_t count);
 };
 
+#define __bss_tim_set(__bss, __aid)	__set_bit((__aid), &(__bss)->tim)
+
 static inline void bss_tim_set(struct ieee80211_local *local,
 			       struct ieee80211_if_ap *bss, int aid)
 {
-	spin_lock(&local->sta_lock);
-	bss->tim[(aid)/8] |= 1<<((aid) % 8);
-	spin_unlock(&local->sta_lock);
+	spin_lock_bh(&local->sta_lock);
+	__bss_tim_set(bss, aid);
+	spin_unlock_bh(&local->sta_lock);
 }
 
+#define __bss_tim_clear(__bss, __aid)	__clear_bit((__aid), &(__bss)->tim)
+
 static inline void bss_tim_clear(struct ieee80211_local *local,
 				 struct ieee80211_if_ap *bss, int aid)
 {
 	spin_lock(&local->sta_lock);
-	bss->tim[(aid)/8] &= !(1<<((aid) % 8));
+	__bss_tim_clear(bss, aid);
 	spin_unlock(&local->sta_lock);
 }

net/d80211/ieee80211_ioctl.c +3 -1

index 81a6e82..5a21b2d 100644
@@ -286,7 +286,9 @@ static int ieee80211_ioctl_add_sta(struct net_device *dev,
 	if (sta->dev != dev) {
 		/* Binding STA to a new interface, so remove all references to
 		 * the old BSS. */
+		spin_lock_bh(&local->sta_lock);
 		sta_info_remove_aid_ptr(sta);
+		spin_unlock_bh(&local->sta_lock);
 	}
 
         /* TODO

@@ -360,7 +362,7 @@ static int ieee80211_ioctl_remove_sta(struct net_device *dev,
 	sta = sta_info_get(local, param->sta_addr);
 	if (sta) {
 		sta_info_put(sta);
-		sta_info_free(sta, 1);
+		sta_info_free(sta, 0);
 	}
 
 	return sta ? 0 : -ENOENT;

net/d80211/sta_info.c +1 -1

index 0c42ae8..f27bd0e 100644
@@ -439,7 +439,7 @@ void sta_info_remove_aid_ptr(struct sta_info *sta)
 		sdata->local->ops->set_tim(local_to_hw(sdata->local),
 					  sta->aid, 0);
 	if (sdata->bss)
-		bss_tim_clear(sdata->local, sdata->bss, sta->aid);
+		__bss_tim_clear(sdata->bss, sta->aid);
 }

Jan Kiszka

author

2007-01-06 16:33 UTC #4 lore raw

Review

Quoted 2 lines · Ivo van Doorn

(quoted diff) +2 -0

+#define __bss_tim_set(__bss, __aid)	__set_bit((__aid), &(__bss)->tim)
+

__set/clear_bit demands unsigned long, tim is u8. That causes quite some warnings here.

...

net/d80211/ieee80211_i.h struct sta_attribute { · line 580

Quoted 7 lines

static inline void bss_tim_clear(struct ieee80211_local *local,

net/d80211/ieee80211_i.h +1 -1

 				 struct ieee80211_if_ap *bss, int aid)
 {
 	spin_lock(&local->sta_lock);
-	bss->tim[(aid)/8] &= !(1<<((aid) % 8));
+	__bss_tim_clear(bss, aid);
 	spin_unlock(&local->sta_lock);

Probably forgotten: we need _bh here as well.

Jan

Johannes Berg

2007-01-06 16:53 UTC #5 lore raw

In reply to Ivo van Doorn

Quoted 2 lines · Ivo van Doorn

This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused.

NACK. This breaks spec compliance for drivers that use the TIM in their beacon frames.

johannes

Johannes Berg

2007-01-06 16:59 UTC #6 lore raw

Quoted 7 lines · Johannes Berg

Quoted 2 lines · Ivo van Doorn
This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused.
NACK. This breaks spec compliance for drivers that use the TIM in their beacon frames.

Which, in fact, is the only point we keep track of the TIM, note that we never actually test if bits there are set, the stack has better ways of keeping track of the pending traffic.

johannes

Jan Kiszka

author

2007-01-06 17:00 UTC #7 lore raw

In reply to Johannes Berg

Quoted 7 lines · Johannes Berg

Quoted 2 lines · Ivo van Doorn
This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused.
NACK. This breaks spec compliance for drivers that use the TIM in their beacon frames.

Bit ordering, I see. Then go for my original patch + comments why this is open-coded in __bss_tim_set/clear + removed unused arguments from those functions, OK?

Jan

Johannes Berg

2007-01-06 17:02 UTC #8 lore raw

Quoted 6 lines · Jan Kiszka

Quoted 5 lines · Johannes Berg
Quoted 2 lines · Ivo van Doorn
This patch uses the __set_bit and __clear_but as suggested by Christoph. It also removes the local argument since it was unused.

Quoted 6 lines

Quoted 2 lines
NACK. This breaks spec compliance for drivers that use the TIM in their beacon frames.
Bit ordering, I see. Then go for my original patch + comments why this is open-coded in __bss_tim_set/clear + removed unused arguments from those functions, OK?

Sounds good to me, care to send a new patch?

johannes

Ivo Van Doorn

2007-01-06 19:09 UTC #9 lore raw

Quoted 5 lines

Quoted 3 lines
Bit ordering, I see. Then go for my original patch + comments why this is open-coded in __bss_tim_set/clear + removed unused arguments from those functions, OK?
Sounds good to me, care to send a new patch?

This patch returns to the original format for setting and clearing the tim bit, as the IEEE specs mandate. (Comment has been added to prevent future attempts to use the __set_bit and __clear_bit)
And the local argument has been removed.

Signed-off-by Jan Kiszka <[email protected]>
Signed-off-by Ivo van Doorn <[email protected]>

---

net/d80211/ieee80211_i.h +0 -0

index ef303da..64d881c 100644
@@ -558,20 +558,38 @@ struct sta_attribute {

	ssize_t (*store)(struct sta_info *, const char *buf, size_t count);
};

+static inline void __bss_tim_set(struct ieee80211_if_ap *bss, int aid) +{
+ /*
+ * This format has ben mandated by the IEEE specifications,
+ * so this line may not be changed to use the __set_bit() format.
+ */
+ bss->tim[(aid)/8] |= 1<<((aid) % 8);
+}
+
static inline void bss_tim_set(struct ieee80211_local *local,

			       struct ieee80211_if_ap *bss, int aid)

{
- spin_lock(&local->sta_lock);
- bss->tim[(aid)/8] |= 1<<((aid) % 8);
- spin_unlock(&local->sta_lock);
+ spin_lock_bh(&local->sta_lock);
+ __bss_tim_set(bss, aid);
+ spin_unlock_bh(&local->sta_lock);
+}
+
+static inline void __bss_tim_clear(struct ieee80211_if_ap *bss, int aid) +{
+ /*
+ * This format has ben mandated by the IEEE specifications,
+ * so this line may not be changed to use the __clear_bit() format.
+ */
+ bss->tim[(aid)/8] &= !(1<<((aid) % 8));
}

static inline void bss_tim_clear(struct ieee80211_local *local,

				 struct ieee80211_if_ap *bss, int aid)

{
- spin_lock(&local->sta_lock);
- bss->tim[(aid)/8] &= !(1<<((aid) % 8));
- spin_unlock(&local->sta_lock);
+ spin_lock_bh(&local->sta_lock);
+ __bss_tim_clear(bss, aid);
+ spin_unlock_bh(&local->sta_lock);
}

/* ieee80211.c */

net/d80211/ieee80211_ioctl.c +0 -0

index c74b431..1363a01 100644
@@ -285,7 +285,9 @@ static int ieee80211_ioctl_add_sta(struct net_device *dev,

	if (sta->dev != dev) {
		/* Binding STA to a new interface, so remove all references to
		 * the old BSS. */
+		spin_lock_bh(&local->sta_lock);
		sta_info_remove_aid_ptr(sta);
+		spin_unlock_bh(&local->sta_lock);
	}

        /* TODO
@@ -359,7 +361,7 @@ static int ieee80211_ioctl_remove_sta(struct
net_device *dev,
	sta = sta_info_get(local, param->sta_addr);
	if (sta) {
		sta_info_put(sta);
-		sta_info_free(sta, 1);
+		sta_info_free(sta, 0);
	}

	return sta ? 0 : -ENOENT;

net/d80211/sta_info.c +0 -0

index 0c42ae8..f27bd0e 100644
@@ -439,7 +439,7 @@ void sta_info_remove_aid_ptr(struct sta_info *sta)

		sdata->local->ops->set_tim(local_to_hw(sdata->local),
					  sta->aid, 0);
	if (sdata->bss)

- bss_tim_clear(sdata->local, sdata->bss, sta->aid);
+ __bss_tim_clear(sdata->bss, sta->aid);
}

Jiri Benc

2007-01-10 20:13 UTC #10 lore raw

Review static int ieee80211_ioctl_remove_sta(struct · line 361

Quoted 3 lines · Ivo Van Doorn

[...]

(quoted diff) +0 -0

@@ -359,7 +361,7 @@ static int ieee80211_ioctl_remove_sta(struct
 net_device *dev,

The patch is line-wrapped here. Applied, but please try to fix your mail client settings.

Thanks for the patch,

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs

Johannes Berg

2007-01-06 16:52 UTC #11 lore raw

In reply to Christoph Hellwig

Quoted 1 line · Christoph Hellwig

This really screams to be converted to __set_bit.

Whoops, I should really have jumped into this thread earlier but somehow missed it.

This cannot be converted to __set_bit because the IEEE specs mandate this format.

We can insert a comment that the format must stay like this :)

You have to realise that the TIM is sent as-is with the 0th bit indicating multicast traffic (IIRC) and each of the other bits indicating traffic for that AID.

johannes