All of lore.kernel.org
 help / color / mirror / Atom feed
* Use of skb_unclone in drivers
@ 2017-04-17 15:02 James Hughes
  2017-04-17 16:07 ` Eric Dumazet
  0 siblings, 1 reply; 5+ messages in thread
From: James Hughes @ 2017-04-17 15:02 UTC (permalink / raw)
  To: netdev

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?


Thanks

James Hughes
Raspberry Pi

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Use of skb_unclone in drivers
  2017-04-17 15:02 Use of skb_unclone in drivers James Hughes
@ 2017-04-17 16:07 ` Eric Dumazet
  2017-04-18  8:34   ` James Hughes
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2017-04-17 16:07 UTC (permalink / raw)
  To: James Hughes; +Cc: netdev

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Use of skb_unclone in drivers
  2017-04-17 16:07 ` Eric Dumazet
@ 2017-04-18  8:34   ` James Hughes
  2017-04-18 16:34     ` Florian Fainelli
  0 siblings, 1 reply; 5+ messages in thread
From: James Hughes @ 2017-04-18  8:34 UTC (permalink / raw)
  To: netdev

Thanks,

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?

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@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.
>
>
>
>
>
>



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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Use of skb_unclone in drivers
  2017-04-18  8:34   ` James Hughes
@ 2017-04-18 16:34     ` Florian Fainelli
  2017-04-18 16:43       ` James Hughes
  0 siblings, 1 reply; 5+ messages in thread
From: Florian Fainelli @ 2017-04-18 16:34 UTC (permalink / raw)
  To: James Hughes, netdev

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.dumazet@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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: Use of skb_unclone in drivers
  2017-04-18 16:34     ` Florian Fainelli
@ 2017-04-18 16:43       ` James Hughes
  0 siblings, 0 replies; 5+ messages in thread
From: James Hughes @ 2017-04-18 16:43 UTC (permalink / raw)
  To: Florian Fainelli; +Cc: netdev

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-04-18 16:43 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-17 15:02 Use of skb_unclone in drivers James Hughes
2017-04-17 16:07 ` Eric Dumazet
2017-04-18  8:34   ` James Hughes
2017-04-18 16:34     ` Florian Fainelli
2017-04-18 16:43       ` James Hughes

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.