Re: Shouldn't we be using alloc_skb/kfree_skb in net/ipv4/netfilter/ipt_recent.c::ip_recent_ctrl ?

[Stephen Frost]

Greetings,

* Jesper Juhl (juhl-lkml@xxxxxx) wrote:
> I was just grep'ing through the source looking for places where skb's 
> might be freed by plain kfree() and, amongst other things, I noticed 
> net/ipv4/netfilter/ipt_recent.c::ip_recent_ctrl, where a struct sk_buff* 
> is defined and then storage for it is allocated with kmalloc() and freed 
> with kfree(), and I'm wondering if we shouldn't be using 
> alloc_skb/kfree_skb instead (as pr the patch below)? Or is there some good 
> reason for doing it the way it's currently done?

  This sounds reasonable to me.  I'm about 99% sure I just based that
  usage off some other usage in the kernel I found back when I
  originally wrote the module.

  So, for what it's worth I suppose (if anything, and I'm probably not
  doing it right anyway)-

Signed-off-by: Stephen Frost <sfrost@xxxxxxxxxxx>

		Enjoy,

			Stephen

> Signed-off-by: Jesper Juhl <juhl-lkml@xxxxxx>
> ---
> 
>  net/ipv4/netfilter/ipt_recent.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> --- linux-2.6.12-rc6-mm1-orig/net/ipv4/netfilter/ipt_recent.c	2005-06-12 15:58:58.000000000 +0200
> +++ linux-2.6.12-rc6-mm1/net/ipv4/netfilter/ipt_recent.c	2005-06-16 23:41:55.000000000 +0200
> @@ -303,7 +303,7 @@ static int ip_recent_ctrl(struct file *f
>  	strncpy(info->name,curr_table->name,IPT_RECENT_NAME_LEN);
>  	info->name[IPT_RECENT_NAME_LEN-1] = '\0';
>  
> -	skb = kmalloc(sizeof(struct sk_buff),GFP_KERNEL);
> +	skb = alloc_skb(sizeof(struct sk_buff),GFP_KERNEL);
>  	if (!skb) {
>  		used = -ENOMEM;
>  		goto out_free_info;
> @@ -322,7 +322,7 @@ static int ip_recent_ctrl(struct file *f
>  
>  	kfree(skb->nh.iph);
>  out_free_skb:
> -	kfree(skb);
> +	kfree_skb(skb);
>  out_free_info:
>  	kfree(info);
>  
>

Attachment: signature.asc
Description: Digital signature