The commit to rework the headroom check in start_xmit() now calls
pxskb_expand_head() unconditionally if the header is CoW. Unfortunately,
it does so with the delta between the extant headroom and the header
length, which may be negative if there is already sufficient headroom.
pskb_expand_head() does allow for size being 0, in which case it just
copies, so clamp the header delta to zero.
Opening Chrome (and all my tabs) on a PCIE device was enough to reliably
hit this.
Fixes: 270a6c1f65fe ("brcmfmac: rework headroom check in .start_xmit()")
Signed-off-by: Daniel Stone <[email protected]>
Cc: Arend Van Spriel <[email protected]>
Cc: James Hughes <[email protected]>
Cc: Hante Meuleman <[email protected]>
Cc: Pieter-Paul Giesberts <[email protected]>
Cc: Franky Lin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 1 +
1 file changed, 1 insertion(+)
Really sorry, I forgot to re-run format-patch after fixing the initial
patch. I've run out of coffee. :(
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 2153e8062b4c..42dbd5a8c220 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -215,6 +215,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
/* Make sure there's enough writeable headroom */
if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
head_delta = drvr->hdrlen - skb_headroom(skb);
+ head_delta = max(head_delta, 0);
brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
brcmf_ifname(ifp), head_delta);
--
2.13.2
The commit to rework the headroom check in start_xmit() now calls
pxskb_expand_head() unconditionally if the header is CoW. Unfortunately,
it does so with the delta between the extant headroom and the header
length, which may be negative if there is already sufficient headroom.
pskb_expand_head() does allow for size being 0, in which case it just
copies, so clamp the header delta to zero.
Opening Chrome (and all my tabs) on a PCIE device was enough to reliably
hit this.
Fixes: 270a6c1f65fe ("brcmfmac: rework headroom check in .start_xmit()")
Signed-off-by: Daniel Stone <[email protected]>
Cc: Arend Van Spriel <[email protected]>
Cc: James Hughes <[email protected]>
Cc: Hante Meuleman <[email protected]>
Cc: Pieter-Paul Giesberts <[email protected]>
Cc: Franky Lin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
v2: Correct thinko.
v3: Bring assignment on to one line.
v4: Use max_t rather than max.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 2153e8062b4c..5cc3a07dda9e 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -214,7 +214,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
/* Make sure there's enough writeable headroom */
if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
- head_delta = drvr->hdrlen - skb_headroom(skb);
+ head_delta = max_t(int, drvr->hdrlen - skb_headroom(skb), 0);
brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
brcmf_ifname(ifp), head_delta);
--
2.13.2
Hi,
On 26-07-17 13:24, Daniel Stone wrote:
> The commit to rework the headroom check in start_xmit() now calls
> pxskb_expand_head() unconditionally if the header is CoW. Unfortunately,
> it does so with the delta between the extant headroom and the header
> length, which may be negative if there is already sufficient headroom.
>
> pskb_expand_head() does allow for size being 0, in which case it just
> copies, so clamp the header delta to zero.
>
> Opening Chrome (and all my tabs) on a PCIE device was enough to reliably
> hit this.
>
> Fixes: 270a6c1f65fe ("brcmfmac: rework headroom check in .start_xmit()")
> Signed-off-by: Daniel Stone <[email protected]>
> Cc: Arend Van Spriel <[email protected]>
> Cc: James Hughes <[email protected]>
> Cc: Hante Meuleman <[email protected]>
> Cc: Pieter-Paul Giesberts <[email protected]>
> Cc: Franky Lin <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> v2: Correct thinko.
> v3: Bring assignment on to one line.
> v4: Use max_t rather than max.
I can confirm that this fixes a brcmfmac kernel panic for me:
Tested-by: Hans de Goede <[email protected]>
Regards,
Hans
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 2153e8062b4c..5cc3a07dda9e 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -214,7 +214,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>
> /* Make sure there's enough writeable headroom */
> if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
> - head_delta = drvr->hdrlen - skb_headroom(skb);
> + head_delta = max_t(int, drvr->hdrlen - skb_headroom(skb), 0);
>
> brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
> brcmf_ifname(ifp), head_delta);
>
Daniel Stone <[email protected]> wrote:
> The commit to rework the headroom check in start_xmit() now calls
> pxskb_expand_head() unconditionally if the header is CoW. Unfortunately,
> it does so with the delta between the extant headroom and the header
> length, which may be negative if there is already sufficient headroom.
>
> pskb_expand_head() does allow for size being 0, in which case it just
> copies, so clamp the header delta to zero.
>
> Opening Chrome (and all my tabs) on a PCIE device was enough to reliably
> hit this.
>
> Fixes: 270a6c1f65fe ("brcmfmac: rework headroom check in .start_xmit()")
> Signed-off-by: Daniel Stone <[email protected]>
> Cc: Arend Van Spriel <[email protected]>
> Cc: James Hughes <[email protected]>
> Cc: Hante Meuleman <[email protected]>
> Cc: Pieter-Paul Giesberts <[email protected]>
> Cc: Franky Lin <[email protected]>
> Tested-by: Hans de Goede <[email protected]>
Patch applied to wireless-drivers.git, thanks.
58f36b4526ad brcmfmac: Don't grow SKB by negative size
--
https://patchwork.kernel.org/patch/9864575/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Daniel Stone <[email protected]> writes:
> The commit to rework the headroom check in start_xmit() now calls
> pxskb_expand_head() unconditionally if the header is CoW. Unfortunately,
> it does so with the delta between the extant headroom and the header
> length, which may be negative if there is already sufficient headroom.
>
> pskb_expand_head() does allow for size being 0, in which case it just
> copies, so clamp the header delta to zero.
>
> Opening Chrome (and all my tabs) on a PCIE device was enough to reliably
> hit this.
>
> Fixes: 270a6c1f65fe ("brcmfmac: rework headroom check in .start_xmit()")
> Signed-off-by: Daniel Stone <[email protected]>
> Cc: Arend Van Spriel <[email protected]>
> Cc: James Hughes <[email protected]>
> Cc: Hante Meuleman <[email protected]>
> Cc: Pieter-Paul Giesberts <[email protected]>
> Cc: Franky Lin <[email protected]>
I'll queue this for 4.13.
--
Kalle Valo
On 7/26/2017 1:03 PM, Daniel Stone wrote:
> The commit to rework the headroom check in start_xmit() now calls
> pxskb_expand_head() unconditionally if the header is CoW. Unfortunately,
> it does so with the delta between the extant headroom and the header
> length, which may be negative if there is already sufficient headroom.
>
> pskb_expand_head() does allow for size being 0, in which case it just
> copies, so clamp the header delta to zero.
>
> Opening Chrome (and all my tabs) on a PCIE device was enough to reliably
> hit this.
>
> Fixes: 270a6c1f65fe ("brcmfmac: rework headroom check in .start_xmit()")
> Signed-off-by: Daniel Stone <[email protected]>
> Cc: Arend Van Spriel <[email protected]>
> Cc: James Hughes <[email protected]>
> Cc: Hante Meuleman <[email protected]>
> Cc: Pieter-Paul Giesberts <[email protected]>
> Cc: Franky Lin <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> v2: Correct thinko.
> v3: Bring assignment on to one line.
>
> Thanks for the quick response Arend. It's not quite as simple as the
> form you suggested, since both hdrlen and skb_headroom() are unsigned,
> so it needs need an explicit cast to signed, which was previously
> implicit from the head_delta lvalue.
Oops. That makes me realize that use of max_t() is preferred which would
take care of it.
Thanks,
Arend
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 2153e8062b4c..0b7db8798214 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -214,7 +214,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>
> /* Make sure there's enough writeable headroom */
> if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
> - head_delta = drvr->hdrlen - skb_headroom(skb);
> + head_delta = max((int) (drvr->hdrlen - skb_headroom(skb)), 0);
>
> brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
> brcmf_ifname(ifp), head_delta);
>
On 7/26/2017 10:49 AM, Daniel Stone wrote:
> The commit to rework the headroom check in start_xmit() now calls
> pxskb_expand_head() unconditionally if the header is CoW. Unfortunately,
> it does so with the delta between the extant headroom and the header
> length, which may be negative if there is already sufficient headroom.
>
> pskb_expand_head() does allow for size being 0, in which case it just
> copies, so clamp the header delta to zero.
>
> Opening Chrome (and all my tabs) on a PCIE device was enough to reliably
> hit this.
>
> Fixes: 270a6c1f65fe ("brcmfmac: rework headroom check in .start_xmit()")
> Signed-off-by: Daniel Stone <[email protected]>
> Cc: Arend Van Spriel <[email protected]>
> Cc: James Hughes <[email protected]>
> Cc: Hante Meuleman <[email protected]>
> Cc: Pieter-Paul Giesberts <[email protected]>
> Cc: Franky Lin <[email protected]>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 1 +
> 1 file changed, 1 insertion(+)
>
> Really sorry, I forgot to re-run format-patch after fixing the initial
> patch. I've run out of coffee. :(
Sufficient coffee over here so I have no excuse. You are obviously
right. Please tag this patch for 4.13, ie.:
[PATCH for-4.13 V3] brcmfmac: ....
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 2153e8062b4c..42dbd5a8c220 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -215,6 +215,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
> /* Make sure there's enough writeable headroom */
> if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
> head_delta = drvr->hdrlen - skb_headroom(skb);
> + head_delta = max(head_delta, 0);
minor nit, but can you make it:
- head_delta = drvr->hdrlen - skb_headroom(skb);
+ head_delta = max(drvr->hdrlen - skb_headroom(skb), 0);
Regards,
Arend
The commit to rework the headroom check in start_xmit() now calls
pxskb_expand_head() unconditionally if the header is CoW. Unfortunately,
it does so with the delta between the extant headroom and the header
length, which may be negative if there is already sufficient headroom.
pskb_expand_head() does allow for size being 0, in which case it just
copies, so clamp the header delta to zero.
Opening Chrome (and all my tabs) on a PCIE device was enough to reliably
hit this.
Fixes: 270a6c1f65fe ("brcmfmac: rework headroom check in .start_xmit()")
Signed-off-by: Daniel Stone <[email protected]>
Cc: Arend Van Spriel <[email protected]>
Cc: James Hughes <[email protected]>
Cc: Hante Meuleman <[email protected]>
Cc: Pieter-Paul Giesberts <[email protected]>
Cc: Franky Lin <[email protected]>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
v2: Correct thinko.
v3: Bring assignment on to one line.
Thanks for the quick response Arend. It's not quite as simple as the
form you suggested, since both hdrlen and skb_headroom() are unsigned,
so it needs need an explicit cast to signed, which was previously
implicit from the head_delta lvalue.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 2153e8062b4c..0b7db8798214 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -214,7 +214,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
/* Make sure there's enough writeable headroom */
if (skb_headroom(skb) < drvr->hdrlen || skb_header_cloned(skb)) {
- head_delta = drvr->hdrlen - skb_headroom(skb);
+ head_delta = max((int) (drvr->hdrlen - skb_headroom(skb)), 0);
brcmf_dbg(INFO, "%s: insufficient headroom (%d)\n",
brcmf_ifname(ifp), head_delta);
--
2.13.2