[PATCH] d80211: Fix inconsistent sta_lock usage

[PATCH] d80211: Fix inconsistent sta_lock usage

[Jan Kiszka]

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

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 <jan.kiszka@web.de>

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

Index: rt2x00/ieee80211/ieee80211_ioctl.c
===================================================================
--- rt2x00.orig/ieee80211/ieee80211_ioctl.c
+++ 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;
Index: rt2x00/ieee80211/ieee80211_i.h
===================================================================
--- rt2x00.orig/ieee80211/ieee80211_i.h
+++ 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 */
Index: rt2x00/ieee80211/sta_info.c
===================================================================
--- rt2x00.orig/ieee80211/sta_info.c
+++ 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);
 }
 
 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Ivo Van Doorn]

On 1/1/07, Jan Kiszka <jan.kiszka@web.de> wrote:
> 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 <jan.kiszka@web.de>
>
> [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 <jan.kiszka@web.de>
Signed-off-by: Ivo van Doorn <IvDoorn@gmail.com>

---

diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
index ef303da..b132ae0 100644
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -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 */
diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
index c74b431..1363a01 100644
--- a/net/d80211/ieee80211_ioctl.c
+++ b/net/d80211/ieee80211_ioctl.c
@@ -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;
diff --git a/net/d80211/sta_info.c b/net/d80211/sta_info.c
index 0c42ae8..e120a4f 100644
--- a/net/d80211/sta_info.c
+++ b/net/d80211/sta_info.c
@@ -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);
 }

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Christoph Hellwig]

On Tue, Jan 02, 2007 at 04:30:41PM +0100, Ivo Van Doorn wrote:
> +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);

> +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

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Ivo van Doorn]

On Tuesday 02 January 2007 17:22, Christoph Hellwig wrote:
> On Tue, Jan 02, 2007 at 04:30:41PM +0100, Ivo Van Doorn wrote:
> > +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);
> 
> > +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 <jan.kiszka@web.de>
Signed-off-by Ivo van Doorn <IvDoorn@gmail.com>

---

diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
index 6ba6a61..c5c04d2 100644
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -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);
 }
 
diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
index 81a6e82..5a21b2d 100644
--- a/net/d80211/ieee80211_ioctl.c
+++ b/net/d80211/ieee80211_ioctl.c
@@ -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;
diff --git a/net/d80211/sta_info.c b/net/d80211/sta_info.c
index 0c42ae8..f27bd0e 100644
--- a/net/d80211/sta_info.c
+++ b/net/d80211/sta_info.c
@@ -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);
 }
 
 

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Jan Kiszka]

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

Ivo van Doorn wrote:
> +#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.

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

Probably forgotten: we need _bh here as well.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Johannes Berg]

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

On Fri, 2007-01-05 at 21:08 +0100, Ivo van Doorn wrote:

> 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

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

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Johannes Berg]

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

On Sat, 2007-01-06 at 17:52 +0100, Johannes Berg wrote:
> On Fri, 2007-01-05 at 21:08 +0100, Ivo van Doorn wrote:
> 
> > 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

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

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Jan Kiszka]

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

Johannes Berg wrote:
> On Fri, 2007-01-05 at 21:08 +0100, Ivo van Doorn wrote:
> 
>> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 249 bytes --]

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Johannes Berg]

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

On Sat, 2007-01-06 at 18:00 +0100, Jan Kiszka wrote:
> Johannes Berg wrote:
> > On Fri, 2007-01-05 at 21:08 +0100, Ivo van Doorn wrote:
> > 
> >> 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?

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

johannes

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

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Ivo Van Doorn]

> > 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 <jan.kiszka@web.de>
Signed-off-by Ivo van Doorn <IvDoorn@gmail.com>

---

diff --git a/net/d80211/ieee80211_i.h b/net/d80211/ieee80211_i.h
index ef303da..64d881c 100644
--- a/net/d80211/ieee80211_i.h
+++ b/net/d80211/ieee80211_i.h
@@ -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 */
diff --git a/net/d80211/ieee80211_ioctl.c b/net/d80211/ieee80211_ioctl.c
index c74b431..1363a01 100644
--- a/net/d80211/ieee80211_ioctl.c
+++ b/net/d80211/ieee80211_ioctl.c
@@ -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;
diff --git a/net/d80211/sta_info.c b/net/d80211/sta_info.c
index 0c42ae8..f27bd0e 100644
--- a/net/d80211/sta_info.c
+++ b/net/d80211/sta_info.c
@@ -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);
 }

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Jiri Benc]

On Sat, 6 Jan 2007 20:09:36 +0100, Ivo Van Doorn wrote:
> [...]
> @@ -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

-- 
Jiri Benc
SUSE Labs

Re: [PATCH] d80211: Fix inconsistent sta_lock usage

[Johannes Berg]

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

On Tue, 2007-01-02 at 16:22 +0000, Christoph Hellwig wrote:

> 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

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