[PATCH v3] brcmfmac: fix incorrect event channel deduction

[PATCH v3] brcmfmac: fix incorrect event channel deduction

[gavinli]

From: Gavin Li <git@thegavinli.com>

brcmf_sdio_fromevntchan() was being called on the the data frame
rather than the software header, causing some frames to be
mischaracterized as on the event channel rather than the data channel.

This fixes a major performance regression (due to dropped packets).

Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
Signed-off-by: Gavin Li <git@thegavinli.com>
Cc: <stable@vger.kernel.org> [4.7+]
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index dfb0658713d9..d2219885071f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -1661,7 +1661,7 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
 					   pfirst->len, pfirst->next,
 					   pfirst->prev);
 			skb_unlink(pfirst, &bus->glom);
-			if (brcmf_sdio_fromevntchan(pfirst->data))
+			if (brcmf_sdio_fromevntchan(&dptr[SDPCM_HWHDR_LEN]))
 				brcmf_rx_event(bus->sdiodev->dev, pfirst);
 			else
 				brcmf_rx_frame(bus->sdiodev->dev, pfirst,
-- 
2.11.0

Re: [PATCH v3] brcmfmac: fix incorrect event channel deduction

[Arend Van Spriel]

On 18-1-2017 0:24, gavinli@thegavinli.com wrote:
> From: Gavin Li <git@thegavinli.com>
> 
> brcmf_sdio_fromevntchan() was being called on the the data frame
> rather than the software header, causing some frames to be
> mischaracterized as on the event channel rather than the data channel.
> 
> This fixes a major performance regression (due to dropped packets).
> 
> Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")

Actually would prefer Franky to chime in as well as he made the change
and confirm correctness. It was introduced a couple of kernel versions
back and only a performance regression so seems ok to let this go in
4.11 for now and backport as needed for stable later.

Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Gavin Li <git@thegavinli.com>
> Cc: <stable@vger.kernel.org> [4.7+]
> ---
>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index dfb0658713d9..d2219885071f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -1661,7 +1661,7 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
>  					   pfirst->len, pfirst->next,
>  					   pfirst->prev);
>  			skb_unlink(pfirst, &bus->glom);
> -			if (brcmf_sdio_fromevntchan(pfirst->data))
> +			if (brcmf_sdio_fromevntchan(&dptr[SDPCM_HWHDR_LEN]))
>  				brcmf_rx_event(bus->sdiodev->dev, pfirst);
>  			else
>  				brcmf_rx_frame(bus->sdiodev->dev, pfirst,
> 

Re: [PATCH v3] brcmfmac: fix incorrect event channel deduction

[Gavin Li]

I think calling this a performance regression is a bit understated; my
download speed jumped from 1Mbit/s back up to 40MBit/s after applying
the patch due to the sheer amount of packets being incorrectly
processed.

In addition, processing arbitrary data frames as firmware events might
be a security vulnerability.

On Wed, Jan 18, 2017 at 4:38 AM, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
> On 18-1-2017 0:24, gavinli@thegavinli.com wrote:
>> From: Gavin Li <git@thegavinli.com>
>>
>> brcmf_sdio_fromevntchan() was being called on the the data frame
>> rather than the software header, causing some frames to be
>> mischaracterized as on the event channel rather than the data channel.
>>
>> This fixes a major performance regression (due to dropped packets).
>>
>> Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
>
> Actually would prefer Franky to chime in as well as he made the change
> and confirm correctness. It was introduced a couple of kernel versions
> back and only a performance regression so seems ok to let this go in
> 4.11 for now and backport as needed for stable later.
>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>> Signed-off-by: Gavin Li <git@thegavinli.com>
>> Cc: <stable@vger.kernel.org> [4.7+]
>> ---
>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> index dfb0658713d9..d2219885071f 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -1661,7 +1661,7 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
>>                                          pfirst->len, pfirst->next,
>>                                          pfirst->prev);
>>                       skb_unlink(pfirst, &bus->glom);
>> -                     if (brcmf_sdio_fromevntchan(pfirst->data))
>> +                     if (brcmf_sdio_fromevntchan(&dptr[SDPCM_HWHDR_LEN]))
>>                               brcmf_rx_event(bus->sdiodev->dev, pfirst);
>>                       else
>>                               brcmf_rx_frame(bus->sdiodev->dev, pfirst,
>>

Re: [PATCH v3] brcmfmac: fix incorrect event channel deduction

[Arend Van Spriel]

On 18-1-2017 19:39, Gavin Li wrote:
> I think calling this a performance regression is a bit understated; my
> download speed jumped from 1Mbit/s back up to 40MBit/s after applying
> the patch due to the sheer amount of packets being incorrectly
> processed.

I was merely using your own words although admittedly I left out the
word "major".

> In addition, processing arbitrary data frames as firmware events might
> be a security vulnerability.

True. So I am not saying there is no need to backport it to stable
kernel. I meant there are no reported crashes related to this bug so
there is no need to push it through the current release cycle.

Regards,
Arend

> On Wed, Jan 18, 2017 at 4:38 AM, Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> On 18-1-2017 0:24, gavinli@thegavinli.com wrote:
>>> From: Gavin Li <git@thegavinli.com>
>>>
>>> brcmf_sdio_fromevntchan() was being called on the the data frame
>>> rather than the software header, causing some frames to be
>>> mischaracterized as on the event channel rather than the data channel.
>>>
>>> This fixes a major performance regression (due to dropped packets).
>>>
>>> Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
>>
>> Actually would prefer Franky to chime in as well as he made the change
>> and confirm correctness. It was introduced a couple of kernel versions
>> back and only a performance regression so seems ok to let this go in
>> 4.11 for now and backport as needed for stable later.
>>
>> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
>>> Signed-off-by: Gavin Li <git@thegavinli.com>
>>> Cc: <stable@vger.kernel.org> [4.7+]
>>> ---
>>>  drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> index dfb0658713d9..d2219885071f 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> @@ -1661,7 +1661,7 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
>>>                                          pfirst->len, pfirst->next,
>>>                                          pfirst->prev);
>>>                       skb_unlink(pfirst, &bus->glom);
>>> -                     if (brcmf_sdio_fromevntchan(pfirst->data))
>>> +                     if (brcmf_sdio_fromevntchan(&dptr[SDPCM_HWHDR_LEN]))
>>>                               brcmf_rx_event(bus->sdiodev->dev, pfirst);
>>>                       else
>>>                               brcmf_rx_frame(bus->sdiodev->dev, pfirst,
>>>

Re: [PATCH v3] brcmfmac: fix incorrect event channel deduction

[Kalle Valo]

Gavin Li <gavinli@thegavinli.com> writes:

> I think calling this a performance regression is a bit understated; my
> download speed jumped from 1Mbit/s back up to 40MBit/s after applying
> the patch due to the sheer amount of packets being incorrectly
> processed.
>
> In addition, processing arbitrary data frames as firmware events might
> be a security vulnerability.

You should always mention valuable information like this in the commit
log, don't make us maintainers (and others) guessing the symptoms and
impact.

-- 
Kalle Valo

Re: [PATCH v3] brcmfmac: fix incorrect event channel deduction

[Kalle Valo]

Gavin Li <gavinli@thegavinli.com> writes:

> Should I make a v4 patch with an updated log?

No need, I'll just copy your description of the bug to the commit log
before I commit it.

-- 
Kalle Valo

Re: [v3] brcmfmac: fix incorrect event channel deduction

[Kalle Valo]

Gavin Li <gavinli@thegavinli.com> wrote:
> From: Gavin Li <git@thegavinli.com>
> 
> brcmf_sdio_fromevntchan() was being called on the the data frame
> rather than the software header, causing some frames to be
> mischaracterized as on the event channel rather than the data channel.
> 
> This fixes a major performance regression (due to dropped packets). With 
> this patch the download speed jumped from 1Mbit/s back up to 40MBit/s due
> to the sheer amount of packets being incorrectly processed.
> 
> Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
> Signed-off-by: Gavin Li <git@thegavinli.com>
> Cc: <stable@vger.kernel.org> # 4.7+
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> [kvalo@codeaurora.org: improve commit logs based on email discussion]

Patch applied to wireless-drivers-next.git, thanks.

8e290cecdd01 brcmfmac: fix incorrect event channel deduction

-- 
https://patchwork.kernel.org/patch/9522185/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches