[PATCH] Make dev_kfree_skb_any check if the skb is valid

[PATCH] Make dev_kfree_skb_any check if the skb is valid

[Erik Hovland]

If dev_kfree_skb_any is called and it then calls dev_kfree_skb_irq.
That call will dereference the skb. If the skb is invalid, down the
drain we go.

This one-liner checks to see if the skb is valid as part of the
determination of whether to call dev_kfree_skb_irq.

Signed-off-by: Erik Hovland <erik@hovland.org>

---

 net/core/dev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 4dc93cc..85f4a4c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1131,7 +1131,7 @@ EXPORT_SYMBOL(__netif_rx_schedule);
 
 void dev_kfree_skb_any(struct sk_buff *skb)
 {
-	if (in_irq() || irqs_disabled())
+	if (skb && (in_irq() || irqs_disabled()))
 		dev_kfree_skb_irq(skb);
 	else
 		dev_kfree_skb(skb);

Re: [PATCH] Make dev_kfree_skb_any check if the skb is valid

[David Miller]

From: Erik Hovland <erik@hovland.org>
Date: Wed, 18 Apr 2007 15:33:44 -0700

> If dev_kfree_skb_any is called and it then calls dev_kfree_skb_irq.
> That call will dereference the skb. If the skb is invalid, down the
> drain we go.
> 
> This one-liner checks to see if the skb is valid as part of the
> determination of whether to call dev_kfree_skb_irq.
> 
> Signed-off-by: Erik Hovland <erik@hovland.org>

This should never be invoked with a NULL skb argument.

Who is doing that?

Re: [PATCH] Make dev_kfree_skb_any check if the skb is valid

[Erik Hovland]

On Wed, Apr 18, 2007 at 03:44:16PM -0700, David Miller wrote:
> From: Erik Hovland <erik@hovland.org>
> Date: Wed, 18 Apr 2007 15:33:44 -0700
> 
> > If dev_kfree_skb_any is called and it then calls dev_kfree_skb_irq.
> > That call will dereference the skb. If the skb is invalid, down the
> > drain we go.
> > 
> > This one-liner checks to see if the skb is valid as part of the
> > determination of whether to call dev_kfree_skb_irq.
> > 
> > Signed-off-by: Erik Hovland <erik@hovland.org>
> 
> This should never be invoked with a NULL skb argument.
>
> Who is doing that?

Heh, the reason I came up with this patch is that the code in
drivers/usb/gadget/ether.c at about line 1653 will attempt to allocate
an skb. If it fails then it uses a goto to jump to line 1672 where it
will call dev_kfree_skb_any (skb) on a potentially null skb. I put a
validity check there and sent it off to the USB gadget maintainer. He
asked me to instead make the dev_kfree_skb_any call more robust and see
how that went over on the netdev list. Like a lead balloon?

E

-- 
Erik Hovland
mail: erik@hovland.org
web: http://hovland.org/
PGP/GPG public key available on request

Re: [PATCH] Make dev_kfree_skb_any check if the skb is valid

[David Miller]

From: Erik Hovland <erik@hovland.org>
Date: Wed, 18 Apr 2007 16:18:15 -0700

> Heh, the reason I came up with this patch is that the code in
> drivers/usb/gadget/ether.c at about line 1653 will attempt to allocate
> an skb. If it fails then it uses a goto to jump to line 1672 where it
> will call dev_kfree_skb_any (skb) on a potentially null skb. I put a
> validity check there and sent it off to the USB gadget maintainer. He
> asked me to instead make the dev_kfree_skb_any call more robust and see
> how that went over on the netdev list. Like a lead balloon?

Yep, like a lead baloon.

The USB gadget driver maintainer should accept your patch
to check for NULL in the gadget driver as that is the one
and only case in the entire tree where that can happen and
we're not eating a conditional and a return just for that
one esoteric case.