| Message ID | 20170117232405.7672-1-gavinli@thegavinli.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8e290cecdd0178f3d4cf7d463c51dc7e462843b4 |
| Delegated to: | Kalle Valo |
| Headers | show |
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, >
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, >>
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, >>>
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.
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.
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
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,