When a device using p54usb joins/connects/associates with an access
point, a kernel panic occurs.
The AP tested uses WPA2; have not tested whether the issue occurs for
other security types or ad hoc connections. The specific devices
tested are 2Wire 802.11g USB v1 (vendor 1630 device 0005). The
firmware used is 2.13.1.0.lm86.arm (a.k.a. "isl3886usb" recommended on
wireless.kernel.org). Tested on Ubuntu 14.10, 32-bit x86 (have not
tested 64-bit or other architectures). Tested on machines with Intel
and SiS USB chipsets. I can try collecting more info (e.g. dmesg
output), and am currently bisecting the kernel somewhere around
3.17-rc1.
Should this be reported as a kernel bug or with the driver?
Hello,
On Wednesday, December 24, 2014 10:39:55 PM Christopher Chavez wrote:
> When a device using p54usb joins/connects/associates with an access
> point, a kernel panic occurs.
I've just got out one of two of my p54usb devices. My device is able
scan, connect, receive and pass traffic to and from my WPA2 AP without
causing any panics.
> The AP tested uses WPA2; have not tested whether the issue occurs for
> other security types or ad hoc connections. The specific devices
> tested are 2Wire 802.11g USB v1 (vendor 1630 device 0005).
All I have are p54usb V2 devices [Specifically, Dell 1450 USB V2].
I don't know if V1 works or not - So, this might actually be the
culprit right here.
> The firmware used is 2.13.1.0.lm86.arm (a.k.a. "isl3886usb"
> recommended on wireless.kernel.org). Tested on Ubuntu 14.10,
> 32-bit x86 (have not tested 64-bit or other architectures).
V2 firmware (due to V2 device). I only have 64-Bit arch.
> Tested on machines with Intel and SiS USB chipsets.
It works with the Intel Z87 chipset I have. Don't know
about other chipsets from different vendors.
> I can try collecting more info (e.g. dmesg output), and am currently
> bisecting the kernel somewhere around 3.17-rc1.
I'm running 3.19-rc1(-wl). I haven't seen or heard anything wrong
with it in the past. It is supposed to work.
> Should this be reported as a kernel bug or with the driver?
"no special mailing list, use the linux wireless list
<yes, [email protected] is fine> for development
and firmware issues."
<http://wireless.kernel.org/en/users/Drivers/p54#Contact>
I can't really dig into anything specific to v1 [no device],
chipset or usb-subsystem. It would be nice if you can capture
a crash and post it [preferably with the right subsystem in CC].
Regards,
Christian
On 12/24/2014 10:39 PM, Christopher Chavez wrote:
> When a device using p54usb joins/connects/associates with an access
> point, a kernel panic occurs.
> The AP tested uses WPA2; have not tested whether the issue occurs for
> other security types or ad hoc connections. The specific devices
> tested are 2Wire 802.11g USB v1 (vendor 1630 device 0005). The
> firmware used is 2.13.1.0.lm86.arm (a.k.a. "isl3886usb" recommended on
> wireless.kernel.org). Tested on Ubuntu 14.10, 32-bit x86 (have not
> tested 64-bit or other architectures). Tested on machines with Intel
> and SiS USB chipsets. I can try collecting more info (e.g. dmesg
> output), and am currently bisecting the kernel somewhere around
> 3.17-rc1.
>
> Should this be reported as a kernel bug or with the driver?
It looks as if this is a bug in p54usb. I Think that I have duplicated the
problem. On my system, the crash doesn't happen when it associates, but crashes
when longer packets are transmitted.
I did not get the entire traceback, but I got a reference to p54_tx_80211+0x3de
from p54common.ko. Using gdb to disassemble this reference, the erring code is
as follows:
(gdb) l *p54_tx_80211+0x3de
0x3c9e is in p54_tx_80211 (drivers/net/wireless/p54/txrx.c:913).
908 memcpy(skb_put(skb, 8), &(info->control.hw_key->key
909 [NL80211_TKIP_DATA_OFFSET_TX_MIC_KEY]), 8);
910 }
911 /* reserve some space for ICV */
912 len += info->control.hw_key->icv_len;
913 memset(skb_put(skb, info->control.hw_key->icv_len), 0,
914 info->control.hw_key->icv_len);
915 } else {
916 txhdr->key_type = 0;
917 txhdr->key_len = 0;
At present I do not know why there is a problem with skb_put() here. Perhaps
someone else will know before I find it.
In any case, file a bug report at bugzilla.kernel.org, mark it as a regression,
and post the bug number here. If you are able to finish the bisection, that
would be helpful.
Larry
I have not yet confirmed Christian's workaround, but thank you both for testing.
> My bisection led to a branch commit d17ec4d as the "bad" commit. Rather than
> finding out where the bisection went bad, I added code to check skb->tail,
> skb->end, and the length to be added. At the time of the call that panics,
> there are 6 bytes between tail and end with 8 bytes needed.
>
> I will be looking for the place where the driver calculates how large the skb
> should be.
In the few remaining revisions from "git bisect visualize",
this one mentions skbs:
commit c70f59a2a007c57843195a93c3b7308204e0a5ab
Author: Ido Yariv <[email protected]>
Date: Tue Jul 29 15:39:14 2014 +0300
mac80211: don't resize skbs needlessly
Header-less cloned skbs with sufficient headroom need not be cloned
unless the tailroom is going to be modified.
Fix ieee80211_skb_resize so it would only resize cloned skbs if either
the header isn't released or the tailroom is going to be modified.
Some drivers might have assumed that skbs are never cloned, so add a HW
flag that explicitly permits cloned TX skbs. Drivers which do not modify
TX skbs should set this flag to avoid copying skbs.
Signed-off-by: Ido Yariv <[email protected]>
Signed-off-by: Emmanuel Grumbach <[email protected]>
Signed-off-by: Johannes Berg <[email protected]>
Alright, here's lunch [for the people in CET].
On Saturday, December 27, 2014 11:10:16 AM Christian Lamparter wrote:
> On Saturday, December 27, 2014 12:15:58 AM Christopher Chavez wrote:
> > > My bisection led to a branch commit d17ec4d as the "bad" commit.
> > > Rather than finding out where the bisection went bad, I added
> > > code to check skb->tail, skb->end, and the length to be added.
> > > At the time of the call that panics, there are 6 bytes between
> > > tail and end with 8 bytes needed.
> > >
> > > I will be looking for the place where the driver calculates how
> > > large the skb should be.
>
> From looking at a other patch from that time and context. I think: "
>
> commit ca34e3b5c808385b175650605faa29e71e91991b
> Author: Ido Yariv <[email protected]>
> Date: Tue Jul 29 15:38:53 2014 +0300
>
> mac80211: Fix accounting of the tailroom-needed counter [1]
>
> [...]
> I can think of several ways of dealing with this issue:
>
> 2. add extra IEEE80211_KEY_FLAG_ or HW_FLAG to restore the old behavior.
> This should be possible and relatively simple. But we/I have to be
> especially careful to differentiate properly between the old and new.
> [i.e.: I need to know what the deal is behind:
> IEEE80211_KEY_FLAG_GENERATE_IV_MGMT in this case? Looks like it can
> be ignored?]
>
---
[Note: for convenience this patch is rolled into one for now -
if it and the concept works, I'll make two parts. one for p54
one for mac80211 [both -stable]. It would be great if someone
could proofread the commit message though - and provide
"tested-by" tags if possible]
mac80211: re-enable tailroom resizing when needed for hardware encryption
The patch "mac80211: Fix accounting of the tailroom-needed counter" reduced
the overhead associated with unnecessary resizing of outgoing frames.
Unfortunately this change broke the assumption that there is always enough
tailroom and this in turn caused p54* to panic.
Reported-by: Christopher Chavez <[email protected]>
Signed-off-by: Christian Lamparter <[email protected]>
---
diff --git a/drivers/net/wireless/p54/main.c b/drivers/net/wireless/p54/main.c
index 97aeff0..b877c7f 100644
--- a/drivers/net/wireless/p54/main.c
+++ b/drivers/net/wireless/p54/main.c
@@ -751,6 +751,7 @@ struct ieee80211_hw *p54_init_common(size_t priv_data_len)
IEEE80211_HW_SUPPORTS_PS |
IEEE80211_HW_PS_NULLFUNC_STACK |
IEEE80211_HW_MFP_CAPABLE |
+ IEEE80211_HW_TAILROOM_CRYPTO |
IEEE80211_HW_REPORTS_TX_ACK_STATUS;
dev->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index 58d719d..c04ac04 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1270,8 +1270,11 @@ struct ieee80211_vif *wdev_to_ieee80211_vif(struct wireless_dev *wdev);
*
* @IEEE80211_KEY_FLAG_GENERATE_IV: This flag should be set by the
* driver to indicate that it requires IV generation for this
- * particular key. Setting this flag does not necessarily mean that SKBs
- * will have sufficient tailroom for ICV or MIC.
+ * particular key. Setting this flag does not mean that SKBs will
+ * have sufficient tailroom for ICV or MIC. If additional tailroom
+ * tailroom needs to be reserved for the ICV or MIC, the driver
+ * should also set the hardware feature flag:
+ * %IEEE80211_HW_TAILROOM_CRYPTO.
* @IEEE80211_KEY_FLAG_GENERATE_MMIC: This flag should be set by
* the driver for a TKIP key if it requires Michael MIC
* generation in software.
@@ -1583,6 +1586,10 @@ struct ieee80211_tx_control {
* @IEEE80211_HW_MFP_CAPABLE:
* Hardware supports management frame protection (MFP, IEEE 802.11w).
*
+ * @IEEE80211_HW_TAILROOM_CRYPTO: The driver would like to have sufficient
+ * tailroom for ICV or MIC for outgoing frames in order to perform
+ * hardware encryption without any additional resizing overhead.
+ *
* @IEEE80211_HW_SUPPORTS_UAPSD:
* Hardware supports Unscheduled Automatic Power Save Delivery
* (U-APSD) in managed mode. The mode is configured with
@@ -1673,7 +1680,7 @@ enum ieee80211_hw_flags {
IEEE80211_HW_MFP_CAPABLE = 1<<13,
IEEE80211_HW_WANT_MONITOR_VIF = 1<<14,
IEEE80211_HW_NO_AUTO_VIF = 1<<15,
- /* free slot */
+ IEEE80211_HW_TAILROOM_CRYPTO = 1<<16,
IEEE80211_HW_SUPPORTS_UAPSD = 1<<17,
IEEE80211_HW_REPORTS_TX_ACK_STATUS = 1<<18,
IEEE80211_HW_CONNECTION_MONITOR = 1<<19,
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index 0bb7038..c3e9a9a 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -140,7 +140,8 @@ 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))
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->local->hw.flags & IEEE80211_HW_TAILROOM_CRYPTO)))
sdata->crypto_tx_tailroom_needed_cnt--;
WARN_ON((key->conf.flags & IEEE80211_KEY_FLAG_PUT_IV_SPACE) &&
@@ -188,7 +189,8 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
sta = key->sta;
sdata = key->sdata;
- if (!(key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC))
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->local->hw.flags & IEEE80211_HW_TAILROOM_CRYPTO)))
increment_tailroom_need_count(sdata);
ret = drv_set_key(key->local, DISABLE_KEY, sdata,
@@ -884,7 +886,8 @@ void ieee80211_remove_key(struct ieee80211_key_conf *keyconf)
if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
- if (!(key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC))
+ if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
+ (key->local->hw.flags & IEEE80211_HW_TAILROOM_CRYPTO)))
increment_tailroom_need_count(key->sdata);
}
On 12/27/2014 05:57 AM, Christian Lamparter wrote:
> Alright, here's lunch [for the people in CET].
>
> On Saturday, December 27, 2014 11:10:16 AM Christian Lamparter wrote:
>> On Saturday, December 27, 2014 12:15:58 AM Christopher Chavez wrote:
>>>> My bisection led to a branch commit d17ec4d as the "bad" commit.
>>>> Rather than finding out where the bisection went bad, I added
>>>> code to check skb->tail, skb->end, and the length to be added.
>>>> At the time of the call that panics, there are 6 bytes between
>>>> tail and end with 8 bytes needed.
>>>>
>>>> I will be looking for the place where the driver calculates how
>>>> large the skb should be.
>>
>> From looking at a other patch from that time and context. I think: "
>>
>> commit ca34e3b5c808385b175650605faa29e71e91991b
>> Author: Ido Yariv <[email protected]>
>> Date: Tue Jul 29 15:38:53 2014 +0300
>>
>> mac80211: Fix accounting of the tailroom-needed counter [1]
>>
>> [...]
>> I can think of several ways of dealing with this issue:
>>
>> 2. add extra IEEE80211_KEY_FLAG_ or HW_FLAG to restore the old behavior.
>> This should be possible and relatively simple. But we/I have to be
>> especially careful to differentiate properly between the old and new.
>> [i.e.: I need to know what the deal is behind:
>> IEEE80211_KEY_FLAG_GENERATE_IV_MGMT in this case? Looks like it can
>> be ignored?]
>>
>
> ---
> [Note: for convenience this patch is rolled into one for now -
> if it and the concept works, I'll make two parts. one for p54
> one for mac80211 [both -stable]. It would be great if someone
> could proofread the commit message though - and provide
> "tested-by" tags if possible]
>
> mac80211: re-enable tailroom resizing when needed for hardware encryption
>
> The patch "mac80211: Fix accounting of the tailroom-needed counter" reduced
> the overhead associated with unnecessary resizing of outgoing frames.
> Unfortunately this change broke the assumption that there is always enough
> tailroom and this in turn caused p54* to panic.
>
> Reported-by: Christopher Chavez <[email protected]>
> Signed-off-by: Christian Lamparter <[email protected]>
> ---
> diff --git a/drivers/net/wireless/p54/main.c b/drivers/net/wireless/p54/main.c
> index 97aeff0..b877c7f 100644
> --- a/drivers/net/wireless/p54/main.c
> +++ b/drivers/net/wireless/p54/main.c
> @@ -751,6 +751,7 @@ struct ieee80211_hw *p54_init_common(size_t priv_data_len)
> IEEE80211_HW_SUPPORTS_PS |
> IEEE80211_HW_PS_NULLFUNC_STACK |
> IEEE80211_HW_MFP_CAPABLE |
> + IEEE80211_HW_TAILROOM_CRYPTO |
> IEEE80211_HW_REPORTS_TX_ACK_STATUS;
>
> dev->wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
> diff --git a/include/net/mac80211.h b/include/net/mac80211.h
> index 58d719d..c04ac04 100644
> --- a/include/net/mac80211.h
> +++ b/include/net/mac80211.h
> @@ -1270,8 +1270,11 @@ struct ieee80211_vif *wdev_to_ieee80211_vif(struct wireless_dev *wdev);
> *
> * @IEEE80211_KEY_FLAG_GENERATE_IV: This flag should be set by the
> * driver to indicate that it requires IV generation for this
> - * particular key. Setting this flag does not necessarily mean that SKBs
> - * will have sufficient tailroom for ICV or MIC.
> + * particular key. Setting this flag does not mean that SKBs will
> + * have sufficient tailroom for ICV or MIC. If additional tailroom
> + * tailroom needs to be reserved for the ICV or MIC, the driver
> + * should also set the hardware feature flag:
> + * %IEEE80211_HW_TAILROOM_CRYPTO.
> * @IEEE80211_KEY_FLAG_GENERATE_MMIC: This flag should be set by
> * the driver for a TKIP key if it requires Michael MIC
> * generation in software.
> @@ -1583,6 +1586,10 @@ struct ieee80211_tx_control {
> * @IEEE80211_HW_MFP_CAPABLE:
> * Hardware supports management frame protection (MFP, IEEE 802.11w).
> *
> + * @IEEE80211_HW_TAILROOM_CRYPTO: The driver would like to have sufficient
> + * tailroom for ICV or MIC for outgoing frames in order to perform
> + * hardware encryption without any additional resizing overhead.
> + *
> * @IEEE80211_HW_SUPPORTS_UAPSD:
> * Hardware supports Unscheduled Automatic Power Save Delivery
> * (U-APSD) in managed mode. The mode is configured with
> @@ -1673,7 +1680,7 @@ enum ieee80211_hw_flags {
> IEEE80211_HW_MFP_CAPABLE = 1<<13,
> IEEE80211_HW_WANT_MONITOR_VIF = 1<<14,
> IEEE80211_HW_NO_AUTO_VIF = 1<<15,
> - /* free slot */
> + IEEE80211_HW_TAILROOM_CRYPTO = 1<<16,
> IEEE80211_HW_SUPPORTS_UAPSD = 1<<17,
> IEEE80211_HW_REPORTS_TX_ACK_STATUS = 1<<18,
> IEEE80211_HW_CONNECTION_MONITOR = 1<<19,
> diff --git a/net/mac80211/key.c b/net/mac80211/key.c
> index 0bb7038..c3e9a9a 100644
> --- a/net/mac80211/key.c
> +++ b/net/mac80211/key.c
> @@ -140,7 +140,8 @@ 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))
> + if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
> + (key->local->hw.flags & IEEE80211_HW_TAILROOM_CRYPTO)))
> sdata->crypto_tx_tailroom_needed_cnt--;
>
> WARN_ON((key->conf.flags & IEEE80211_KEY_FLAG_PUT_IV_SPACE) &&
> @@ -188,7 +189,8 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
> sta = key->sta;
> sdata = key->sdata;
>
> - if (!(key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC))
> + if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
> + (key->local->hw.flags & IEEE80211_HW_TAILROOM_CRYPTO)))
> increment_tailroom_need_count(sdata);
>
> ret = drv_set_key(key->local, DISABLE_KEY, sdata,
> @@ -884,7 +886,8 @@ void ieee80211_remove_key(struct ieee80211_key_conf *keyconf)
> if (key->flags & KEY_FLAG_UPLOADED_TO_HARDWARE) {
> key->flags &= ~KEY_FLAG_UPLOADED_TO_HARDWARE;
>
> - if (!(key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC))
> + if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
> + (key->local->hw.flags & IEEE80211_HW_TAILROOM_CRYPTO)))
> increment_tailroom_need_count(key->sdata);
> }
Christian,
I had redone the bisection and found that ca34e3b was the bad commit. That was
late last night (CST - UTC-5). I was pleased to find your patch in my mailbox
this morning.
With your patch, my system has survived for about 2 hours, whereas it would have
failed in 2 minutes or less. You can add
Tested-by: Larry Finger <[email protected]>
I think this needs to be applied to 3.{17-19}.
Thanks,
Larry
[Readded Larry to the CC]
On Saturday, December 27, 2014 12:15:58 AM Christopher Chavez wrote:
> > My bisection led to a branch commit d17ec4d as the "bad" commit.
> > Rather than finding out where the bisection went bad, I added
> > code to check skb->tail, skb->end, and the length to be added.
> > At the time of the call that panics, there are 6 bytes between
> > tail and end with 8 bytes needed.
> >
> > I will be looking for the place where the driver calculates how
> > large the skb should be.
I think this narrows it down. However, I'm not 100% sure yet if the
problem is just because of "mac80211: don't resize skbs needlessly".
>From looking at a other patch from that time and context. I think: "
commit ca34e3b5c808385b175650605faa29e71e91991b
Author: Ido Yariv <[email protected]>
Date: Tue Jul 29 15:38:53 2014 +0300
mac80211: Fix accounting of the tailroom-needed counter [1]
When hw acceleration is enabled, the GENERATE_IV or PUT_IV_SPACE flags
will only require headroom space. Consequently, the tailroom-needed
counter can safely be decremented."
changed/broke things for p54* (note: cw1200 could be affected as well?
This driver also modifies the tailroom for skbs in cw1200_tx_h_crypt).
Previously, the driver didn't need to manage the tailroom. If the
IEEE80211_KEY_FLAG_GENERATE_IV flag was set, mac80211 would take care of
resizing the skb at the right time and just in one place [of course the
downside was that mac80211 did the resize needlessly].
I can think of several ways of dealing with this issue:
1. move the expand and trim tailroom into the driver.
AFAICT this would add an additional resize [at a bad time].
2. add extra IEEE80211_KEY_FLAG_ or HW_FLAG to restore the old behavior.
This should be possible and relatively simple. But we/I have to be
especially careful to differentiate properly between the old and new.
[i.e.: I need to know what the deal is behind:
IEEE80211_KEY_FLAG_GENERATE_IV_MGMT in this case? Looks like it can
be ignored?]
3. suggestions?
[No, I'm not going to touch crypto_tx_tailroom_needed_cnt outside of
mac80211 :D]
Regards,
Christian
[1] <http://www.spinics.net/lists/linux-wireless/msg125374.html>
Larry Finger <Larry.Finger@...> writes:
> I think this needs to be applied to 3.{17-19}.
I finally tested the patch on 3.18.1, and it works. It would be nice to have
this available in 3.17 and 3.18 -stable.
Tested-by: Christopher Chavez <[email protected]>
nohwcrypt=1 also worked.
Thanks
When hw acceleration is enabled, the GENERATE_IV or PUT_IV_SPACE flags
only require headroom space. Therefore, the tailroom-needed counter can
safely be decremented for most drivers.
The older incarnation of this patch (ca34e3b5) assumed that the above
holds true for all drivers. As reported by Christopher Chavez and
researched by Christian Lamparter and Larry Finger, this isn't a valid
assumption for p54 and cw1200.
Drivers that still require tailroom for ICV/MIC even when HW encryption
is enabled can use IEEE80211_KEY_FLAG_RESERVE_TAILROOM to indicate it.
Signed-off-by: Ido Yariv <[email protected]>
Cc: Christopher Chavez <[email protected]>
Cc: Christian Lamparter <[email protected]>
Cc: Larry Finger <[email protected]>
Cc: Solomon Peachy <[email protected]>
---
drivers/net/wireless/cw1200/sta.c | 3 ++-
drivers/net/wireless/p54/main.c | 2 ++
include/net/mac80211.h | 11 +++++++++--
net/mac80211/key.c | 9 +++------
4 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/cw1200/sta.c b/drivers/net/wireless/cw1200/sta.c
index 5b84664..b96524c 100644
--- a/drivers/net/wireless/cw1200/sta.c
+++ b/drivers/net/wireless/cw1200/sta.c
@@ -708,7 +708,8 @@ int cw1200_set_key(struct ieee80211_hw *dev, enum set_key_cmd cmd,
if (sta)
peer_addr = sta->addr;
- key->flags |= IEEE80211_KEY_FLAG_PUT_IV_SPACE;
+ key->flags |= IEEE80211_KEY_FLAG_PUT_IV_SPACE |
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM;
switch (key->cipher) {
case WLAN_CIPHER_SUITE_WEP40:
diff --git a/drivers/net/wireless/p54/main.c b/drivers/net/wireless/p54/main.c
index 97aeff0..13a30c4 100644
--- a/drivers/net/wireless/p54/main.c
+++ b/drivers/net/wireless/p54/main.c
@@ -575,6 +575,8 @@ static int p54_set_key(struct ieee80211_hw *dev, enum set_key_cmd cmd,
key->hw_key_idx = 0xff;
goto out_unlock;
}
+
+ key->flags |= IEEE80211_KEY_FLAG_RESERVE_TAILROOM;
} else {
slot = key->hw_key_idx;
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index a9de1da..e6a8015 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1227,7 +1227,8 @@ struct ieee80211_vif *wdev_to_ieee80211_vif(struct wireless_dev *wdev);
*
* @IEEE80211_KEY_FLAG_GENERATE_IV: This flag should be set by the
* driver to indicate that it requires IV generation for this
- * particular key.
+ * particular key. Setting this flag does not necessarily mean that SKBs
+ * will have sufficient tailroom for ICV or MIC.
* @IEEE80211_KEY_FLAG_GENERATE_MMIC: This flag should be set by
* the driver for a TKIP key if it requires Michael MIC
* generation in software.
@@ -1239,7 +1240,9 @@ struct ieee80211_vif *wdev_to_ieee80211_vif(struct wireless_dev *wdev);
* @IEEE80211_KEY_FLAG_PUT_IV_SPACE: This flag should be set by the driver
* if space should be prepared for the IV, but the IV
* itself should not be generated. Do not set together with
- * @IEEE80211_KEY_FLAG_GENERATE_IV on the same key.
+ * @IEEE80211_KEY_FLAG_GENERATE_IV on the same key. Setting this flag does
+ * not necessarily mean that SKBs will have sufficient tailroom for ICV or
+ * MIC.
* @IEEE80211_KEY_FLAG_RX_MGMT: This key will be used to decrypt received
* management frames. The flag can help drivers that have a hardware
* crypto implementation that doesn't deal with management frames
@@ -1250,6 +1253,9 @@ struct ieee80211_vif *wdev_to_ieee80211_vif(struct wireless_dev *wdev);
* @IEEE80211_KEY_FLAG_GENERATE_IV_MGMT: This flag should be set by the
* driver for a CCMP key to indicate that is requires IV generation
* only for managment frames (MFP).
+ * @IEEE80211_KEY_FLAG_RESERVE_TAILROOM: This flag should be set by the
+ * driver for a key to indicate that sufficient tailroom must always
+ * be reserved for ICV or MIC, even when HW encryption is enabled.
*/
enum ieee80211_key_flags {
IEEE80211_KEY_FLAG_GENERATE_IV_MGMT = BIT(0),
@@ -1259,6 +1265,7 @@ enum ieee80211_key_flags {
IEEE80211_KEY_FLAG_SW_MGMT_TX = BIT(4),
IEEE80211_KEY_FLAG_PUT_IV_SPACE = BIT(5),
IEEE80211_KEY_FLAG_RX_MGMT = BIT(6),
+ IEEE80211_KEY_FLAG_RESERVE_TAILROOM = BIT(7),
};
/**
diff --git a/net/mac80211/key.c b/net/mac80211/key.c
index bb8e697..88614c7 100644
--- a/net/mac80211/key.c
+++ b/net/mac80211/key.c
@@ -132,8 +132,7 @@ static int ieee80211_key_enable_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_PUT_IV_SPACE)))
+ (key->conf.flags & IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
sdata->crypto_tx_tailroom_needed_cnt--;
WARN_ON((key->conf.flags & IEEE80211_KEY_FLAG_PUT_IV_SPACE) &&
@@ -182,8 +181,7 @@ static void ieee80211_key_disable_hw_accel(struct ieee80211_key *key)
sdata = key->sdata;
if (!((key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_MMIC) ||
- (key->conf.flags & IEEE80211_KEY_FLAG_GENERATE_IV) ||
- (key->conf.flags & IEEE80211_KEY_FLAG_PUT_IV_SPACE)))
+ (key->conf.flags & IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
increment_tailroom_need_count(sdata);
ret = drv_set_key(key->local, DISABLE_KEY, sdata,
@@ -880,8 +878,7 @@ void ieee80211_remove_key(struct ieee80211_key_conf *keyconf)
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_PUT_IV_SPACE)))
+ (key->conf.flags & IEEE80211_KEY_FLAG_RESERVE_TAILROOM)))
increment_tailroom_need_count(key->sdata);
}
--
2.1.0
On Sat, 2014-12-27 at 12:38 -0600, Larry Finger wrote:
> I had redone the bisection and found that ca34e3b was the bad commit. That was
> late last night (CST - UTC-5). I was pleased to find your patch in my mailbox
> this morning.
>
> With your patch, my system has survived for about 2 hours, whereas it would have
> failed in 2 minutes or less. You can add
>
> Tested-by: Larry Finger <[email protected]>
>
> I think this needs to be applied to 3.{17-19}.
Thanks everyone. I just sent out a revert - that'll be easier to manage
for 3.17-3.19, and we can redo the original commit properly for 3.20.
johannes
On 01/05/2015 03:33 AM, Johannes Berg wrote:
> On Sat, 2014-12-27 at 12:38 -0600, Larry Finger wrote:
>
>> I had redone the bisection and found that ca34e3b was the bad commit. That was
>> late last night (CST - UTC-5). I was pleased to find your patch in my mailbox
>> this morning.
>>
>> With your patch, my system has survived for about 2 hours, whereas it would have
>> failed in 2 minutes or less. You can add
>>
>> Tested-by: Larry Finger <[email protected]>
>>
>> I think this needs to be applied to 3.{17-19}.
>
> Thanks everyone. I just sent out a revert - that'll be easier to manage
> for 3.17-3.19, and we can redo the original commit properly for 3.20.
That sounds good.
Larry
On Tue, 2015-01-06 at 08:39 -0500, Ido Yariv wrote:
> When hw acceleration is enabled, the GENERATE_IV or PUT_IV_SPACE flags
> only require headroom space. Therefore, the tailroom-needed counter can
> safely be decremented for most drivers.
>
> The older incarnation of this patch (ca34e3b5) assumed that the above
> holds true for all drivers. As reported by Christopher Chavez and
> researched by Christian Lamparter and Larry Finger, this isn't a valid
> assumption for p54 and cw1200.
>
> Drivers that still require tailroom for ICV/MIC even when HW encryption
> is enabled can use IEEE80211_KEY_FLAG_RESERVE_TAILROOM to indicate it.
Applied. Let's hope it sticks this time :)
johannes