Yogesh Ashok Powar (2):
Revert "Revert "mac80211: Skip tailroom reservation for full
HW-crypto devices""
mac80211: Fixing Races for skipping tailroom reservation
net/mac80211/ieee80211_i.h | 7 ++++++
net/mac80211/key.c | 46 ++++++++++++++++++++++++++++++++++++++++++-
net/mac80211/main.c | 2 +
net/mac80211/tx.c | 13 ++++++-----
4 files changed, 60 insertions(+), 8 deletions(-)
--
1.7.5.4
>What I'm worried about is that there's no memory barrier after the
>counter update, so how do we know it cannot happen after
>synchronize_rcu()? I think we need something like rcu_assign_pointer()
>but I don't see rcu_assign_index() (any more?)
I did not find much on rcu_assign_index other than
http://markmail.org/thread/h53fuecyo5odg6xv which has this function defined as
+#define rcu_assign_index(p, v) ({ \
+ smp_wmb(); \
+ (p) = (v); \
+ })
Will smp_wmb() do the trick?
counter++;
smp_wmb();
synchronize_rcu();
or will have to move to pointer logic? And use rcu_assign_pointer at key.c and
rcu_dereference at tx.c with overhead of kmalloc and kfree?
Thanks
Yogesh
On Thu, 16 Jun 2011 15:57:08 +0530, Yogesh Ashok Powar wrote:
> Following warning was observed after the commit
> aac6af5534fade2b18682a0b9efad1a6c04c34c6
>
>>WARNING: at net/mac80211/wpa.c:397 ccmp_encrypt_skb+0xc4/0x1f0
>
> Consider a scenario where reserving skb tailroom is skipped
> because software encryption is not enabled. SW encryption
> can be disabled because of a) All the keys are hardware
> planted b) No key has been created. But, before actual
> transmit if hardware encryption is disabled or software
> encryption is started, there will not be enough tailroom
> space to accommodate the sw crypto's MMIC or IV and
> WARN_ON will be hit.
>
> This race between updations of hw keys and skipping & using
> tailroom, is fixed by protecting critical regions (code
> accessing crypto_tx_tailroom_needed_cnt and code from
> tailroom skipping to skb_put for IV or MMIC) with the
> spinlock.
Haha, good joke. You've got to be kidding. NACK. No inserting spinlocks
into the TX/RX paths.
johannes
Hi,
I'll test the patch as far as it is ready :-). Could you please create a
complete patch, which runs against a clean compat-wireless-3.0-rc1-1 (or
compat-wireless-2011-06-15)? And maybe, for testing purpose, a log
entry, which makes it possible to check, whether the code is really used
(if it doesn't flood messages)?
Thanks,
Andreas
On Tue, Jun 21, 2011 at 07:40:15AM -0700, Johannes Berg wrote:
> On Tue, 2011-06-21 at 19:40 +0530, Yogesh Ashok Powar wrote:
>
> > @@ -455,6 +496,8 @@ int ieee80211_key_link(struct ieee80211_key *key,
> > __ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
> > __ieee80211_key_destroy(old_key);
> >
> > + increment_tailroom_need_count(key->local);
> > +
>
> This doesn't seem right -- it links the key in first and then does the
> update, the mechanism I described relies on doing it the other way
> around.
In that case we should have something like this
@@ -493,11 +493,11 @@ int ieee80211_key_link(struct ieee80211_key *key,
else
old_key = key_mtx_dereference(sdata->local, sdata->keys[idx]);
+ increment_tailroom_need_count(key->local);
+
__ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
__ieee80211_key_destroy(old_key);
- increment_tailroom_need_count(key->local);
-
ieee80211_debugfs_key_add(key);
ret = ieee80211_key_enable_hw_accel(key);
>
> > @@ -498,8 +541,12 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
> >
> > mutex_lock(&sdata->local->key_mtx);
> >
> > - list_for_each_entry(key, &sdata->key_list, list)
> > + sdata->local->crypto_tx_tailroom_needed_cnt = 0;
> > +
>
> That doesn't seem right either -- only if you have a single sdata that
> will work, I think?
Right. For multiple sdata count will be over written to 0.
I think, following should fix this
Thanks
Yogesh
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index edf9f40..0bf450d 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -541,8 +541,6 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
mutex_lock(&sdata->local->key_mtx);
- sdata->local->crypto_tx_tailroom_needed_cnt = 0;
-
list_for_each_entry(key, &sdata->key_list, list) {
increment_tailroom_need_count(sdata->local);
ieee80211_key_enable_hw_accel(key);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 05e3fb8..bef3bdd 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1271,6 +1271,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
mutex_unlock(&local->sta_mtx);
}
+ /* Reset tailroom skip count */
+ local->crypto_tx_tailroom_needed_cnt = 0;
+
/* add back keys */
list_for_each_entry(sdata, &local->interfaces, list)
if (ieee80211_sdata_running(sdata))
On Mon, Jun 20, 2011 at 10:29:40AM -0700, Johannes Berg wrote:
> On Mon, 2011-06-20 at 09:49 -0700, Yogesh Powar wrote:
> > >No, they're not, the mutex trickery seems completely pointless, and the
> > >conditional (on _is_locked no less) locking is horrible.
> > Ok.
> >
> > Will spend some more time on exploring solutions using rcu primitives only
> > (synchronize_net or something similar).
> >
> > If nothing of only-RCU is feasible then, I think, we need to resize the skb if it has
> > already skipped the tailroom instead of WARN_ON. This will come in to picuture
> > only during the race cases. But this wont get rid of race.
>
> Ahrg, seriously, just analyse the situation for once. Do I really need
> to do that?
>
> There are two race cases, during transitions:
> 1) need to resize -> no need to (key moved to HW)
> 2) no need to resize -> need to (new key, ...)
>
> The first is uninteresting, it's fine if the race happens and the skb is
> still resized even if it didn't need to.
>
> The second one is what's causing issues. But the race happens like this:
> - counter is 0
> - skb goes through tx start, no tailroom allocated
> - key added, counter > 0
> - key lookup while TX processing is successful
>
> So ... how can you solve it? Clearly, the locking attempts you made were
> pretty useless. But now that we understand the race, can we fix it? I
> think we can, by doing something like this:
>
> counter++;
> /* flush packets being processed */
> synchronize_net();
> /* do whatever causes the key to be found by sw crypto */
>
> That should be it. The only overhead is a little bit in control paths
> for key settings which are delayed a bit by synchronize_net(), but who
> cares, we don't change keys every packet.
>
> Of course you need to encapsulate that code into a small function and
> write comments about why it's necessary and how it works.
>
> Was that so hard? :-)
:(
Following is the implementation based on your design.
Let me know if I miss anything here.
Thanks
Yogesh
---
net/mac80211/key.c | 43 +++++++++++++++++++++++++++++++++++++------
1 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 31afd71..4e0be17 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -61,6 +61,36 @@ static struct ieee80211_sta *get_sta_for_key(struct ieee80211_key *key)
return NULL;
}
+static void increment_tailroom_need_count(struct ieee80211_local *local)
+{
+ /*
+ * When this count is zero, SKB resizing for allocating tailroom
+ * for IV or MMIC is skipped. But, this check has created two race
+ * cases in xmit path while transiting from zero count to one:
+ *
+ * 1. SKB resize was skipped because no key was added but just before
+ * the xmit key is added and SW encryption kicks off.
+ *
+ * 2. SKB resize was skipped because all the keys were hw planted but
+ * just before xmit one of the key is deleted and SW encryption kicks
+ * off.
+ *
+ * In both the above case SW encryption will find not enough space for
+ * tailroom and exits with WARN_ON. (See WARN_ONs at wpa.c)
+ *
+ * Solution has been explained at
+ * http://markmail.org/message/c3ykchyv6azzmdum
+ */
+
+ if (!local->crypto_tx_tailroom_needed_cnt++) {
+ /*
+ * Flush all XMIT packets currently using HW encryption or no
+ * encryption at all if the count transition is from 0 -> 1.
+ */
+ synchronize_net();
+ }
+}
+
static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
{
struct ieee80211_sub_if_data *sdata;
@@ -144,6 +174,10 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE))
return;
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+ increment_tailroom_need_count(key->local);
+
sta = get_sta_for_key(key);
sdata = key->sdata;
@@ -162,9 +196,6 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
- if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
- (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
- key->local->crypto_tx_tailroom_needed_cnt++;
}
void ieee80211_key_removed(struct ieee80211_key_conf *key_conf)
@@ -466,9 +497,9 @@ int ieee80211_key_link(struct ieee80211_key *key,
__ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
__ieee80211_key_destroy(old_key);
- ieee80211_debugfs_key_add(key);
+ increment_tailroom_need_count(key->local);
- key->local->crypto_tx_tailroom_needed_cnt++;
+ ieee80211_debugfs_key_add(key);
ret = ieee80211_key_enable_hw_accel(key);
@@ -514,7 +545,7 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
sdata->local->crypto_tx_tailroom_needed_cnt = 0;
list_for_each_entry(key, &sdata->key_list, list) {
- sdata->local->crypto_tx_tailroom_needed_cnt++;
+ increment_tailroom_need_count(sdata->local);
ieee80211_key_enable_hw_accel(key);
}
--
1.5.4.1
On Wed, 2011-06-22 at 18:28 +0530, Yogesh Ashok Powar wrote:
> On Wed, Jun 22, 2011 at 05:49:25AM -0700, Johannes Berg wrote:
> > On Wed, 2011-06-22 at 18:01 +0530, Yogesh Ashok Powar wrote:
> > > > Will work on some other logic.
> > >
> > > Following is the complete V2-patch
> > >
> > > v2 changes: a) Moved counter++ before __ieee80211_key_replace in
> > > key_link()
> > > b) Moved crypto_tx_tailroom_needed_cnt to sdata resolve
> > > issue with multiple sdata instances in hw reset.
> >
> > Looks good. Now I'm just worried about memory and compiler barriers that
> > may be needed so the counter update doesn't move after anything else...
> > Hmm.
>
> I some how feel that synchronize_net may be replaced with
> synchronize_rcu and still the race wont happen.
>
> So the new logic would be
> - counter++ <-- Here keys are not added or deleted
> so rcu readers wont have problem
> with extra space allocated.
>
> - synchronize_rcu <-- This will flush existing readers. Again
> new readers will have no problem with extra
> space allocated.
>
> Let me know your opinion on this.
It doesn't really matter -- synchronize_net() is exactly the same as
synchronize_rcu().
What I'm worried about is that there's no memory barrier after the
counter update, so how do we know it cannot happen after
synchronize_rcu()? I think we need something like rcu_assign_pointer()
but I don't see rcu_assign_index() (any more?)
johannes
>No, they're not, the mutex trickery seems completely pointless, and the
>conditional (on _is_locked no less) locking is horrible.
Ok.
Will spend some more time on exploring solutions using rcu primitives only
(synchronize_net or something similar).
If nothing of only-RCU is feasible then, I think, we need to resize the skb if it has
already skipped the tailroom instead of WARN_ON. This will come in to picuture
only during the race cases. But this wont get rid of race.
Thanks
Yogesh
On Mon, 2011-06-20 at 20:00 +0530, Yogesh Ashok Powar wrote:
> On Fri, Jun 17, 2011 at 10:24:45AM -0700, Johannes Berg wrote:
> > > Using spinlocks in TX/RX path is not allowed because spinlocks will make
> > > TX/RX path to block and its illegal to block while in an RCU read-side
> > > critical section. I think, I got the joke here :(.
> >
> > No, that'd be allowed, but you can't spinlock all the TX path because of performance.
> >
> > I think the way to solve it would be to use synchronize_net() somehow maybe; not really sure.
>
> Following patch avoids tailroom skip check for RCU readside
> critical sections that begin inside the synchronize_rcu's grace
> period, without grabbing any lock in the TX patch and there by avoiding
> the race conditions.
>
> I will request Andreas to test this patch on his testbed if the changes
> are fine with you.
No, they're not, the mutex trickery seems completely pointless, and the
conditional (on _is_locked no less) locking is horrible.
johannes
> Using spinlocks in TX/RX path is not allowed because spinlocks will make
> TX/RX path to block and its illegal to block while in an RCU read-side
> critical section. I think, I got the joke here :(.
No, that'd be allowed, but you can't spinlock all the TX path because of performance.
I think the way to solve it would be to use synchronize_net() somehow maybe; not really sure.
Johannes
On Tue, 2011-06-21 at 18:33 +0530, Yogesh Ashok Powar wrote:
> Following is the implementation based on your design.
>
> Let me know if I miss anything here.
Since your original code was reverted you should send a full patch. But
overall it looks fine. Came to think of it yesterday though, for the
0->1 case it's of course necessary to do the adjustment before the key
can be found, but also for the 1->0 case it'll be necessary to do the
counter adjustment after the key adjustments.
johannes
On Fri, 2011-06-24 at 14:34 +0530, yogeshp wrote:
> Andreas has tested the current implementation on his setup and haven't
> seen any WARN_ONs being hit. Should I send the final patch?
Yeah, sure, thanks everyone. I'll ask around about the memory barrier
things independently.
johannes
On Wednesday 22 June 2011 06:42 PM, Johannes Berg wrote:
> On Wed, 2011-06-22 at 18:28 +0530, Yogesh Ashok Powar wrote:
>> On Wed, Jun 22, 2011 at 05:49:25AM -0700, Johannes Berg wrote:
>>> On Wed, 2011-06-22 at 18:01 +0530, Yogesh Ashok Powar wrote:
>>>>> Will work on some other logic.
>>>>
>>>> Following is the complete V2-patch
>>>>
>>>> v2 changes: a) Moved counter++ before __ieee80211_key_replace in
>>>> key_link()
>>>> b) Moved crypto_tx_tailroom_needed_cnt to sdata resolve
>>>> issue with multiple sdata instances in hw reset.
>>>
>>> Looks good. Now I'm just worried about memory and compiler barriers that
>>> may be needed so the counter update doesn't move after anything else...
>>> Hmm.
>>
>> I some how feel that synchronize_net may be replaced with
>> synchronize_rcu and still the race wont happen.
>>
>> So the new logic would be
>> - counter++ <-- Here keys are not added or deleted
>> so rcu readers wont have problem
>> with extra space allocated.
>>
>> - synchronize_rcu <-- This will flush existing readers. Again
>> new readers will have no problem with extra
>> space allocated.
>>
>> Let me know your opinion on this.
>
> It doesn't really matter -- synchronize_net() is exactly the same as
> synchronize_rcu().
>
> What I'm worried about is that there's no memory barrier after the
> counter update, so how do we know it cannot happen after
> synchronize_rcu()? I think we need something like rcu_assign_pointer()
> but I don't see rcu_assign_index() (any more?)
Hi Johannes,
Andreas has tested the current implementation on his setup and haven't
seen any WARN_ONs being hit. Should I send the final patch?
Thanks
Yogesh
This reverts commit ab6a44ce1da48d35fe7ec95fa068aa617bd7e8dd.
Signed-off-by: Yogesh Ashok Powar <[email protected]>
---
net/mac80211/ieee80211_i.h | 3 +++
net/mac80211/key.c | 21 +++++++++++++++++++--
net/mac80211/tx.c | 7 +------
3 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 090b0ec..2025af5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -775,6 +775,9 @@ struct ieee80211_local {
int tx_headroom; /* required headroom for hardware/radiotap */
+ /* count for keys needing tailroom space allocation */
+ int crypto_tx_tailroom_needed_cnt;
+
/* Tasklet and skb queue to process calls from IRQ mode. All frames
* added to skb_queue will be processed, but frames in
* skb_queue_unreliable may be dropped if the total length of these
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index f825e2f..31afd712 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -101,6 +101,11 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
if (!ret) {
key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
+
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+ key->local->crypto_tx_tailroom_needed_cnt--;
+
return 0;
}
@@ -156,6 +161,10 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
key->conf.keyidx, sta ? sta->addr : bcast_addr, ret);
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
+
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+ key->local->crypto_tx_tailroom_needed_cnt++;
}
void ieee80211_key_removed(struct ieee80211_key_conf *key_conf)
@@ -394,8 +403,10 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key)
ieee80211_aes_key_free(key->u.ccmp.tfm);
if (key->conf.cipher == WLAN_CIPHER_SUITE_AES_CMAC)
ieee80211_aes_cmac_key_free(key->u.aes_cmac.tfm);
- if (key->local)
+ if (key->local) {
ieee80211_debugfs_key_remove(key);
+ key->local->crypto_tx_tailroom_needed_cnt--;
+ }
kfree(key);
}
@@ -457,6 +468,8 @@ int ieee80211_key_link(struct ieee80211_key *key,
ieee80211_debugfs_key_add(key);
+ key->local->crypto_tx_tailroom_needed_cnt++;
+
ret = ieee80211_key_enable_hw_accel(key);
mutex_unlock(&sdata->local->key_mtx);
@@ -498,8 +511,12 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
mutex_lock(&sdata->local->key_mtx);
- list_for_each_entry(key, &sdata->key_list, list)
+ sdata->local->crypto_tx_tailroom_needed_cnt = 0;
+
+ list_for_each_entry(key, &sdata->key_list, list) {
+ sdata->local->crypto_tx_tailroom_needed_cnt++;
ieee80211_key_enable_hw_accel(key);
+ }
mutex_unlock(&sdata->local->key_mtx);
}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3104c84..64e0f75 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1480,12 +1480,7 @@ static int ieee80211_skb_resize(struct ieee80211_local *local,
{
int tail_need = 0;
- /*
- * This could be optimised, devices that do full hardware
- * crypto (including TKIP MMIC) need no tailroom... But we
- * have no drivers for such devices currently.
- */
- if (may_encrypt) {
+ if (may_encrypt && local->crypto_tx_tailroom_needed_cnt) {
tail_need = IEEE80211_ENCRYPT_TAILROOM;
tail_need -= skb_tailroom(skb);
tail_need = max_t(int, tail_need, 0);
--
1.7.5.4
Following warning was observed after the commit
aac6af5534fade2b18682a0b9efad1a6c04c34c6
>WARNING: at net/mac80211/wpa.c:397 ccmp_encrypt_skb+0xc4/0x1f0
Consider a scenario where reserving skb tailroom is skipped
because software encryption is not enabled. SW encryption
can be disabled because of a) All the keys are hardware
planted b) No key has been created. But, before actual
transmit if hardware encryption is disabled or software
encryption is started, there will not be enough tailroom
space to accommodate the sw crypto's MMIC or IV and
WARN_ON will be hit.
This race between updations of hw keys and skipping & using
tailroom, is fixed by protecting critical regions (code
accessing crypto_tx_tailroom_needed_cnt and code from
tailroom skipping to skb_put for IV or MMIC) with the
spinlock.
Reported-and-tested-by: Andreas Hartmann <[email protected]>
Cc: Andreas Hartmann <[email protected]>
Signed-off-by: Yogesh Ashok Powar <[email protected]>
---
net/mac80211/ieee80211_i.h | 6 +++++-
net/mac80211/key.c | 27 ++++++++++++++++++++++++++-
net/mac80211/main.c | 2 ++
net/mac80211/tx.c | 6 ++++++
4 files changed, 39 insertions(+), 2 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2025af5..844d385 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -775,8 +775,12 @@ struct ieee80211_local {
int tx_headroom; /* required headroom for hardware/radiotap */
- /* count for keys needing tailroom space allocation */
+ /*
+ * count for keys needing tailroom space allocation,
+ * its access is protected by spinlock tailroom_skp (see below)
+ */
int crypto_tx_tailroom_needed_cnt;
+ spinlock_t tailroom_skip;
/* Tasklet and skb queue to process calls from IRQ mode. All frames
* added to skb_queue will be processed, but frames in
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 31afd712..b78ef6f 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -104,8 +104,13 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
(key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+
+ spin_lock(&key->local->tailroom_skip);
+
key->local->crypto_tx_tailroom_needed_cnt--;
+ spin_unlock(&key->local->tailroom_skip);
+
return 0;
}
@@ -163,8 +168,14 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
- (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+ (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV))) {
+
+ spin_lock(&key->local->tailroom_skip);
+
key->local->crypto_tx_tailroom_needed_cnt++;
+
+ spin_unlock(&key->local->tailroom_skip);
+ }
}
void ieee80211_key_removed(struct ieee80211_key_conf *key_conf)
@@ -405,7 +416,12 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key)
ieee80211_aes_cmac_key_free(key->u.aes_cmac.tfm);
if (key->local) {
ieee80211_debugfs_key_remove(key);
+
+ spin_lock(&key->local->tailroom_skip);
+
key->local->crypto_tx_tailroom_needed_cnt--;
+
+ spin_unlock(&key->local->tailroom_skip);
}
kfree(key);
@@ -468,8 +484,13 @@ int ieee80211_key_link(struct ieee80211_key *key,
ieee80211_debugfs_key_add(key);
+
+ spin_lock(&key->local->tailroom_skip);
+
key->local->crypto_tx_tailroom_needed_cnt++;
+ spin_unlock(&key->local->tailroom_skip);
+
ret = ieee80211_key_enable_hw_accel(key);
mutex_unlock(&sdata->local->key_mtx);
@@ -511,6 +532,8 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
mutex_lock(&sdata->local->key_mtx);
+ spin_lock(&sdata->local->tailroom_skip);
+
sdata->local->crypto_tx_tailroom_needed_cnt = 0;
list_for_each_entry(key, &sdata->key_list, list) {
@@ -518,6 +541,8 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
ieee80211_key_enable_hw_accel(key);
}
+ spin_unlock(&sdata->local->tailroom_skip);
+
mutex_unlock(&sdata->local->key_mtx);
}
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 866f269..4506ed3 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -625,6 +625,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
spin_lock_init(&local->filter_lock);
spin_lock_init(&local->queue_stop_reason_lock);
+ spin_lock_init(&local->tailroom_skip);
+
/*
* The rx_skb_queue is only accessed from tasklets,
* but other SKB queues are used from within IRQ
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 64e0f75..5ea1baa 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1573,8 +1573,11 @@ static void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
headroom -= skb_headroom(skb);
headroom = max_t(int, 0, headroom);
+ spin_lock(&local->tailroom_skip);
+
if (ieee80211_skb_resize(local, skb, headroom, may_encrypt)) {
dev_kfree_skb(skb);
+ spin_unlock(&local->tailroom_skip);
rcu_read_unlock();
return;
}
@@ -1587,12 +1590,15 @@ static void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
!is_multicast_ether_addr(hdr->addr1))
if (mesh_nexthop_lookup(skb, sdata)) {
/* skb queued: don't free */
+ spin_unlock(&local->tailroom_skip);
rcu_read_unlock();
return;
}
ieee80211_set_qos_hdr(local, skb);
ieee80211_tx(sdata, skb, false);
+
+ spin_unlock(&local->tailroom_skip);
rcu_read_unlock();
}
--
1.7.5.4
On Wed, Jun 22, 2011 at 05:49:25AM -0700, Johannes Berg wrote:
> On Wed, 2011-06-22 at 18:01 +0530, Yogesh Ashok Powar wrote:
> > > Will work on some other logic.
> >
> > Following is the complete V2-patch
> >
> > v2 changes: a) Moved counter++ before __ieee80211_key_replace in
> > key_link()
> > b) Moved crypto_tx_tailroom_needed_cnt to sdata resolve
> > issue with multiple sdata instances in hw reset.
>
> Looks good. Now I'm just worried about memory and compiler barriers that
> may be needed so the counter update doesn't move after anything else...
> Hmm.
I some how feel that synchronize_net may be replaced with
synchronize_rcu and still the race wont happen.
So the new logic would be
- counter++ <-- Here keys are not added or deleted
so rcu readers wont have problem
with extra space allocated.
- synchronize_rcu <-- This will flush existing readers. Again
new readers will have no problem with extra
space allocated.
Let me know your opinion on this.
Thanks
Yogesh
On Fri, Jun 17, 2011 at 10:24:45AM -0700, Johannes Berg wrote:
> > Using spinlocks in TX/RX path is not allowed because spinlocks will make
> > TX/RX path to block and its illegal to block while in an RCU read-side
> > critical section. I think, I got the joke here :(.
>
> No, that'd be allowed, but you can't spinlock all the TX path because of performance.
>
> I think the way to solve it would be to use synchronize_net() somehow maybe; not really sure.
Following patch avoids tailroom skip check for RCU readside
critical sections that begin inside the synchronize_rcu's grace
period, without grabbing any lock in the TX patch and there by avoiding
the race conditions.
I will request Andreas to test this patch on his testbed if the changes
are fine with you.
Thanks
Yogesh
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 2025af5..56c7751b 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -777,6 +777,7 @@ struct ieee80211_local {
/* count for keys needing tailroom space allocation */
int crypto_tx_tailroom_needed_cnt;
+ struct mutex tailroom_skip;
/* Tasklet and skb queue to process calls from IRQ mode. All frames
* added to skb_queue will be processed, but frames in
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 31afd712..d59837a 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -390,14 +390,20 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key)
if (!key)
return;
+ if (key->local && !(key->local->crypto_tx_tailroom_needed_cnt))
+ mutex_lock(&key->local->tailroom_skip);
+
/*
* Synchronize so the TX path can no longer be using
* this key before we free/remove it.
*/
synchronize_rcu();
- if (key->local)
+ if (key->local) {
ieee80211_key_disable_hw_accel(key);
+ if (mutex_is_locked(&key->local->tailroom_skip))
+ mutex_unlock(&key->local->tailroom_skip);
+ }
if (key->conf.cipher == WLAN_CIPHER_SUITE_CCMP)
ieee80211_aes_key_free(key->u.ccmp.tfm);
@@ -468,8 +474,16 @@ int ieee80211_key_link(struct ieee80211_key *key,
ieee80211_debugfs_key_add(key);
+ if (!key->local->crypto_tx_tailroom_needed_cnt) {
+ mutex_lock(&key->local->tailroom_skip);
+ synchronize_rcu();
+ }
+
key->local->crypto_tx_tailroom_needed_cnt++;
+ if (mutex_is_locked(&key->local->tailroom_skip))
+ mutex_unlock(&key->local->tailroom_skip);
+
ret = ieee80211_key_enable_hw_accel(key);
mutex_unlock(&sdata->local->key_mtx);
@@ -513,11 +527,17 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
sdata->local->crypto_tx_tailroom_needed_cnt = 0;
+ mutex_lock(&sdata->local->tailroom_skip);
+
+ synchronize_rcu();
+
list_for_each_entry(key, &sdata->key_list, list) {
sdata->local->crypto_tx_tailroom_needed_cnt++;
ieee80211_key_enable_hw_accel(key);
}
+ mutex_unlock(&sdata->local->tailroom_skip);
+
mutex_unlock(&sdata->local->key_mtx);
}
diff --git a/net/mac80211/main.c b/net/mac80211/main.c
index 866f269..c70b26d 100644
--- a/net/mac80211/main.c
+++ b/net/mac80211/main.c
@@ -625,6 +625,8 @@ struct ieee80211_hw *ieee80211_alloc_hw(size_t priv_data_len,
spin_lock_init(&local->filter_lock);
spin_lock_init(&local->queue_stop_reason_lock);
+ mutex_init(&local->tailroom_skip);
+
/*
* The rx_skb_queue is only accessed from tasklets,
* but other SKB queues are used from within IRQ
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 64e0f75..ce2ee4a 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1480,7 +1480,8 @@ static int ieee80211_skb_resize(struct ieee80211_local *local,
{
int tail_need = 0;
- if (may_encrypt && local->crypto_tx_tailroom_needed_cnt) {
+ if (may_encrypt && (local->crypto_tx_tailroom_needed_cnt ||
+ mutex_is_locked(&local->tailroom_skip))) {
tail_need = IEEE80211_ENCRYPT_TAILROOM;
tail_need -= skb_tailroom(skb);
tail_need = max_t(int, tail_need, 0);
On Wed, 2011-06-22 at 18:01 +0530, Yogesh Ashok Powar wrote:
> > Will work on some other logic.
>
> Following is the complete V2-patch
>
> v2 changes: a) Moved counter++ before __ieee80211_key_replace in
> key_link()
> b) Moved crypto_tx_tailroom_needed_cnt to sdata resolve
> issue with multiple sdata instances in hw reset.
Looks good. Now I'm just worried about memory and compiler barriers that
may be needed so the counter update doesn't move after anything else...
Hmm.
johannes
On Thu, Jun 16, 2011 at 08:36:24AM -0700, Johannes Berg wrote:
> On Thu, 16 Jun 2011 15:57:08 +0530, Yogesh Ashok Powar wrote:
> > Following warning was observed after the commit
> > aac6af5534fade2b18682a0b9efad1a6c04c34c6
> >
> >>WARNING: at net/mac80211/wpa.c:397 ccmp_encrypt_skb+0xc4/0x1f0
> >
> > Consider a scenario where reserving skb tailroom is skipped
> > because software encryption is not enabled. SW encryption
> > can be disabled because of a) All the keys are hardware
> > planted b) No key has been created. But, before actual
> > transmit if hardware encryption is disabled or software
> > encryption is started, there will not be enough tailroom
> > space to accommodate the sw crypto's MMIC or IV and
> > WARN_ON will be hit.
> >
> > This race between updations of hw keys and skipping & using
> > tailroom, is fixed by protecting critical regions (code
> > accessing crypto_tx_tailroom_needed_cnt and code from
> > tailroom skipping to skb_put for IV or MMIC) with the
> > spinlock.
> Haha, good joke. You've got to be kidding. NACK. No inserting spinlocks
> into the TX/RX paths.
Using spinlocks in TX/RX path is not allowed because spinlocks will make
TX/RX path to block and its illegal to block while in an RCU read-side
critical section. I think, I got the joke here :(.
In my opinion, following should be the fix for the races explained above
a) The call synchronize_rcu() should be present before enabling sw
encryption :
This will make pre-existing RCU readers in RCU readside critical
section to complete. Or, SKBs which have already skipped
tailroom to xmit before the end of synchronize_rcu's grace
period.
b) Avoid tailroom skip check for RCU readside critical sections that begin
inside the grace period :
This will make new entrants to mandatory allocating tailroom
space irrespective of sw encryption (or hw encryption with
generate IV/MMIC flags set) and hence avoid the WARN_ON.
Implementing first part is straight forward.
For later part, RCU readers must be aware of on going grace period. I am
currently not sure of any way to implement this. May be iff atomic read
is allowed inside RCU read-side critical sections, then before entering
the grace period, updater can set a atomic flag and unset it once done;
and RCU reader can check this atomic flag to avoid the
__tailroom skip check__.
Please suggest your opinion.
Thanks
Yogesh
> > > @@ -498,8 +541,12 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
> > >
> > > mutex_lock(&sdata->local->key_mtx);
> > >
> > > - list_for_each_entry(key, &sdata->key_list, list)
> > > + sdata->local->crypto_tx_tailroom_needed_cnt = 0;
> > > +
> >
> > That doesn't seem right either -- only if you have a single sdata that
> > will work, I think?
> Right. For multiple sdata count will be over written to 0.
>
> I think, following should fix this
>
> Thanks
> Yogesh
>
> diff --git a/net/mac80211/key.c b/net/mac80211/key.c
> index edf9f40..0bf450d 100644
> --- a/net/mac80211/key.c
> +++ b/net/mac80211/key.c
> @@ -541,8 +541,6 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
>
> mutex_lock(&sdata->local->key_mtx);
>
> - sdata->local->crypto_tx_tailroom_needed_cnt = 0;
> -
> list_for_each_entry(key, &sdata->key_list, list) {
> increment_tailroom_need_count(sdata->local);
> ieee80211_key_enable_hw_accel(key);
> diff --git a/net/mac80211/util.c b/net/mac80211/util.c
> index 05e3fb8..bef3bdd 100644
> --- a/net/mac80211/util.c
> +++ b/net/mac80211/util.c
> @@ -1271,6 +1271,9 @@ int ieee80211_reconfig(struct ieee80211_local *local)
> mutex_unlock(&local->sta_mtx);
> }
>
> + /* Reset tailroom skip count */
> + local->crypto_tx_tailroom_needed_cnt = 0;
> +
> /* add back keys */
> list_for_each_entry(sdata, &local->interfaces, list)
> if (ieee80211_sdata_running(sdata))
>
>
Above logic will not work if one the sdatas fails to enable keys may be
because of sdata not running.
Will work on some other logic.
Thanks
Yogesh
On Tue, Jun 21, 2011 at 06:43:34AM -0700, Johannes Berg wrote:
> On Tue, 2011-06-21 at 18:33 +0530, Yogesh Ashok Powar wrote:
>
> > Following is the implementation based on your design.
> >
> > Let me know if I miss anything here.
>
> Since your original code was reverted you should send a full patch. But
> overall it looks fine.
Thanks
>Came to think of it yesterday though, for the
> 0->1 case it's of course necessary to do the adjustment before the key
> can be found, but also for the 1->0 case it'll be necessary to do the
> counter adjustment after the key adjustments.
Right.
I reconfirmed that the current code handles these cases.
Following is the complete patch.
Andreas will it be possible for you to test this patch on your setup?
Will send an official patch once Andreas confirms things are working.
Thanks
Yogesh
---
net/mac80211/ieee80211_i.h | 3 ++
net/mac80211/key.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
net/mac80211/tx.c | 7 +-----
3 files changed, 53 insertions(+), 8 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 090b0ec..2025af5 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -775,6 +775,9 @@ struct ieee80211_local {
int tx_headroom; /* required headroom for hardware/radiotap */
+ /* count for keys needing tailroom space allocation */
+ int crypto_tx_tailroom_needed_cnt;
+
/* Tasklet and skb queue to process calls from IRQ mode. All frames
* added to skb_queue will be processed, but frames in
* skb_queue_unreliable may be dropped if the total length of these
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index f825e2f..edf9f40 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -61,6 +61,36 @@ static struct ieee80211_sta *get_sta_for_key(struct ieee80211_key *key)
return NULL;
}
+static void increment_tailroom_need_count(struct ieee80211_local *local)
+{
+ /*
+ * When this count is zero, SKB resizing for allocating tailroom
+ * for IV or MMIC is skipped. But, this check has created two race
+ * cases in xmit path while transiting from zero count to one:
+ *
+ * 1. SKB resize was skipped because no key was added but just before
+ * the xmit key is added and SW encryption kicks off.
+ *
+ * 2. SKB resize was skipped because all the keys were hw planted but
+ * just before xmit one of the key is deleted and SW encryption kicks
+ * off.
+ *
+ * In both the above case SW encryption will find not enough space for
+ * tailroom and exits with WARN_ON. (See WARN_ONs at wpa.c)
+ *
+ * Solution has been explained at
+ * http://markmail.org/message/c3ykchyv6azzmdum
+ */
+
+ if (!local->crypto_tx_tailroom_needed_cnt++) {
+ /*
+ * Flush all XMIT packets currently using HW encryption or no
+ * encryption at all if the count transition is from 0 -> 1.
+ */
+ synchronize_net();
+ }
+}
+
static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
{
struct ieee80211_sub_if_data *sdata;
@@ -101,6 +131,11 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
if (!ret) {
key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
+
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+ key->local->crypto_tx_tailroom_needed_cnt--;
+
return 0;
}
@@ -139,6 +174,10 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
if (!(key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE))
return;
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+ increment_tailroom_need_count(key->local);
+
sta = get_sta_for_key(key);
sdata = key->sdata;
@@ -394,8 +433,10 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key)
ieee80211_aes_key_free(key->u.ccmp.tfm);
if (key->conf.cipher == WLAN_CIPHER_SUITE_AES_CMAC)
ieee80211_aes_cmac_key_free(key->u.aes_cmac.tfm);
- if (key->local)
+ if (key->local) {
ieee80211_debugfs_key_remove(key);
+ key->local->crypto_tx_tailroom_needed_cnt--;
+ }
kfree(key);
}
@@ -455,6 +496,8 @@ int ieee80211_key_link(struct ieee80211_key *key,
__ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
__ieee80211_key_destroy(old_key);
+ increment_tailroom_need_count(key->local);
+
ieee80211_debugfs_key_add(key);
ret = ieee80211_key_enable_hw_accel(key);
@@ -498,8 +541,12 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
mutex_lock(&sdata->local->key_mtx);
- list_for_each_entry(key, &sdata->key_list, list)
+ sdata->local->crypto_tx_tailroom_needed_cnt = 0;
+
+ list_for_each_entry(key, &sdata->key_list, list) {
+ increment_tailroom_need_count(sdata->local);
ieee80211_key_enable_hw_accel(key);
+ }
mutex_unlock(&sdata->local->key_mtx);
}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3104c84..64e0f75 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1480,12 +1480,7 @@ static int ieee80211_skb_resize(struct ieee80211_local *local,
{
int tail_need = 0;
- /*
- * This could be optimised, devices that do full hardware
- * crypto (including TKIP MMIC) need no tailroom... But we
- * have no drivers for such devices currently.
- */
- if (may_encrypt) {
+ if (may_encrypt && local->crypto_tx_tailroom_needed_cnt) {
tail_need = IEEE80211_ENCRYPT_TAILROOM;
tail_need -= skb_tailroom(skb);
tail_need = max_t(int, tail_need, 0);
--
1.5.4.1
On Tue, 2011-06-21 at 19:40 +0530, Yogesh Ashok Powar wrote:
> @@ -455,6 +496,8 @@ int ieee80211_key_link(struct ieee80211_key *key,
> __ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
> __ieee80211_key_destroy(old_key);
>
> + increment_tailroom_need_count(key->local);
> +
This doesn't seem right -- it links the key in first and then does the
update, the mechanism I described relies on doing it the other way
around.
> @@ -498,8 +541,12 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
>
> mutex_lock(&sdata->local->key_mtx);
>
> - list_for_each_entry(key, &sdata->key_list, list)
> + sdata->local->crypto_tx_tailroom_needed_cnt = 0;
> +
That doesn't seem right either -- only if you have a single sdata that
will work, I think?
johannes
> Will work on some other logic.
Following is the complete V2-patch
v2 changes: a) Moved counter++ before __ieee80211_key_replace in
key_link()
b) Moved crypto_tx_tailroom_needed_cnt to sdata resolve
issue with multiple sdata instances in hw reset.
Thanks
Yogesh
---
net/mac80211/ieee80211_i.h | 3 ++
net/mac80211/key.c | 51 ++++++++++++++++++++++++++++++++++++++++++-
net/mac80211/tx.c | 14 ++++-------
3 files changed, 57 insertions(+), 11 deletions(-)
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 090b0ec..74c2f62 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -544,6 +544,9 @@ struct ieee80211_sub_if_data {
/* keys */
struct list_head key_list;
+ /* count for keys needing tailroom space allocation */
+ int crypto_tx_tailroom_needed_cnt;
+
struct net_device *dev;
struct ieee80211_local *local;
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index f825e2f..aea5a71 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -61,6 +61,36 @@ static struct ieee80211_sta *get_sta_for_key(struct ieee80211_key *key)
return NULL;
}
+static void increment_tailroom_need_count(struct ieee80211_sub_if_data *sdata)
+{
+ /*
+ * When this count is zero, SKB resizing for allocating tailroom
+ * for IV or MMIC is skipped. But, this check has created two race
+ * cases in xmit path while transiting from zero count to one:
+ *
+ * 1. SKB resize was skipped because no key was added but just before
+ * the xmit key is added and SW encryption kicks off.
+ *
+ * 2. SKB resize was skipped because all the keys were hw planted but
+ * just before xmit one of the key is deleted and SW encryption kicks
+ * off.
+ *
+ * In both the above case SW encryption will find not enough space for
+ * tailroom and exits with WARN_ON. (See WARN_ONs at wpa.c)
+ *
+ * Solution has been explained at
+ * http://markmail.org/message/c3ykchyv6azzmdum
+ */
+
+ if (!sdata->crypto_tx_tailroom_needed_cnt++) {
+ /*
+ * Flush all XMIT packets currently using HW encryption or no
+ * encryption at all if the count transition is from 0 -> 1.
+ */
+ synchronize_net();
+ }
+}
+
static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
{
struct ieee80211_sub_if_data *sdata;
@@ -101,6 +131,11 @@ static int ieee80211_key_enable_hw_accel(struct ieee80211_key *key)
if (!ret) {
key->flags |= KEY_FLAG_UPLOADED_TO_HARDWARE;
+
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+ sdata->crypto_tx_tailroom_needed_cnt--;
+
return 0;
}
@@ -142,6 +177,10 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
sta = get_sta_for_key(key);
sdata = key->sdata;
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV)))
+ increment_tailroom_need_count(sdata);
+
if (sdata->vif.type == NL80211_IFTYPE_AP_VLAN)
sdata = container_of(sdata->bss,
struct ieee80211_sub_if_data,
@@ -394,8 +433,10 @@ static void __ieee80211_key_destroy(struct ieee80211_key *key)
ieee80211_aes_key_free(key->u.ccmp.tfm);
if (key->conf.cipher == WLAN_CIPHER_SUITE_AES_CMAC)
ieee80211_aes_cmac_key_free(key->u.aes_cmac.tfm);
- if (key->local)
+ if (key->local) {
ieee80211_debugfs_key_remove(key);
+ key->sdata->crypto_tx_tailroom_needed_cnt--;
+ }
kfree(key);
}
@@ -452,6 +493,8 @@ int ieee80211_key_link(struct ieee80211_key *key,
else
old_key = key_mtx_dereference(sdata->local, sdata->keys[idx]);
+ increment_tailroom_need_count(sdata);
+
__ieee80211_key_replace(sdata, sta, pairwise, old_key, key);
__ieee80211_key_destroy(old_key);
@@ -498,8 +541,12 @@ void ieee80211_enable_keys(struct ieee80211_sub_if_data *sdata)
mutex_lock(&sdata->local->key_mtx);
- list_for_each_entry(key, &sdata->key_list, list)
+ sdata->crypto_tx_tailroom_needed_cnt = 0;
+
+ list_for_each_entry(key, &sdata->key_list, list) {
+ increment_tailroom_need_count(sdata);
ieee80211_key_enable_hw_accel(key);
+ }
mutex_unlock(&sdata->local->key_mtx);
}
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 3104c84..e8d0d2d 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1474,18 +1474,14 @@ static bool ieee80211_tx(struct ieee80211_sub_if_data *sdata,
/* device xmit handlers */
-static int ieee80211_skb_resize(struct ieee80211_local *local,
+static int ieee80211_skb_resize(struct ieee80211_sub_if_data *sdata,
struct sk_buff *skb,
int head_need, bool may_encrypt)
{
+ struct ieee80211_local *local = sdata->local;
int tail_need = 0;
- /*
- * This could be optimised, devices that do full hardware
- * crypto (including TKIP MMIC) need no tailroom... But we
- * have no drivers for such devices currently.
- */
- if (may_encrypt) {
+ if (may_encrypt && sdata->crypto_tx_tailroom_needed_cnt) {
tail_need = IEEE80211_ENCRYPT_TAILROOM;
tail_need -= skb_tailroom(skb);
tail_need = max_t(int, tail_need, 0);
@@ -1578,7 +1574,7 @@ static void ieee80211_xmit(struct ieee80211_sub_if_data *sdata,
headroom -= skb_headroom(skb);
headroom = max_t(int, 0, headroom);
- if (ieee80211_skb_resize(local, skb, headroom, may_encrypt)) {
+ if (ieee80211_skb_resize(sdata, skb, headroom, may_encrypt)) {
dev_kfree_skb(skb);
rcu_read_unlock();
return;
@@ -1945,7 +1941,7 @@ netdev_tx_t ieee80211_subif_start_xmit(struct sk_buff *skb,
head_need += IEEE80211_ENCRYPT_HEADROOM;
head_need += local->tx_headroom;
head_need = max_t(int, 0, head_need);
- if (ieee80211_skb_resize(local, skb, head_need, true))
+ if (ieee80211_skb_resize(sdata, skb, head_need, true))
goto fail;
}
--
1.5.4.1
On Mon, 2011-06-20 at 09:49 -0700, Yogesh Powar wrote:
> >No, they're not, the mutex trickery seems completely pointless, and the
> >conditional (on _is_locked no less) locking is horrible.
> Ok.
>
> Will spend some more time on exploring solutions using rcu primitives only
> (synchronize_net or something similar).
>
> If nothing of only-RCU is feasible then, I think, we need to resize the skb if it has
> already skipped the tailroom instead of WARN_ON. This will come in to picuture
> only during the race cases. But this wont get rid of race.
Ahrg, seriously, just analyse the situation for once. Do I really need
to do that?
There are two race cases, during transitions:
1) need to resize -> no need to (key moved to HW)
2) no need to resize -> need to (new key, ...)
The first is uninteresting, it's fine if the race happens and the skb is
still resized even if it didn't need to.
The second one is what's causing issues. But the race happens like this:
- counter is 0
- skb goes through tx start, no tailroom allocated
- key added, counter > 0
- key lookup while TX processing is successful
So ... how can you solve it? Clearly, the locking attempts you made were
pretty useless. But now that we understand the race, can we fix it? I
think we can, by doing something like this:
counter++;
/* flush packets being processed */
synchronize_net();
/* do whatever causes the key to be found by sw crypto */
That should be it. The only overhead is a little bit in control paths
for key settings which are delayed a bit by synchronize_net(), but who
cares, we don't change keys every packet.
Of course you need to encapsulate that code into a small function and
write comments about why it's necessary and how it works.
Was that so hard? :-)
johannes
Hi Johannes,
This patch breaks a compilation of a compat-wireless tar ball:
nl80211.c:3641:2: error: implicit declaration of function ‘genl_dump_check_consistent’
&
nl80211.c:3731:4: error: ‘struct netlink_callback’ has no member named ‘seq’
Walter