On 04/18/2017 01:34 AM, James Hughes wrote: > Thanks, (please don't top-post on netdev)
> > 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. > > 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
