mac80211: Allow sleeping in set_key op

PATCH In review Michael Buesch 3 participants 7 messages May 6, 2007 – May 14, 2007 mac80211

Share & tools

Michael Buesch

author

2007-05-06 18:39 UTC #0 lore raw

PATCH 1/1 mac80211: Allow sleeping in set_key op

Allow drivers to sleep in the setkey function, as it's rather complicated to defer set_key to a workqueue in the driver (error code is lost, keyconf->hw_key_idx is possibly hard to get right.)

Signed-off-by Michael Buesch <[email protected]>

net/mac80211/ieee80211_i.h +0 -1

Index: bu3sch-wireless-dev/net/mac80211/ieee80211_i.h
===================================================================
@@ -420,7 +420,6 @@ struct ieee80211_local {
 	spinlock_t sta_lock; /* mutex for STA data structures */
 	int num_sta; /* number of stations in sta_list */
 	struct list_head sta_list;
-	struct list_head deleted_sta_list;
 	struct sta_info *sta_hash[STA_HASH_SIZE];
 	struct timer_list sta_cleanup;

net/mac80211/sta_info.c +47 ~~-41~~

129 lines not on this page · raw message

net/mac80211/sta_info.h +2 -0

Index: bu3sch-wireless-dev/net/mac80211/sta_info.h
===================================================================
@@ -133,6 +133,8 @@ struct sta_info {
 #endif
 	} debugfs;
 #endif
+
+	struct work_struct finish_free_work;
 };

include/net/mac80211.h +1 -2

Index: bu3sch-wireless-dev/include/net/mac80211.h
===================================================================
@@ -620,8 +620,7 @@ struct ieee80211_ops {
 	 * station hwaddr for individual keys. aid of the station is given
 	 * to help low-level driver in selecting which key->hw_key_idx to use
 	 * for this key. TX control data will use the hw_key_idx selected by
-	 * the low-level driver.
-	 * Must be atomic. */
+	 * the low-level driver. */
 	int (*set_key)(struct ieee80211_hw *hw, set_key_cmd cmd,
 		       u8 *addr, struct ieee80211_key_conf *key, int aid);

Signature · 1 line

Greetings Michael.

Jiri Benc

2007-05-07 16:52 UTC #1 lore raw

net/mac80211/sta_info.c static void finish_sta_info_free(struct · line 247

Quoted 12 lines · On Sun, 6 May 2007 20:37:34 +0200 Michael Buesch wrote:

[...]
static void finish_sta_info_free(struct ieee80211_local *local,

net/mac80211/sta_info.c +2 -0

 				 struct sta_info *sta)
 {
+	sta_info_key_disable(local, sta);
+
 #ifdef CONFIG_MAC80211_VERBOSE_DEBUG
 	printk(KERN_DEBUG "%s: Removed STA " MAC_FMT "\n",
 	       local->mdev->name, MAC_ARG(sta->addr));

@@ -213,6 +246,16 @@ static void finish_sta_info_free(struct 
 	sta_info_put(sta);
 }

There is a race here. You already removed the sta from sta_hash list and you're not protected by any lock. Thus, it is possible to add a new station with the same address before finish_sta_info_free is called. When this happens, you call the set_key handler for the new key and after that you call it again with DISABLE_KEY.

It's not easy to get this right. I remember also problems with dereferencing already freed key when I thought about possible ways to solve exactly this problem.

Thanks,

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs

Michael Buesch

author

2007-05-07 20:45 UTC #2 lore raw

Quoted 20 lines · On Monday 07 May 2007 18:52:24 Jiri Benc wrote:

net/mac80211/sta_info.c static void finish_sta_info_free(struct · line 247
Quoted 12 lines · On Sun, 6 May 2007 20:37:34 +0200 Michael Buesch wrote:
[...]
static void finish_sta_info_free(struct ieee80211_local *local,
net/mac80211/sta_info.c +2 -0
 				 struct sta_info *sta)
 {
+	sta_info_key_disable(local, sta);
+
 #ifdef CONFIG_MAC80211_VERBOSE_DEBUG
 	printk(KERN_DEBUG "%s: Removed STA " MAC_FMT "\n",
 	       local->mdev->name, MAC_ARG(sta->addr));
@@ -213,6 +246,16 @@ static void finish_sta_info_free(struct 
 	sta_info_put(sta);
 }
There is a race here. You already removed the sta from sta_hash list and you're not protected by any lock. Thus, it is possible to add a new station with the same address before finish_sta_info_free is called. When this happens, you call the set_key handler for the new key and after that you call it again with DISABLE_KEY.

Quoted 3 lines

It's not easy to get this right. I remember also problems with dereferencing already freed key when I thought about possible ways to solve exactly this problem.

I'm not sure this race exists.
You assume that when a new key is added with the same mac as before it overrides the old key. I think that's a bug in the driver then. IMO the driver _must_ keep track of used key "slots" and don't re-allocate them for new keys until disable-key is called. And that's exactly what bcm43xx does.

IMO it's a bug in the driver, when it overrides a key that's not DISABLEd.

Short: I don't think there is a race, and if, then it's a driver bug.

This is one of the showstopper bugs for bcm43xx to get merged. (There are others, though).

Signature · 1 line

Greetings Michael.

Jiri Benc

2007-05-14 17:37 UTC #3 lore raw

Quoted 7 lines · Michael Buesch

Quoted 6 lines · On Monday 07 May 2007 18:52:24 Jiri Benc wrote:
There is a race here. You already removed the sta from sta_hash list and you're not protected by any lock. Thus, it is possible to add a new station with the same address before finish_sta_info_free is called. When this happens, you call the set_key handler for the new key and after that you call it again with DISABLE_KEY.

Quoted 7 lines

Quoted 3 lines
It's not easy to get this right. I remember also problems with dereferencing already freed key when I thought about possible ways to solve exactly this problem.
I'm not sure this race exists.
You assume that when a new key is added with the same mac as before it overrides the old key.

Yes.

Quoted 3 lines

I think that's a bug in the driver then.
IMO the driver _must_ keep track of used key "slots" and don't re-allocate them for new keys until disable-key is called.

How the hardware will act if you supply two different keys for the same MAC address? And later, how do you recognize which of these two keys is being disabled?

Quoted 6 lines

And that's exactly what bcm43xx does.
IMO it's a bug in the driver, when it overrides a key that's not DISABLEd.
Short: I don't think there is a race, and if, then it's a driver bug.

I must admit I don't know how exactly keys are handled in all possible hardware. So I just want to be sure and keep exact serialization here. If there is a consensus that serialization is not needed here, well, it's easier then.

Do you know how other drivers work regarding this?

Thanks,

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs

Michael Buesch

author

2007-05-14 17:47 UTC #4 lore raw

Quoted 12 lines · On Monday 14 May 2007 19:37:15 Jiri Benc wrote:

Quoted 11 lines · Michael Buesch
Quoted 9 lines · On Monday 07 May 2007 18:52:24 Jiri Benc wrote:
There is a race here. You already removed the sta from sta_hash list and you're not protected by any lock. Thus, it is possible to add a new station with the same address before finish_sta_info_free is called. When this happens, you call the set_key handler for the new key and after that you call it again with DISABLE_KEY.
It's not easy to get this right. I remember also problems with dereferencing already freed key when I thought about possible ways to solve exactly this problem.

Quoted 5 lines

Quoted 3 lines
I'm not sure this race exists.
You assume that when a new key is added with the same mac as before it overrides the old key.
Yes.

I think that's an invalid assumption.
The key is per-bss and not per bssid-address; although that is mostly the same, except for such cornercases like this race.

Quoted 7 lines

Quoted 3 lines
I think that's a bug in the driver then.
IMO the driver _must_ keep track of used key "slots" and don't re-allocate them for new keys until disable-key is called.
How the hardware will act if you supply two different keys for the same MAC address? And later, how do you recognize which of these two keys is being disabled?

That's what hw_key_idx is for.

Quoted 5 lines

Quoted 5 lines
And that's exactly what bcm43xx does.
IMO it's a bug in the driver, when it overrides a key that's not DISABLEd.

Quoted 8 lines

Quoted 1 line
Short: I don't think there is a race, and if, then it's a driver bug.
I must admit I don't know how exactly keys are handled in all possible hardware. So I just want to be sure and keep exact serialization here. If there is a consensus that serialization is not needed here, well, it's easier then.
Do you know how other drivers work regarding this?

I'm sorry, no.

Signature · 1 line

Greetings Michael.

Michael Wu

2007-05-07 19:17 UTC #5 lore raw

In reply to Michael Buesch

Quoted 5 lines · Michael Buesch

Allow drivers to sleep in the setkey function, as it's rather complicated to defer set_key to a workqueue in the driver (error code is lost, keyconf->hw_key_idx is possibly hard to get right.)

I'd rather not defer to a workqueue. That just patches over the awkward locking that the sta_info code has, which is really the reason why you can't sleep in set_key.

-Michael Wu

Michael Buesch

author

2007-05-07 20:50 UTC #6 lore raw

Quoted 9 lines · On Monday 07 May 2007 21:17:14 Michael Wu wrote:

Quoted 5 lines · Michael Buesch
Allow drivers to sleep in the setkey function, as it's rather complicated to defer set_key to a workqueue in the driver (error code is lost, keyconf->hw_key_idx is possibly hard to get right.)
I'd rather not defer to a workqueue. That just patches over the awkward locking that the sta_info code has, which is really the reason why you can't sleep in set_key.

Well, but it seems nobody is fixing the locking. I was told it's almost impossible or something like that. ;)
This is just trying to fix showstopper bugs for bcm43xx.

Signature · 1 line

Greetings Michael.