[PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[James Hughes]

The driver was failing to check that the SKB wasn't cloned
before adding checksum data or adding header data.
Replace existing handling to extend the buffer with
skb_cow. Don't use skb_cow_head as the sw checksum
code modifies the data portion.

Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
 drivers/net/usb/smsc95xx.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index df60c98..04f6397 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
 	/* We do not advertise SG, so skbs should be already linearized */
 	BUG_ON(skb_shinfo(skb)->nr_frags);
 
-	if (skb_headroom(skb) < overhead) {
-		struct sk_buff *skb2 = skb_copy_expand(skb,
-			overhead, 0, flags);
-		dev_kfree_skb_any(skb);
-		skb = skb2;
-		if (!skb)
-			return NULL;
+	/* Make writable and expand space by overhead if required */
+	if (skb_cow(skb, overhead)) {
+		return NULL;
 	}
 
 	if (csum) {
-- 
2.9.3

Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[Eric Dumazet]

On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
> The driver was failing to check that the SKB wasn't cloned
> before adding checksum data or adding header data.
> Replace existing handling to extend the buffer with
> skb_cow. Don't use skb_cow_head as the sw checksum
> code modifies the data portion.
> 
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> ---
>  drivers/net/usb/smsc95xx.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index df60c98..04f6397 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>  	/* We do not advertise SG, so skbs should be already linearized */
>  	BUG_ON(skb_shinfo(skb)->nr_frags);
>  
> -	if (skb_headroom(skb) < overhead) {
> -		struct sk_buff *skb2 = skb_copy_expand(skb,
> -			overhead, 0, flags);
> -		dev_kfree_skb_any(skb);
> -		skb = skb2;
> -		if (!skb)
> -			return NULL;
> +	/* Make writable and expand space by overhead if required */
> +	if (skb_cow(skb, overhead)) {
> +		return NULL;
>  	}

Note that this patch will probably force a copy of all locally generated
TCP packets.

For them skb_cloned(skb) is true.

I do believe skb_cow_head() would be better, since TCP stack uses the
__skb_header_release() helper to tell lower stacks they can write the
header part, even on a clone.

Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 18 Apr 2017 08:51:51 -0700

> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
>> The driver was failing to check that the SKB wasn't cloned
>> before adding checksum data or adding header data.
>> Replace existing handling to extend the buffer with
>> skb_cow. Don't use skb_cow_head as the sw checksum
>> code modifies the data portion.
>> 
>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>> ---
>>  drivers/net/usb/smsc95xx.c | 10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> index df60c98..04f6397 100644
>> --- a/drivers/net/usb/smsc95xx.c
>> +++ b/drivers/net/usb/smsc95xx.c
>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>>  	/* We do not advertise SG, so skbs should be already linearized */
>>  	BUG_ON(skb_shinfo(skb)->nr_frags);
>>  
>> -	if (skb_headroom(skb) < overhead) {
>> -		struct sk_buff *skb2 = skb_copy_expand(skb,
>> -			overhead, 0, flags);
>> -		dev_kfree_skb_any(skb);
>> -		skb = skb2;
>> -		if (!skb)
>> -			return NULL;
>> +	/* Make writable and expand space by overhead if required */
>> +	if (skb_cow(skb, overhead)) {
>> +		return NULL;
>>  	}
> 
> Note that this patch will probably force a copy of all locally generated
> TCP packets.
> 
> For them skb_cloned(skb) is true.
> 
> I do believe skb_cow_head() would be better, since TCP stack uses the
> __skb_header_release() helper to tell lower stacks they can write the
> header part, even on a clone.

Agreed.

Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[James Hughes]

On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Tue, 18 Apr 2017 08:51:51 -0700
>
>> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
>>> The driver was failing to check that the SKB wasn't cloned
>>> before adding checksum data or adding header data.
>>> Replace existing handling to extend the buffer with
>>> skb_cow. Don't use skb_cow_head as the sw checksum
>>> code modifies the data portion.
>>>
>>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>>> ---
>>>  drivers/net/usb/smsc95xx.c | 10 +++-------
>>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>>> index df60c98..04f6397 100644
>>> --- a/drivers/net/usb/smsc95xx.c
>>> +++ b/drivers/net/usb/smsc95xx.c
>>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>>>      /* We do not advertise SG, so skbs should be already linearized */
>>>      BUG_ON(skb_shinfo(skb)->nr_frags);
>>>
>>> -    if (skb_headroom(skb) < overhead) {
>>> -            struct sk_buff *skb2 = skb_copy_expand(skb,
>>> -                    overhead, 0, flags);
>>> -            dev_kfree_skb_any(skb);
>>> -            skb = skb2;
>>> -            if (!skb)
>>> -                    return NULL;
>>> +    /* Make writable and expand space by overhead if required */
>>> +    if (skb_cow(skb, overhead)) {
>>> +            return NULL;
>>>      }
>>
>> Note that this patch will probably force a copy of all locally generated
>> TCP packets.
>>
>> For them skb_cloned(skb) is true.
>>
>> I do believe skb_cow_head() would be better, since TCP stack uses the
>> __skb_header_release() helper to tell lower stacks they can write the
>> header part, even on a clone.
>
> Agreed.

I'm happy to work it as you see fit - you know this code far better than I do.

Our reading of the code is that the software checksum path is
modifying the data rather than just adding a header. Based on the
description of skb_cow_head it therefore isn't appropriate. If that
isn't a concern in reality then skb_cow_head is fine and I'll make a
V2 patchset.
Or do we need to skb_cow if doing the software checksum, but
skb_cow_head normally? That can be done instead but requires a
slightly larger change.

The failure case we were seeing was with a bridged network using
SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
making an SKB clone of broadcasts for the two interfaces, and then
both drivers were adding headers without checking skb_cloned(skb)
first, hence trampling on each other. For small packets the SMSC95xx
driver will be computing the software checksum and writing it in to
the data, so the wifi driver will also be seeing it. For many drivers
that probably won't matter, but is that always true?

(Patches for the Broadcom wifi driver will be coming once we've worked
out the best way of fixing this - there is no error path easily
available if the skb_cow_head call fails).


James

Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[Eric Dumazet]

On Tue, 2017-04-18 at 17:16 +0100, James Hughes wrote:
> On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Tue, 18 Apr 2017 08:51:51 -0700
> >
> >> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
> >>> The driver was failing to check that the SKB wasn't cloned
> >>> before adding checksum data or adding header data.
> >>> Replace existing handling to extend the buffer with
> >>> skb_cow. Don't use skb_cow_head as the sw checksum
> >>> code modifies the data portion.
> >>>
> >>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> >>> ---
> >>>  drivers/net/usb/smsc95xx.c | 10 +++-------
> >>>  1 file changed, 3 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> >>> index df60c98..04f6397 100644
> >>> --- a/drivers/net/usb/smsc95xx.c
> >>> +++ b/drivers/net/usb/smsc95xx.c
> >>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
> >>>      /* We do not advertise SG, so skbs should be already linearized */
> >>>      BUG_ON(skb_shinfo(skb)->nr_frags);
> >>>
> >>> -    if (skb_headroom(skb) < overhead) {
> >>> -            struct sk_buff *skb2 = skb_copy_expand(skb,
> >>> -                    overhead, 0, flags);
> >>> -            dev_kfree_skb_any(skb);
> >>> -            skb = skb2;
> >>> -            if (!skb)
> >>> -                    return NULL;
> >>> +    /* Make writable and expand space by overhead if required */
> >>> +    if (skb_cow(skb, overhead)) {
> >>> +            return NULL;
> >>>      }
> >>
> >> Note that this patch will probably force a copy of all locally generated
> >> TCP packets.
> >>
> >> For them skb_cloned(skb) is true.
> >>
> >> I do believe skb_cow_head() would be better, since TCP stack uses the
> >> __skb_header_release() helper to tell lower stacks they can write the
> >> header part, even on a clone.
> >
> > Agreed.
> 
> I'm happy to work it as you see fit - you know this code far better than I do.
> 
> Our reading of the code is that the software checksum path is
> modifying the data rather than just adding a header. Based on the
> description of skb_cow_head it therefore isn't appropriate. If that
> isn't a concern in reality then skb_cow_head is fine and I'll make a
> V2 patchset.
> Or do we need to skb_cow if doing the software checksum, but
> skb_cow_head normally? That can be done instead but requires a
> slightly larger change.
> 
> The failure case we were seeing was with a bridged network using
> SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
> making an SKB clone of broadcasts for the two interfaces, and then
> both drivers were adding headers without checking skb_cloned(skb)
> first, hence trampling on each other. For small packets the SMSC95xx
> driver will be computing the software checksum and writing it in to
> the data, so the wifi driver will also be seeing it. For many drivers
> that probably won't matter, but is that always true?
> 
> (Patches for the Broadcom wifi driver will be coming once we've worked
> out the best way of fixing this - there is no error path easily
> available if the skb_cow_head call fails).
> 

You misread what the driver does.

The TCP data (payload) is _not_ modified.

Only additional headers are pushed in front of the existing (Ethernet,
IP, TCP) headers.

For this, skb_cow_head() is the perfect solution.

Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[James Hughes]

On 18 April 2017 at 17:52, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-04-18 at 17:16 +0100, James Hughes wrote:
>> On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
>> > From: Eric Dumazet <eric.dumazet@gmail.com>
>> > Date: Tue, 18 Apr 2017 08:51:51 -0700
>> >
>> >> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
>> >>> The driver was failing to check that the SKB wasn't cloned
>> >>> before adding checksum data or adding header data.
>> >>> Replace existing handling to extend the buffer with
>> >>> skb_cow. Don't use skb_cow_head as the sw checksum
>> >>> code modifies the data portion.
>> >>>
>> >>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>> >>> ---
>> >>>  drivers/net/usb/smsc95xx.c | 10 +++-------
>> >>>  1 file changed, 3 insertions(+), 7 deletions(-)
>> >>>
>> >>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>> >>> index df60c98..04f6397 100644
>> >>> --- a/drivers/net/usb/smsc95xx.c
>> >>> +++ b/drivers/net/usb/smsc95xx.c
>> >>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>> >>>      /* We do not advertise SG, so skbs should be already linearized */
>> >>>      BUG_ON(skb_shinfo(skb)->nr_frags);
>> >>>
>> >>> -    if (skb_headroom(skb) < overhead) {
>> >>> -            struct sk_buff *skb2 = skb_copy_expand(skb,
>> >>> -                    overhead, 0, flags);
>> >>> -            dev_kfree_skb_any(skb);
>> >>> -            skb = skb2;
>> >>> -            if (!skb)
>> >>> -                    return NULL;
>> >>> +    /* Make writable and expand space by overhead if required */
>> >>> +    if (skb_cow(skb, overhead)) {
>> >>> +            return NULL;
>> >>>      }
>> >>
>> >> Note that this patch will probably force a copy of all locally generated
>> >> TCP packets.
>> >>
>> >> For them skb_cloned(skb) is true.
>> >>
>> >> I do believe skb_cow_head() would be better, since TCP stack uses the
>> >> __skb_header_release() helper to tell lower stacks they can write the
>> >> header part, even on a clone.
>> >
>> > Agreed.
>>
>> I'm happy to work it as you see fit - you know this code far better than I do.
>>
>> Our reading of the code is that the software checksum path is
>> modifying the data rather than just adding a header. Based on the
>> description of skb_cow_head it therefore isn't appropriate. If that
>> isn't a concern in reality then skb_cow_head is fine and I'll make a
>> V2 patchset.
>> Or do we need to skb_cow if doing the software checksum, but
>> skb_cow_head normally? That can be done instead but requires a
>> slightly larger change.
>>
>> The failure case we were seeing was with a bridged network using
>> SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
>> making an SKB clone of broadcasts for the two interfaces, and then
>> both drivers were adding headers without checking skb_cloned(skb)
>> first, hence trampling on each other. For small packets the SMSC95xx
>> driver will be computing the software checksum and writing it in to
>> the data, so the wifi driver will also be seeing it. For many drivers
>> that probably won't matter, but is that always true?
>>
>> (Patches for the Broadcom wifi driver will be coming once we've worked
>> out the best way of fixing this - there is no error path easily
>> available if the skb_cow_head call fails).
>>
>
> You misread what the driver does.
>
> The TCP data (payload) is _not_ modified.
>
> Only additional headers are pushed in front of the existing (Ethernet,
> IP, TCP) headers.
>
> For this, skb_cow_head() is the perfect solution.
>

OK, will modify the patch, commit and resubmit. Thanks

James

Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[Florian Fainelli]

On 04/18/2017 09:52 AM, Eric Dumazet wrote:
> On Tue, 2017-04-18 at 17:16 +0100, James Hughes wrote:
>> On 18 April 2017 at 16:55, David Miller <davem@davemloft.net> wrote:
>>> From: Eric Dumazet <eric.dumazet@gmail.com>
>>> Date: Tue, 18 Apr 2017 08:51:51 -0700
>>>
>>>> On Tue, 2017-04-18 at 15:48 +0100, James Hughes wrote:
>>>>> The driver was failing to check that the SKB wasn't cloned
>>>>> before adding checksum data or adding header data.
>>>>> Replace existing handling to extend the buffer with
>>>>> skb_cow. Don't use skb_cow_head as the sw checksum
>>>>> code modifies the data portion.
>>>>>
>>>>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>>>>> ---
>>>>>  drivers/net/usb/smsc95xx.c | 10 +++-------
>>>>>  1 file changed, 3 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
>>>>> index df60c98..04f6397 100644
>>>>> --- a/drivers/net/usb/smsc95xx.c
>>>>> +++ b/drivers/net/usb/smsc95xx.c
>>>>> @@ -2067,13 +2067,9 @@ static struct sk_buff *smsc95xx_tx_fixup(struct usbnet *dev,
>>>>>      /* We do not advertise SG, so skbs should be already linearized */
>>>>>      BUG_ON(skb_shinfo(skb)->nr_frags);
>>>>>
>>>>> -    if (skb_headroom(skb) < overhead) {
>>>>> -            struct sk_buff *skb2 = skb_copy_expand(skb,
>>>>> -                    overhead, 0, flags);
>>>>> -            dev_kfree_skb_any(skb);
>>>>> -            skb = skb2;
>>>>> -            if (!skb)
>>>>> -                    return NULL;
>>>>> +    /* Make writable and expand space by overhead if required */
>>>>> +    if (skb_cow(skb, overhead)) {
>>>>> +            return NULL;
>>>>>      }
>>>>
>>>> Note that this patch will probably force a copy of all locally generated
>>>> TCP packets.
>>>>
>>>> For them skb_cloned(skb) is true.
>>>>
>>>> I do believe skb_cow_head() would be better, since TCP stack uses the
>>>> __skb_header_release() helper to tell lower stacks they can write the
>>>> header part, even on a clone.
>>>
>>> Agreed.
>>
>> I'm happy to work it as you see fit - you know this code far better than I do.
>>
>> Our reading of the code is that the software checksum path is
>> modifying the data rather than just adding a header. Based on the
>> description of skb_cow_head it therefore isn't appropriate. If that
>> isn't a concern in reality then skb_cow_head is fine and I'll make a
>> V2 patchset.
>> Or do we need to skb_cow if doing the software checksum, but
>> skb_cow_head normally? That can be done instead but requires a
>> slightly larger change.
>>
>> The failure case we were seeing was with a bridged network using
>> SMSC9514 and a Broadcom wifi chip on Raspberry Pi 3. The bridge was
>> making an SKB clone of broadcasts for the two interfaces, and then
>> both drivers were adding headers without checking skb_cloned(skb)
>> first, hence trampling on each other. For small packets the SMSC95xx
>> driver will be computing the software checksum and writing it in to
>> the data, so the wifi driver will also be seeing it. For many drivers
>> that probably won't matter, but is that always true?
>>
>> (Patches for the Broadcom wifi driver will be coming once we've worked
>> out the best way of fixing this - there is no error path easily
>> available if the skb_cow_head call fails).
>>
> 
> You misread what the driver does.
> 
> The TCP data (payload) is _not_ modified.
> 
> Only additional headers are pushed in front of the existing (Ethernet,
> IP, TCP) headers.
> 
> For this, skb_cow_head() is the perfect solution.

BTW, this pattern of using skb_headroom() ... skb_copy_expand() seems to
be recurring in pretty much all USB network drivers that have a tx_fixup
callback set. The problem is that each driver needs its own
headroom/tailroom so the fix is not as simple as putting the
skb_cow_head() before the call to the drivers' tx_fixup...

I wonder if a coccinelle patch would be able to do that for us?
-- 
Florian

Re: [PATCH] smsc95xx: Use skb_cow to deal with cloned skbs

[Eric Dumazet]

On Tue, 2017-04-18 at 10:10 -0700, Florian Fainelli wrote:

> BTW, this pattern of using skb_headroom() ... skb_copy_expand() seems to
> be recurring in pretty much all USB network drivers that have a tx_fixup
> callback set. The problem is that each driver needs its own
> headroom/tailroom so the fix is not as simple as putting the
> skb_cow_head() before the call to the drivers' tx_fixup...
> 
> I wonder if a coccinelle patch would be able to do that for us?

Not sure about coccinelle, because some drivers also need some tailroom.