[KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5

[KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5

[Anne Thrax]

Replaced a kfree() with a kfree_skb() when freeing an sk_buff.

--- /usr/src/linux-2.6.17-rc2-git5-orig/net/ipv4/netfilter/ipt_recent.c 
2006-04-23 16:47:53.000000000 -0500
+++ /usr/src/linux-2.6.17-rc2-git5/net/ipv4/netfilter/ipt_recent.c 
2006-04-23 16:51:09.000000000 -0500
@@ -323,7 +323,7 @@

         kfree(skb->nh.iph);
  out_free_skb:
-       kfree(skb);
+       kfree_skb(skb);
  out_free_info:
         kfree(info);
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5

[Matthew Wilcox]

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

On Mon, Apr 24, 2006 at 12:09:50AM -0400, Anne Thrax wrote:
> Replaced a kfree() with a kfree_skb() when freeing an sk_buff.

But this skbuff wasn't allocated with alloc_skb().  Is this kosher?
Is it also a bug that this skb was kmalloced?

It looks to me like this is a fake skb and it should be kfree'd,
but I'd welcome more eyes on this one.

> --- /usr/src/linux-2.6.17-rc2-git5-orig/net/ipv4/netfilter/ipt_recent.c 
> 2006-04-23 16:47:53.000000000 -0500
> +++ /usr/src/linux-2.6.17-rc2-git5/net/ipv4/netfilter/ipt_recent.c 
> 2006-04-23 16:51:09.000000000 -0500
> @@ -323,7 +323,7 @@
> 
>         kfree(skb->nh.iph);
>  out_free_skb:
> -       kfree(skb);
> +       kfree_skb(skb);
>  out_free_info:
>         kfree(info);
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5

[Anne Thrax]

>
> But this skbuff wasn't allocated with alloc_skb().  Is this kosher?
> Is it also a bug that this skb was kmalloced?
>
> It looks to me like this is a fake skb and it should be kfree'd,
> but I'd welcome more eyes on this one.
>

Maybe we should then have it alloc_skb() rather than just kmalloc()
The function isn't there for nothing, and it does do some checking
that would not be done through kmalloc() and kfree().

--- /usr/src/linux-2.6.17-rc2-git5-orig/net/ipv4/netfilter/ipt_recent.c 
2006-04-23 16:47:53.000000000 -0500
+++ /usr/src/linux-2.6.17-rc2-git5/net/ipv4/netfilter/ipt_recent.c 
2006-04-23 23:50:50.000000000 -0500
@@ -304,7 +304,7 @@
        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;
@@ -323,7 +323,7 @@

        kfree(skb->nh.iph);
 out_free_skb:
-       kfree(skb);
+       kfree_skb(skb);
 out_free_info:
        kfree(info);


While reading the function alloc_skb(), you can request it to be
a certain size. But why would anyone want to allocate a different
size than sizeof(sk_buff)? Unless there is something I'm missing
here, why don't we just remove the size parameter altogether?
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5

[Matthew Wilcox]

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

On Mon, Apr 24, 2006 at 04:39:10PM -0400, Anne Thrax wrote:
> >
> >But this skbuff wasn't allocated with alloc_skb().  Is this kosher?
> >Is it also a bug that this skb was kmalloced?
> >
> >It looks to me like this is a fake skb and it should be kfree'd,
> >but I'd welcome more eyes on this one.
> 
> Maybe we should then have it alloc_skb() rather than just kmalloc()
> The function isn't there for nothing, and it does do some checking
> that would not be done through kmalloc() and kfree().

But then, alloc_skb() does a lot of work that kmalloc doesn't do.
If the author knows that work is irrelevant, then this is a good
optimisation, and perhaps even necessary given that this code path is
potentially called for every packet.

I don't know; you may be right.  Let's see if the author can explain why
he's kmallocing instead of skb_allocing

> --- /usr/src/linux-2.6.17-rc2-git5-orig/net/ipv4/netfilter/ipt_recent.c 
> 2006-04-23 16:47:53.000000000 -0500
> +++ /usr/src/linux-2.6.17-rc2-git5/net/ipv4/netfilter/ipt_recent.c 
> 2006-04-23 23:50:50.000000000 -0500
> @@ -304,7 +304,7 @@
>        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;
> @@ -323,7 +323,7 @@
> 
>        kfree(skb->nh.iph);
> out_free_skb:
> -       kfree(skb);
> +       kfree_skb(skb);
> out_free_info:
>        kfree(info);
> 
> 
> While reading the function alloc_skb(), you can request it to be
> a certain size. But why would anyone want to allocate a different
> size than sizeof(sk_buff)? Unless there is something I'm missing
> here, why don't we just remove the size parameter altogether?
> _______________________________________________
> Kernel-janitors mailing list
> Kernel-janitors@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/kernel-janitors

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5

[Stephen Frost]

[-- Attachment #1.1: Type: text/plain, Size: 1983 bytes --]

* Matthew Wilcox (matthew@wil.cx) wrote:
> I don't know; you may be right.  Let's see if the author can explain why
> he's kmallocing instead of skb_allocing

Honestly, and I know this is terrible, but, this was my first real
kernel module and I don't think I knew about alloc_skb().  I don't
recall seeing alloc_skb() in the module I was baseing mine off of but
it's possible that module didn't have any reason to use it.  No, I don't
remember which module I was copying (might have been psd?).

Thanks for looking into this.  I don't see any particular reason not to
use alloc_skb(), especially if it's in general the appropriate thing to
do in netfilter modules.

	Thanks again,

		Stephen

> > --- /usr/src/linux-2.6.17-rc2-git5-orig/net/ipv4/netfilter/ipt_recent.c 
> > 2006-04-23 16:47:53.000000000 -0500
> > +++ /usr/src/linux-2.6.17-rc2-git5/net/ipv4/netfilter/ipt_recent.c 
> > 2006-04-23 23:50:50.000000000 -0500
> > @@ -304,7 +304,7 @@
> >        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;
> > @@ -323,7 +323,7 @@
> > 
> >        kfree(skb->nh.iph);
> > out_free_skb:
> > -       kfree(skb);
> > +       kfree_skb(skb);
> > out_free_info:
> >        kfree(info);
> > 
> > 
> > While reading the function alloc_skb(), you can request it to be
> > a certain size. But why would anyone want to allocate a different
> > size than sizeof(sk_buff)? Unless there is something I'm missing
> > here, why don't we just remove the size parameter altogether?
> > _______________________________________________
> > Kernel-janitors mailing list
> > Kernel-janitors@lists.osdl.org
> > https://lists.osdl.org/mailman/listinfo/kernel-janitors

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

[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5

[Anne Thrax]

>
> Honestly, and I know this is terrible, but, this was my first real
> kernel module and I don't think I knew about alloc_skb().
>   

Hehe, that was my first patch.
_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors