kfree_skb() bug in 2.4.22

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 ;)

@@ -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);	
 }

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

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

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 :)

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.