[Click] 2.6 update - Bug + Fix

[Eddie Kohler]

Beyers,

Thanks much for this fix!  However there are a couple problems with it.  You 
say this:

!(atomic_read(&skb_shinfo(skb)->dataref) - skb->nohdr ? (1 << 
SKB_DATAREF_SHIFT) + 1 : 1))

Because of C precedence rules this gets parsed as

!( (atomic_read(&skb_shinfo(skb)->dataref) - skb->nohdr)
    ? (1 << SKB_DATAREF_SHIFT) + 1
    : 1 )

which is always true.  So skbs never get recycled in your code.

I've checked in a different fix, which changes the if statement to:

		if (skb->fclone == SKB_FCLONE_UNAVAILABLE
		    && (!skb->cloned ||
			atomic_read(&skb_shinfo(skb)->dataref) == (skb->nohdr ? (1 << 
SKB_DATAREF_SHIFT) + 1 : 1))) {

There is no need for a following atomic_sub, since dataref is set to 1 below 
anyway.

Does this work for you?
Eddie



Beyers Cronje wrote:
> Hi Eddie,
> 
> I found the bug I reported earlier on the new etc/linux-2.6.16.13-patch 
> you posted below. The problem is that skb_recycle does a 
> atomic_sub_return on the skb dataref if the skb is cloned. If the skb 
> data-reference is more than 1, ie shared by another skb and the 
> recycling condition fails, skb_recycle calls  kfree_skbmem, which in 
> turns  calls skb_release_data which does another atomic_sub_return on 
> the dataref of the skb. This double subtraction on dataref on the same 
> skb will obviously cause skb_release_data to prematurely free the packet 
> data.
> 
> The attached patch fixes this by first checking using atomic_read on 
> dataref, and only calls atomic_sub after we know we can recycle the skb.
> 
> I'm not sure what performance impact the additional atomic operation 
> will have.
> 
> Cheers
> 
> Beyers Cronje
> 
>          if (skb->fclone == SKB_FCLONE_UNAVAILABLE
>              && (!skb->cloned ||
> -            !atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 
> 1 : 1,
> -                       &skb_shinfo(skb)->dataref))) {
> +            !(atomic_read(&skb_shinfo(skb)->dataref) - skb->nohdr ? (1 
> << SKB_DATAREF_SHIFT) + 1 : 1))) {
> +            if (skb->cloned)
> +                atomic_sub(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 
> 1,&skb_shinfo(skb)->dataref);
>              if (skb_shinfo(skb)->nr_frags) {
>                  int i;
>                  for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)
> 
> 
> 
> On 6/23/06, *Eddie Kohler* <kohler at cs.ucla.edu 
> <mailto:kohler at cs.ucla.edu>> wrote:
> 
>     Hi all,
> 
>     A brief update on Linux 2.6 kernel module support.  We've just
>     checked in a
>     fix that plugs a large memory leak of sk_buffs.  You will need to
>     re-patch
>     your kernels with the current etc/linux-2.6.16.13-patch .  FYI.
> 
>     Eddie
>     _______________________________________________
>     click mailing list
>     click at amsterdam.lcs.mit.edu <mailto:click at amsterdam.lcs.mit.edu>
>     https://amsterdam.lcs.mit.edu/mailman/listinfo/click
> 
> 
> 
> ------------------------------------------------------------------------
> 
> --- click.skbuff.c	2006-06-28 07:05:40.000000000 +0200
> +++ src/linux-2.6.16.13/net/core/skbuff.c	2006-06-28 06:21:23.000000000 +0200
> @@ -570,8 +570,9 @@
>  
>  		if (skb->fclone == SKB_FCLONE_UNAVAILABLE
>  		    && (!skb->cloned ||
> -			!atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
> -					   &skb_shinfo(skb)->dataref))) {
> +		    !(atomic_read(&skb_shinfo(skb)->dataref) - skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1))) {
> +			if (skb->cloned)
> +				atomic_sub(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,&skb_shinfo(skb)->dataref);
>  			if (skb_shinfo(skb)->nr_frags) {
>  				int i;
>  				for (i = 0; i < skb_shinfo(skb)->nr_frags; i++)