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]>
Index: bu3sch-wireless-dev/net/mac80211/ieee80211_i.h
===================================================================
--- bu3sch-wireless-dev.orig/net/mac80211/ieee80211_i.h 2007-05-06 01:50:54.000000000 +0200
+++ bu3sch-wireless-dev/net/mac80211/ieee80211_i.h 2007-05-06 19:59:13.000000000 +0200
@@ -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;
Index: bu3sch-wireless-dev/net/mac80211/sta_info.c
===================================================================
--- bu3sch-wireless-dev.orig/net/mac80211/sta_info.c 2007-05-06 01:50:54.000000000 +0200
+++ bu3sch-wireless-dev/net/mac80211/sta_info.c 2007-05-06 20:22:20.000000000 +0200
@@ -193,9 +193,42 @@ struct sta_info * sta_info_add(struct ie
return sta;
}
+
+static void sta_info_key_disable(struct ieee80211_local *local,
+ struct sta_info *sta)
+{
+ might_sleep();
+ if (sta->key) {
+ if (local->ops->set_key) {
+ struct ieee80211_key_conf *key;
+ key = ieee80211_key_data2conf(local, sta->key);
+ if (key) {
+ local->ops->set_key(local_to_hw(local),
+ DISABLE_KEY,
+ sta->addr, key, sta->aid);
+ kfree(key);
+ }
+ }
+ } else if (sta->key_idx_compression != HW_KEY_IDX_INVALID) {
+ struct ieee80211_key_conf conf;
+ memset(&conf, 0, sizeof(conf));
+ conf.hw_key_idx = sta->key_idx_compression;
+ conf.alg = ALG_NULL;
+ conf.flags |= IEEE80211_KEY_FORCE_SW_ENCRYPT;
+ if (local->ops->set_key) {
+ local->ops->set_key(local_to_hw(local), DISABLE_KEY,
+ sta->addr, &conf, sta->aid);
+ }
+ sta->key_idx_compression = HW_KEY_IDX_INVALID;
+ }
+}
+
+
static void finish_sta_info_free(struct ieee80211_local *local,
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);
}
+
+static void finish_sta_info_free_work(struct work_struct *work)
+{
+ struct sta_info *sta =
+ container_of(work, struct sta_info, finish_free_work);
+
+ finish_sta_info_free(sta->local, sta);
+}
+
+
static void sta_info_remove(struct sta_info *sta)
{
struct ieee80211_local *local = sta->local;
@@ -230,6 +273,7 @@ static void sta_info_remove(struct sta_i
sta_info_remove_aid_ptr(sta);
}
+
void sta_info_free(struct sta_info *sta, int locked)
{
struct sk_buff *skb;
@@ -254,34 +298,11 @@ void sta_info_free(struct sta_info *sta,
dev_kfree_skb_any(skb);
}
- if (sta->key) {
- if (local->ops->set_key) {
- struct ieee80211_key_conf *key;
- key = ieee80211_key_data2conf(local, sta->key);
- if (key) {
- local->ops->set_key(local_to_hw(local),
- DISABLE_KEY,
- sta->addr, key, sta->aid);
- kfree(key);
- }
- }
- } else if (sta->key_idx_compression != HW_KEY_IDX_INVALID) {
- struct ieee80211_key_conf conf;
- memset(&conf, 0, sizeof(conf));
- conf.hw_key_idx = sta->key_idx_compression;
- conf.alg = ALG_NULL;
- conf.flags |= IEEE80211_KEY_FORCE_SW_ENCRYPT;
- local->ops->set_key(local_to_hw(local), DISABLE_KEY,
- sta->addr, &conf, sta->aid);
- sta->key_idx_compression = HW_KEY_IDX_INVALID;
- }
-
-#ifdef CONFIG_MAC80211_DEBUGFS
if (in_atomic()) {
- list_add(&sta->list, &local->deleted_sta_list);
- queue_work(local->hw.workqueue, &local->sta_debugfs_add);
+ /* Schedule the finishing to be able to sleep. */
+ INIT_WORK(&sta->finish_free_work, finish_sta_info_free_work);
+ queue_work(local->hw.workqueue, &sta->finish_free_work);
} else
-#endif
finish_sta_info_free(local, sta);
}
@@ -363,20 +384,6 @@ static void sta_info_debugfs_add_task(st
struct sta_info *sta, *tmp;
while (1) {
- spin_lock_bh(&local->sta_lock);
- if (!list_empty(&local->deleted_sta_list)) {
- sta = list_entry(local->deleted_sta_list.next,
- struct sta_info, list);
- list_del(local->deleted_sta_list.next);
- } else
- sta = NULL;
- spin_unlock_bh(&local->sta_lock);
- if (!sta)
- break;
- finish_sta_info_free(local, sta);
- }
-
- while (1) {
sta = NULL;
spin_lock_bh(&local->sta_lock);
list_for_each_entry(tmp, &local->sta_list, list) {
@@ -403,7 +410,6 @@ void sta_info_init(struct ieee80211_loca
{
spin_lock_init(&local->sta_lock);
INIT_LIST_HEAD(&local->sta_list);
- INIT_LIST_HEAD(&local->deleted_sta_list);
init_timer(&local->sta_cleanup);
local->sta_cleanup.expires = jiffies + STA_INFO_CLEANUP_INTERVAL;
Index: bu3sch-wireless-dev/net/mac80211/sta_info.h
===================================================================
--- bu3sch-wireless-dev.orig/net/mac80211/sta_info.h 2007-05-06 01:50:54.000000000 +0200
+++ bu3sch-wireless-dev/net/mac80211/sta_info.h 2007-05-06 19:58:58.000000000 +0200
@@ -133,6 +133,8 @@ struct sta_info {
#endif
} debugfs;
#endif
+
+ struct work_struct finish_free_work;
};
Index: bu3sch-wireless-dev/include/net/mac80211.h
===================================================================
--- bu3sch-wireless-dev.orig/include/net/mac80211.h 2007-05-01 22:32:36.000000000 +0200
+++ bu3sch-wireless-dev/include/net/mac80211.h 2007-05-06 20:29:26.000000000 +0200
@@ -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);
--
Greetings Michael.
On Sunday 06 May 2007 14:37, Michael Buesch wrote:
> 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
On Monday 07 May 2007 21:17:14 Michael Wu wrote:
> On Sunday 06 May 2007 14:37, Michael Buesch wrote:
> > 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.
--
Greetings Michael.
On Monday 14 May 2007 19:37:15 Jiri Benc wrote:
> On Mon, 7 May 2007 22:44:23 +0200, Michael Buesch wrote:
> > 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.
> >
> > 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.
> > 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.
> > 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?
I'm sorry, no.
--
Greetings Michael.
On Sun, 6 May 2007 20:37:34 +0200 Michael Buesch wrote:
> [...]
> static void finish_sta_info_free(struct ieee80211_local *local,
> 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
--
Jiri Benc
SUSE Labs
On Mon, 7 May 2007 22:44:23 +0200, Michael Buesch wrote:
> 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.
>
> 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 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?
> 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
--
Jiri Benc
SUSE Labs
On Monday 07 May 2007 18:52:24 Jiri Benc wrote:
> On Sun, 6 May 2007 20:37:34 +0200 Michael Buesch wrote:
> > [...]
> > static void finish_sta_info_free(struct ieee80211_local *local,
> > 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.
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).
--
Greetings Michael.