[PATCH] kfree_skb() bug in 2.4.22

[PATCH] kfree_skb() bug in 2.4.22

[Tobias DiPasquale]

Hi all,

I was debugging one of my iptables/netfilter modules yesterday and I
came across this bug in kfree_skb(). One of my functions returns a
struct skbuff * on success and NULL on failure. When it failed, the code
calling said function attempted to free the struct skbuff *, which at
that point was NULL. This produced a kernel panic. I investigated the
problem and found that, not only should I be checking for a NULL pointer
when freeing the struct skbuff *, but the actual cause of the panic was
because kfree_skb() and kfree_skb_fast() do not check for skb==NULL,
either. They immediately attempt to dereference the users field of the
struct skbuff * in order to decrement that reference counter. 

I have come up with a patch that applies to both the 2.4.22 pristine
source tree and the 2.4.23-pre6 source tree that solves this issue (see
below). I tried to follow Documentation/SubmittingPatches to the letter;
please let me know if I failed this in some way. Thanks :)

P.S. I wasn't sure who exactly maintains this particular code; that's
why I just sent it to everyone listed in MAINTAINERS remotely associated
with include/linux/skbuff.h ;)


--- include/linux/skbuff.h.orig	2003-10-08 07:52:31.000000000 -0400
+++ include/linux/skbuff.h	2003-10-08 07:52:52.000000000 -0400
@@ -293,6 +293,8 @@
  
 static inline void kfree_skb(struct sk_buff *skb)
 {
+	if (!skb)
+		return;
 	if (atomic_read(&skb->users) == 1 || atomic_dec_and_test(&skb->users))
 		__kfree_skb(skb);
 }
@@ -300,6 +302,8 @@
 /* Use this if you didn't touch the skb state [for fast switching] */
 static inline void kfree_skb_fast(struct sk_buff *skb)
 {
+	if (!skb)
+		return;
 	if (atomic_read(&skb->users) == 1 || atomic_dec_and_test(&skb->users))
 		kfree_skbmem(skb);	
 }

Re: [PATCH] kfree_skb() bug in 2.4.22

[Jeff Garzik]

Tobias DiPasquale wrote:
> Hi all,
> 
> I was debugging one of my iptables/netfilter modules yesterday and I
> came across this bug in kfree_skb(). One of my functions returns a
> struct skbuff * on success and NULL on failure. When it failed, the code
> calling said function attempted to free the struct skbuff *, which at
> that point was NULL. This produced a kernel panic. I investigated the
> problem and found that, not only should I be checking for a NULL pointer
> when freeing the struct skbuff *, but the actual cause of the panic was
> because kfree_skb() and kfree_skb_fast() do not check for skb==NULL,
> either. They immediately attempt to dereference the users field of the
> struct skbuff * in order to decrement that reference counter. 


I would prefer that you fix your code instead, to not pass NULL to 
kfree_skb()...

Re: [PATCH] kfree_skb() bug in 2.4.22

[David S. Miller]

On Wed, 08 Oct 2003 09:09:48 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> I would prefer that you fix your code instead, to not pass NULL to 
> kfree_skb()...

Absolutely, there is no valid reason to pass NULL into these
routines.

Re: [PATCH] kfree_skb() bug in 2.4.22

[Ingo Oeser]

On Wednesday 08 October 2003 15:47, David S. Miller wrote:
> On Wed, 08 Oct 2003 09:09:48 -0400
>
> Jeff Garzik <jgarzik@pobox.com> wrote:
> > I would prefer that you fix your code instead, to not pass NULL to
> > kfree_skb()...
>
> Absolutely, there is no valid reason to pass NULL into these
> routines.

Would you mind __attribute_nonnull__ for these functions, if we
enable GCC 3.3 support for this[1]?


[1] Which includes editing the compiler.h and gcc3-compiler.h and so on.

Regards

Ingo Oeser

Re: [PATCH] kfree_skb() bug in 2.4.22

[David S. Miller]

On Fri, 10 Oct 2003 14:53:44 +0200
Ingo Oeser <ioe-lkml@rameria.de> wrote:

> On Wednesday 08 October 2003 15:47, David S. Miller wrote:
> > On Wed, 08 Oct 2003 09:09:48 -0400
> >
> > Jeff Garzik <jgarzik@pobox.com> wrote:
> > > I would prefer that you fix your code instead, to not pass NULL to
> > > kfree_skb()...
> >
> > Absolutely, there is no valid reason to pass NULL into these
> > routines.
> 
> Would you mind __attribute_nonnull__ for these functions, if we
> enable GCC 3.3 support for this[1]?

I would say yes, but why?  All this attribute does is optimize
away tests for NULL which surprise surprise we don't have any
of in kfree_skb().

Re: [PATCH] kfree_skb() bug in 2.4.22

[Ingo Oeser]

On Friday 10 October 2003 15:00, David S. Miller wrote:
> Ingo Oeser <ioe-lkml@rameria.de> wrote:
> > Would you mind __attribute_nonnull__ for these functions, if we
> > enable GCC 3.3 support for this[1]?
>
> I would say yes, but why?  All this attribute does is optimize
> away tests for NULL which surprise surprise we don't have any
> of in kfree_skb().

And it wouldn't warn about passing NULL to these functions? That's bad...
But maybe sparse/smatch are better for this...

Re: [PATCH] kfree_skb() bug in 2.4.22

[Dan Kegel]

Ingo Oeser wrote:
> On Friday 10 October 2003 15:00, David S. Miller wrote:
> 
>>Ingo Oeser <ioe-lkml@rameria.de> wrote:
>>
>>>Would you mind __attribute_nonnull__ for these functions, if we
>>>enable GCC 3.3 support for this[1]?
>>
>>I would say yes, but why?  All this attribute does is optimize
>>away tests for NULL which surprise surprise we don't have any
>>of in kfree_skb().
> 
> 
> And it wouldn't warn about passing NULL to these functions? That's bad...
> But maybe sparse/smatch are better for this...

Things like smatch, sparse, and checker can use the __attribute_nonnull__.
I'd say it's a good idea.  Should I submit the patch, then, since I'm
the one who like the idea?
- Dan

-- 
Dan Kegel
http://www.kegel.com
http://counter.li.org/cgi-bin/runscript/display-person.cgi?user=78045

Re: [PATCH] kfree_skb() bug in 2.4.22

[Tobias DiPasquale]

On Wed, 2003-10-08 at 09:09, Jeff Garzik wrote:
> Tobias DiPasquale wrote:
> > Hi all,
> > 
> > I was debugging one of my iptables/netfilter modules yesterday and I
> > came across this bug in kfree_skb(). One of my functions returns a
> > struct skbuff * on success and NULL on failure. When it failed, the code
> > calling said function attempted to free the struct skbuff *, which at
> > that point was NULL. This produced a kernel panic. I investigated the
> > problem and found that, not only should I be checking for a NULL pointer
> > when freeing the struct skbuff *, but the actual cause of the panic was
> > because kfree_skb() and kfree_skb_fast() do not check for skb==NULL,
> > either. They immediately attempt to dereference the users field of the
> > struct skbuff * in order to decrement that reference counter. 
> 
> 
> I would prefer that you fix your code instead, to not pass NULL to 
> kfree_skb()...
> 

Well, I certainly have done that already ;-) But I have checked kfree()
and vfree() and they have a sanity check for NULL before processing, as
well as those are also the well-known semantics for the userspace free()
call. It seems to me (and I recognize that my understanding is limited)
that it could do no harm and may even help in certain cases. Am I
missing something in why it would be preferable _not_ to check for NULL
in kfree_skb()? Is it a performance issue associated with the extra
overhead of having to check for NULL on every kfree_skb[_fast]() call?
And, if so, could we possibly document in the source code and/or kernel
documentation in order to let less experienced programmers know that
they should under no circumstances pass NULL into these functions? I
certainly didn't know that, since I was working off of the semantics of
the other kernel *free() functions. Help me understand my error in
judgement. Thanks :)

Re: [PATCH] kfree_skb() bug in 2.4.22

[David S. Miller]

On Wed, 08 Oct 2003 10:11:43 -0400
Tobias DiPasquale <toby@cbcg.net> wrote:

> Well, I certainly have done that already ;-) But I have checked kfree()
> and vfree() and they have a sanity check for NULL before processing, as
> well as those are also the well-known semantics for the userspace free()
> call.

So what?  Those are totally different APIs and they in no way determine
how other interfaces should behave.

Passing NULL pointers around usually indicates poorly designed
software anyways (unless the NULL pointer is being returned by
a routine to indicate an allocation failure).

This isn't even worth discussing anymore.