On 18 April 2017 at 17:34, Florian Fainelli <f.faine...@gmail.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.duma...@gmail.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
