p54usb kernel panic on recent mainline kernels

p54usb kernel panic on recent mainline kernels

[Christopher Chavez]

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?

Re: p54usb kernel panic on recent mainline kernels

[Christian Lamparter]

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, linux-wireless@vger.kernel.org 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

Re: p54usb kernel panic on recent mainline kernels

[Larry Finger]

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

Re: p54usb kernel panic on recent mainline kernels

[Christopher Chavez]

Larry Finger <Larry.Finger@...> writes:
> 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.

Reported.
https://bugzilla.kernel.org/show_bug.cgi?id=90331

Re: p54usb kernel panic on recent mainline kernels

[Christian Lamparter]

On Friday, December 26, 2014 04:23:21 AM Christopher Chavez wrote:
> Larry Finger <Larry.Finger@...> writes:
> > 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.
> 
> Reported.
> https://bugzilla.kernel.org/show_bug.cgi?id=90331

According to what Larry wrote, this bug should disappear, if
the hw crypto offloading is disabled. 

Christopher, can you please load the p54common kernel module 
with the module parameter "nohwcrypt=1" and report back?

Regards,

Christian

Re: p54usb kernel panic on recent mainline kernels

[Larry Finger]

On 12/26/2014 08:35 AM, Christian Lamparter wrote:
> On Friday, December 26, 2014 04:23:21 AM Christopher Chavez wrote:
>> Larry Finger <Larry.Finger@...> writes:
>>> 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.
>>
>> Reported.
>> https://bugzilla.kernel.org/show_bug.cgi?id=90331
>
> According to what Larry wrote, this bug should disappear, if
> the hw crypto offloading is disabled.
>
> Christopher, can you please load the p54common kernel module
> with the module parameter "nohwcrypt=1" and report back?

Christian,

I can confirm that the skb panic goes away if hardware encryption is disabled.

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.

Larry

Re: p54usb kernel panic on recent mainline kernels

[Christopher Chavez]

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 <ido@wizery.com>
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 <idox.yariv@intel.com>
    Signed-off-by: Emmanuel Grumbach <emmanuel.grumbach@intel.com>
    Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Re: p54usb kernel panic on recent mainline kernels

[Christian Lamparter]

[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 <ido@wizery.com>
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>

Re: p54usb kernel panic on recent mainline kernels

[Christian Lamparter]

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 <ido@wizery.com>
> 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 <chrischavez@gmx.us>
Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
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);
 	}
 

Re: p54usb kernel panic on recent mainline kernels

[Larry Finger]

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 <ido@wizery.com>
>> 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 <chrischavez@gmx.us>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> 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 <Larry.Finger@lwfinger.net>

I think this needs to be applied to 3.{17-19}.

Thanks,

Larry

Re: p54usb kernel panic on recent mainline kernels

[Christopher Chavez]

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 <chrischavez@gmx.us>

nohwcrypt=1 also worked.

Thanks

Re: p54usb kernel panic on recent mainline kernels

[Johannes Berg]

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 <Larry.Finger@lwfinger.net>
> 
> 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

Re: p54usb kernel panic on recent mainline kernels

[Larry Finger]

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 <Larry.Finger@lwfinger.net>
>>
>> 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

[PATCH] mac80211: Re-fix accounting of the tailroom-needed counter

[Ido Yariv]

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 <idox.yariv@intel.com>
Cc: Christopher Chavez <chrischavez@gmx.us>
Cc: Christian Lamparter <chunkeey@googlemail.com>
Cc: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Solomon Peachy <pizza@shaftnet.org>
---
 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

Re: [PATCH] mac80211: Re-fix accounting of the tailroom-needed counter

[Johannes Berg]

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