Message-ID: <CAE_XsMJysdO-WvyVRuS=SsBjYpVV5QMYeVkxbumP2SjngV986Q@mail.gmail.com>
Date:   Tue, 18 Apr 2017 17:43:44 +0100
From:   James Hughes <james.hughes@...pberrypi.org>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: Use of skb_unclone in drivers

On 18 April 2017 at 17:34, Florian Fainelli <f.fainelli@...il.com> wrote:
> On 04/18/2017 01:34 AM, James Hughes wrote:
>> Thanks,
>
> (please don't top-post on netdev)
>

Trying not to! New to this.

>>
>> Quick check on that function - very similar to unclone which we know
>> ameliorates the issue, just has the headroom parameter rather than
>> priority. So clearly the right way to go. Just one more question I
>> hope - where is the appropriate  place for the skb_cow_head call in
>> the driver - at the main entry point, or closer or in the function(s)
>> that may alter the header itself?
>
> You should place the call to skb_cow_head() in the same function(s) that
> do(es) the header(s) modifications, that way it's clear to the reader
> what the intent is, and the code is reasonably easy to audit.
>

OK, thanks - have done that and posted a patch for the SMSC driver which
others are reviewing. Still trying to sort out the Brcm wireless driver which
is lacking any sane error path should the skb_cow_head function fail in
the same function where the header modification is made.

>>
>> James
>>
>> (Apologies to Eric for originally sending him this message rather than
>> to netdev - was doing it all from a phone and screwed it up)
>>
>> On 17 April 2017 at 17:07, Eric Dumazet <eric.dumazet@...il.com> wrote:
>>> On Mon, 2017-04-17 at 16:02 +0100, James Hughes wrote:
>>>> Netdevs,
>>>>
>>>> We have recently got to the bottom of an issue which we have been
>>>> encountering on a Raspberry Pi being used as an access point, and we
>>>> need a bit of advice on the correct way of fixing the issue.
>>>>
>>>> The set up is a Raspberry Pi 3 running hostapd on its inbuilt wireless
>>>> adaptor (Brcm43438 ), bridged to the built in ethernet adaptor
>>>> (smsc9514). Using the standard drivers for these devices as of 4.9
>>>> (looking at 4.11, no changes noted that would affect the issue. I will
>>>> be trying the latest kernel once I get back in to the office).
>>>>
>>>> We were encountering an error in the Brcm Wireless driver that after
>>>> investigation was a skb_buff being corrupted by an action in the smsc
>>>> Ethernet driver. Further digging shows that the bridge was cloning an
>>>> incoming skb from the Ethernet, then sending it out to both the
>>>> Ethernet and wlan ports. This mean that both the Ethernet driver and
>>>> the wireless driver were looking at the same physical data, and one or
>>>> both were altering that data in different ways (varied headers) ,
>>>> which mean that headers were corrupted. This was only happening on
>>>> broadcast packets.
>>>>
>>>> We can fix the issue by, in the drivers, using skb_unclone in
>>>> appropriate places in the driver.
>>>>
>>>> So now to the questions. Is adding the unclone to the drivers the
>>>> correct way of dealing with this issue? Examining other drivers shows
>>>> that unclone is not particularly common, presumably in many cases,
>>>> where the driver does not alter the skb, it doesn't matter. However,
>>>> in any case where a driver may add header information to the skb (as
>>>> is the case with the Brcm wireless driver), when the incoming skb has
>>>> been cloned, the results must surely be undetermined unless an unclone
>>>> is performed.
>>>>
>>>> Or, is the bridging code making a mistake by cloning the skb and
>>>> passing it to multiple recipients?
>>>>
>>>
>>>
>>> bridge code is fine.
>>>
>>> Problem is that some drivers lack calls to skb_cow_head() before they
>>> mess with headers.
>>>
>>
>
>
> --
> Florian



-- 
James Hughes
Principal Software Engineer,
Raspberry Pi (Trading) Ltd

Powered by blists - more mailing lists