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



-
To unsubscribe from this list: send the line "unsubscribe linux-net" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html