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

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

[Jesper Juhl]

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?


Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
---

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

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

[Stephen Frost]

[-- Attachment #1: Type: text/plain, Size: 1813 bytes --]

Greetings,

* Jesper Juhl (juhl-lkml@dif.dk) 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@snowman.net>

		Enjoy,

			Stephen

> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> ---
> 
>  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 #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[David S. Miller]

From: Jesper Juhl <juhl-lkml@dif.dk>
Date: Fri, 17 Jun 2005 00:36:04 +0200 (CEST)

> 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?

It's using it to send a dummy packet to the patch function.
It is gross, but it does work because it allocated it's own
private data area to skb->nh.iph.

Just leave it alone for now, ipt_recent is gross and full of many
errors and bug, and thus stands to have a rewrite. Patrick McHardy
said he will try to do that.

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

[Jesper Juhl]

On 6/17/05, David S. Miller <davem@davemloft.net> wrote:
> From: Jesper Juhl <juhl-lkml@dif.dk>
> Date: Fri, 17 Jun 2005 00:36:04 +0200 (CEST)
> 
> > 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?
> 
> It's using it to send a dummy packet to the patch function.
> It is gross, but it does work because it allocated it's own
> private data area to skb->nh.iph.
> 
> Just leave it alone for now, ipt_recent is gross and full of many
> errors and bug, and thus stands to have a rewrite. Patrick McHardy
> said he will try to do that.
> 
Ok. I was just about to send the patch off to Andrew based on
Stephen's reply, but I'll hold off on that then.

-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

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

[Stephen Frost]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

* David S. Miller (davem@davemloft.net) wrote:
> It's using it to send a dummy packet to the patch function.
> It is gross, but it does work because it allocated it's own
> private data area to skb->nh.iph.

Seriously doubt ipt_recent is alone in that given I based the module off
an existing netfilter module and I'm pretty confident I didn't change
anything with regard to that aspect.

> Just leave it alone for now, ipt_recent is gross and full of many
> errors and bug, and thus stands to have a rewrite. Patrick McHardy
> said he will try to do that.

Ideally it should probably be rolled into the new ippool/ipset
framework, if it's capable of supporting what ipt_recent currently does.
I had heard vaugue claims that the new framework was supposted to be
able to support something like ipt_recent but I havn't looked into it
personally.

I'm mildly curious what the issues you have with it are but I've got
nothing against someone rewriting it as long as the functionality
remains the same.  It'd be nice to have a simpler module (perhaps the
new ippool stuff does this already, not sure) which just has a
hash-based table of IPs to match against since I know alot of people use
ipt_recent for that.  It'd also be nice to be able to do ranges and jump
to specific chains based on a hash-lookup to an IP/range.

	Stephen

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]