* [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
@ 2017-04-18 17:17 James Hughes
2017-04-18 18:58 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: James Hughes @ 2017-04-18 17:17 UTC (permalink / raw)
To: netdev, Steve Glendinning, Microchip Linux Driver Support; +Cc: James Hughes
The driver was failing to check that the SKB wasn't cloned
before adding checksum data.
Replace existing handling to extend/copy the header buffer
with skb_cow_head.
Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
---
Changes in v2
- Changed skb_cow to skb_cow_head as suggested by netdev list
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..094f0ee 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 header space by overhead if required */
+ if (skb_cow_head(skb, overhead)) {
+ return NULL;
}
if (csum) {
--
2.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
2017-04-18 17:17 [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs James Hughes
@ 2017-04-18 18:58 ` Eric Dumazet
2017-04-18 22:09 ` Woojung.Huh
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2017-04-18 18:58 UTC (permalink / raw)
To: James Hughes; +Cc: netdev, Steve Glendinning, Microchip Linux Driver Support
On Tue, 2017-04-18 at 18:17 +0100, James Hughes wrote:
> The driver was failing to check that the SKB wasn't cloned
> before adding checksum data.
> Replace existing handling to extend/copy the header buffer
> with skb_cow_head.
>
> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
> ---
> Changes in v2
> - Changed skb_cow to skb_cow_head as suggested by netdev list
>
>
> 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..094f0ee 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 header space by overhead if required */
> + if (skb_cow_head(skb, overhead)) {
I believe you still need to
dev_kfree_skb_any(skb);
> + return NULL;
> }
>
> if (csum) {
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
2017-04-18 18:58 ` Eric Dumazet
@ 2017-04-18 22:09 ` Woojung.Huh
2017-04-18 22:46 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Woojung.Huh @ 2017-04-18 22:09 UTC (permalink / raw)
To: eric.dumazet, james.hughes; +Cc: netdev, steve.glendinning, UNGLinuxDriver
> > @@ -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 header space by overhead if required
> */
> > + if (skb_cow_head(skb, overhead)) {
>
> I believe you still need to
> dev_kfree_skb_any(skb);
>
I think caller of usbnet_start_xmit() takes care of free when return NULL.
- Woojung
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
2017-04-18 22:09 ` Woojung.Huh
@ 2017-04-18 22:46 ` Eric Dumazet
2017-04-18 23:18 ` Woojung.Huh
2017-04-19 8:33 ` James Hughes
0 siblings, 2 replies; 7+ messages in thread
From: Eric Dumazet @ 2017-04-18 22:46 UTC (permalink / raw)
To: Woojung.Huh; +Cc: james.hughes, netdev, steve.glendinning, UNGLinuxDriver
On Tue, 2017-04-18 at 22:09 +0000, Woojung.Huh@microchip.com wrote:
> > > @@ -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 header space by overhead if required
> > */
> > > + if (skb_cow_head(skb, overhead)) {
> >
> > I believe you still need to
> > dev_kfree_skb_any(skb);
> >
> I think caller of usbnet_start_xmit() takes care of free when return NULL.
I do not think so. Here is the code in usbnet_start_xmit() :
if (info->tx_fixup) {
skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
if (!skb) { // Note that skb is NULL now
if (info->flags & FLAG_MULTI_PACKET)
goto not_drop;
netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
goto drop;
If you really think about it (even before double checking the code in
usbnet_start_xmit()), that would have been a very serious bug.
Calling dev_kfree_skb_any(skb) in this fixup code, then later from
usbnet_start_xmit() would have been a double free, or use after free.
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
2017-04-18 22:46 ` Eric Dumazet
@ 2017-04-18 23:18 ` Woojung.Huh
2017-04-19 8:33 ` James Hughes
1 sibling, 0 replies; 7+ messages in thread
From: Woojung.Huh @ 2017-04-18 23:18 UTC (permalink / raw)
To: eric.dumazet; +Cc: james.hughes, netdev, steve.glendinning, UNGLinuxDriver
> if (info->tx_fixup) {
> skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
>
> if (!skb) { // Note that skb is NULL now
You are right. skb is return value of tx_fixup().
- Woojung
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
2017-04-18 22:46 ` Eric Dumazet
2017-04-18 23:18 ` Woojung.Huh
@ 2017-04-19 8:33 ` James Hughes
2017-04-19 9:40 ` James Hughes
1 sibling, 1 reply; 7+ messages in thread
From: James Hughes @ 2017-04-19 8:33 UTC (permalink / raw)
To: Eric Dumazet
Cc: Woojung.Huh, netdev, Steve Glendinning, Microchip Linux Driver Support
On 18 April 2017 at 23:46, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2017-04-18 at 22:09 +0000, Woojung.Huh@microchip.com wrote:
>> > > @@ -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 header space by overhead if required
>> > */
>> > > + if (skb_cow_head(skb, overhead)) {
>> >
>> > I believe you still need to
>> > dev_kfree_skb_any(skb);
>> >
>> I think caller of usbnet_start_xmit() takes care of free when return NULL.
>
> I do not think so. Here is the code in usbnet_start_xmit() :
>
> if (info->tx_fixup) {
> skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
>
> if (!skb) { // Note that skb is NULL now
>
> if (info->flags & FLAG_MULTI_PACKET)
> goto not_drop;
> netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
> goto drop;
>
>
> If you really think about it (even before double checking the code in
> usbnet_start_xmit()), that would have been a very serious bug.
>
> Calling dev_kfree_skb_any(skb) in this fixup code, then later from
> usbnet_start_xmit() would have been a double free, or use after free.
>
>
So, I still need to return NULL (as per the code this is replacing) to
indicate failure, but need to free the skb prior to return, as per
fragment below.
/* Make writable and expand header space by overhead if required */
if (skb_cow_head(skb, overhead)) {
dev_kfree_skb_any(skb);
return NULL;
}
Once confirmed, I'll do a another patch.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs
2017-04-19 8:33 ` James Hughes
@ 2017-04-19 9:40 ` James Hughes
0 siblings, 0 replies; 7+ messages in thread
From: James Hughes @ 2017-04-19 9:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: Woojung.Huh, netdev, Steve Glendinning, Microchip Linux Driver Support
On 19 April 2017 at 09:33, James Hughes <james.hughes@raspberrypi.org> wrote:
> On 18 April 2017 at 23:46, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> On Tue, 2017-04-18 at 22:09 +0000, Woojung.Huh@microchip.com wrote:
>>> > > @@ -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 header space by overhead if required
>>> > */
>>> > > + if (skb_cow_head(skb, overhead)) {
>>> >
>>> > I believe you still need to
>>> > dev_kfree_skb_any(skb);
>>> >
>>> I think caller of usbnet_start_xmit() takes care of free when return NULL.
>>
>> I do not think so. Here is the code in usbnet_start_xmit() :
>>
>> if (info->tx_fixup) {
>> skb = info->tx_fixup (dev, skb, GFP_ATOMIC);
>>
>> if (!skb) { // Note that skb is NULL now
>>
>> if (info->flags & FLAG_MULTI_PACKET)
>> goto not_drop;
>> netif_dbg(dev, tx_err, dev->net, "can't tx_fixup skb\n");
>> goto drop;
>>
>>
>> If you really think about it (even before double checking the code in
>> usbnet_start_xmit()), that would have been a very serious bug.
>>
>> Calling dev_kfree_skb_any(skb) in this fixup code, then later from
>> usbnet_start_xmit() would have been a double free, or use after free.
>>
>>
>
> So, I still need to return NULL (as per the code this is replacing) to
> indicate failure, but need to free the skb prior to return, as per
> fragment below.
>
> /* Make writable and expand header space by overhead if required */
> if (skb_cow_head(skb, overhead)) {
> dev_kfree_skb_any(skb);
> return NULL;
> }
>
> Once confirmed, I'll do a another patch.
Actually, having checked the code path, doing the free here does look
like the correct operation. Returning NULL indicates to the caller
that the call has failed, but also means the skb is not deallocated
later on. It's seems rather a clunky mechanism for the driver to have
to deallocate, but I guess that's 'just the way it is'.
I'll do another patch.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-19 9:40 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-18 17:17 [PATCH v2] smsc95xx: Use skb_cow_head to deal with cloned skbs James Hughes
2017-04-18 18:58 ` Eric Dumazet
2017-04-18 22:09 ` Woojung.Huh
2017-04-18 22:46 ` Eric Dumazet
2017-04-18 23:18 ` Woojung.Huh
2017-04-19 8:33 ` James Hughes
2017-04-19 9:40 ` 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.