Return-path: Received: from mx07-00252a01.pphosted.com ([62.209.51.214]:10723 "EHLO mx07-00252a01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S971414AbdDXIpI (ORCPT ); Mon, 24 Apr 2017 04:45:08 -0400 Received: from pps.filterd (m0102628.ppops.net [127.0.0.1]) by mx07-00252a01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v3O8hO88005705 for ; Mon, 24 Apr 2017 09:45:07 +0100 Received: from mail-wm0-f72.google.com (mail-wm0-f72.google.com [74.125.82.72]) by mx07-00252a01.pphosted.com with ESMTP id 29yvjyrypj-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=OK) for ; Mon, 24 Apr 2017 09:45:06 +0100 Received: by mail-wm0-f72.google.com with SMTP id q126so4297147wmg.20 for ; Mon, 24 Apr 2017 01:45:06 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <20170420111651.10213-1-james.hughes@raspberrypi.org> From: James Hughes Date: Mon, 24 Apr 2017 09:45:04 +0100 Message-ID: (sfid-20170424_104518_712100_86F9EAD5) Subject: Re: [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable To: Arend Van Spriel Cc: netdev@vger.kernel.org, Franky Lin , Hante Meuleman , Kalle Valo , linux-wireless Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: On 23 April 2017 at 20:34, Arend Van Spriel wrote: > On 21-4-2017 11:22, James Hughes wrote: >> On 20 April 2017 at 20:48, Arend van Spriel >> wrote: >>> + linux-wireless >>> >>> On 4/20/2017 1:16 PM, James Hughes wrote: >>>> >>>> The driver was adding header information to incoming skb >>>> without ensuring the head was uncloned and hence writable. >>>> >>>> skb_cow_head has been used to ensure they are writable, however, >>>> this required some changes to error handling to ensure that >>>> if skb_cow_head failed it was not ignored. >>>> >>>> This really needs to be reviewed by someone who is more familiar >>>> with this code base to ensure any deallocation of skb's is >>>> still correct. >>>> >>>> Signed-off-by: James Hughes >>>> --- >>>> .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 15 ++++++++-- >>>> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 23 +++++----------- >>>> .../broadcom/brcm80211/brcmfmac/fwsignal.c | 32 >>>> +++++++++++++++++----- >>>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 7 ++++- >>>> 4 files changed, 51 insertions(+), 26 deletions(-) >>>> >>> >>> [...] >>> >>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c >>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c >>>> index 5eaac13..08272e8 100644 >>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c >>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c >>>> @@ -198,7 +198,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct >>>> sk_buff *skb, >>>> int ret; >>>> struct brcmf_if *ifp = netdev_priv(ndev); >>>> struct brcmf_pub *drvr = ifp->drvr; >>>> - struct ethhdr *eh = (struct ethhdr *)(skb->data); >>>> + struct ethhdr *eh; >>>> brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx); >>>> @@ -212,23 +212,14 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct >>>> sk_buff *skb, >>>> } >>>> /* 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); >>>> - dev_kfree_skb(skb); >>>> - skb = skb2; >>>> - if (skb == NULL) { >>>> - brcmf_err("%s: skb_realloc_headroom failed\n", >>>> - brcmf_ifname(ifp)); >>>> - ret = -ENOMEM; >>>> - goto done; >>>> - } >>> >>> >>> What you are throwing away here is code that assures there is sufficient >>> headroom for protocol and bus layer in the tx path, because that is >>> determined by drvr->hdrlen. This is where the skb is handed to the driver so >>> if you could leave the functionality above *and* assure it is writeable that >>> would be the best solution as there is no need for all the other changes >>> down the tx path. >> >> The skb_cow_head function takes the required headroom as a parameter >> and will ensure that there is enough space, so I don't think this code >> segment is >> required or have I misunderstood what you mean here? > > I looked more into what skb_cow_head() and skb_realloc_headroom() > actually do and it seems you are right. > >> Is it safe to rely on the _cow_ being done here and not further down >> in the stack? >> Or at least checked further down in the stack. Previous comments from >> another patch >> requested that the _cow_ be done close to the actual addition of the >> header. I presume >> it is unlikely/impossible that the functions that add header >> information down the stack >> will be called without the above being done first? > > It is safe. During probe sequence each layer in the driver stack > increments drvr->hdrlen with headroom it needs. So drvr->hdrlen will > indicate the total headroom needed when .start_xmit() is called. And > yes, the .start_xmit() is the single point of entry in the transmit > path. So the other locations where skb_push() is done are safe. > OK, I will redo the patch (making it v3 + changelogs), taking this in to account. >>> >>>> + ret = skb_cow_head(skb, drvr->hdrlen); >>>> + if (ret) { >>> >>> >>> So move the realloc code above here instead of simply freeing the skb. >>> >>>> + dev_kfree_skb_any(skb); >>>> + goto done; >>>> } >>>> + eh = (struct ethhdr *)(skb->data); >>> >>> >>> Now this is actually a separate fix so I would like a separate patch for it. >>> >> >> No problem, but see final paragraph below. > > Let me see... I'll split this fix out, but should I issue it in the same patch set or as a separate patch? And for a different patch, should I base it on the _cow_ version patch or the original version? > >>> I have a RPi3 sitting on my desk so how can I replicate the issue. It was >>> something about broadcast/multicast traffic when using AP mode and a bridge, >>> right? >>> >>> Regards, >>> Arend >> >> See this issue for details on replication. >> https://github.com/raspberrypi/firmware/issues/673 >> >> The bridge I use is setup using a similar procedure to that described >> here. https://www.raspberrypi.org/documentation/configuration/wireless/access-point.md >> >> I'm happy to pass over this work to you guys if you think it is >> appropriate, you are the >> experts on the codebase. > > Sure. However, I must say you did an excellent job digging into the > issue and root causing it. We always were under the impression that we > could treat the skb as our own when handed to us in .start_xmit() callback. > > Regards, > Arend Thanks! I suspect that a few drivers are making the same mistake, Eric found 6 over and above the smsc95xx and this one that I originally found. Regards James