Return-path: Received: from mail-qk0-f182.google.com ([209.85.220.182]:35366 "EHLO mail-qk0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S976467AbdDXTOQ (ORCPT ); Mon, 24 Apr 2017 15:14:16 -0400 Received: by mail-qk0-f182.google.com with SMTP id f76so46468284qke.2 for ; Mon, 24 Apr 2017 12:14:16 -0700 (PDT) Subject: Re: [PATCH v2] brcmfmac: Make skb header writable before use To: James Hughes , Franky Lin , Hante Meuleman , Kalle Valo , linux-wireless@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com, netdev@vger.kernel.org References: <20170424130322.476-1-james.hughes@raspberrypi.org> From: Arend Van Spriel Message-ID: (sfid-20170424_211444_187319_166FF55C) Date: Mon, 24 Apr 2017 21:14:11 +0200 MIME-Version: 1.0 In-Reply-To: <20170424130322.476-1-james.hughes@raspberrypi.org> Content-Type: text/plain; charset=windows-1252 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 24-4-2017 15:03, James Hughes wrote: > The driver was making changes to the skb_header without > ensuring it was writable (i.e. uncloned). > This patch also removes some boiler plate header size > checking/adjustment code as that is also handled by the > skb_cow_header function used to make header writable. > > This patch depends on > brcmfmac: Ensure pointer correctly set if skb data location changes Hi James, I actually thought I was going to tackle this so I spend some time working on it today, but I will submit that as a subsequent patch. Below some comments but apart from that: Acked-by: Arend van Spriel > Signed-off-by: James Hughes > --- > Changes in v2 > Makes the _cow_ call at the entry point of the skb in to the > stack, means only needs to be done once, and error handling > is easier. > Split a separate minor bug fix off to a separate patch (which > this patch depends on) > > > > .../net/wireless/broadcom/brcm80211/brcmfmac/core.c | 19 ++++--------------- > 1 file changed, 4 insertions(+), 15 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > index 9b7c19a508ac..88f8675a94c2 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > @@ -211,22 +211,11 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb, > goto done; > } > > - /* Make sure there's enough room for any header */ > - if (skb_headroom(skb) < drvr->hdrlen) { > - struct sk_buff *skb2; > - > - brcmf_dbg(INFO, "%s: insufficient headroom\n", > - brcmf_ifname(ifp)); > - drvr->bus_if->tx_realloc++; > - skb2 = skb_realloc_headroom(skb, drvr->hdrlen); > + /* Make sure there's enough room for any header, and make it writable */ Please rephrase: /* Make sure there is enough writable headroom */ Just do: ret = skb_cow_head(skb, drvr->hdrlen); if (ret < 0) { brcmf_err("%s: skb_cow_head failed\n", brcmf_ifname(ifp)); > + if (skb_cow_head(skb, drvr->hdrlen)) { > dev_kfree_skb(skb); > - skb = skb2; > - if (skb == NULL) { > - brcmf_err("%s: skb_realloc_headroom failed\n", > - brcmf_ifname(ifp)); > - ret = -ENOMEM; > - goto done; > - } > + ret = -ENOMEM; ... and drop this assignment. > + goto done; > } > > /* validate length for ether packet */ >