* Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5
2006-04-24 4:09 [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5 Anne Thrax
@ 2006-04-24 4:41 ` Matthew Wilcox
2006-04-24 20:39 ` Anne Thrax
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2006-04-24 4:41 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5
2006-04-24 4:09 [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5 Anne Thrax
2006-04-24 4:41 ` Matthew Wilcox
@ 2006-04-24 20:39 ` Anne Thrax
2006-04-24 23:24 ` Matthew Wilcox
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Anne Thrax @ 2006-04-24 20:39 UTC (permalink / raw)
To: kernel-janitors
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5
2006-04-24 4:09 [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5 Anne Thrax
2006-04-24 4:41 ` Matthew Wilcox
2006-04-24 20:39 ` Anne Thrax
@ 2006-04-24 23:24 ` Matthew Wilcox
2006-04-25 0:31 ` Stephen Frost
2006-04-25 1:04 ` Anne Thrax
4 siblings, 0 replies; 6+ messages in thread
From: Matthew Wilcox @ 2006-04-24 23:24 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5
2006-04-24 4:09 [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5 Anne Thrax
` (2 preceding siblings ...)
2006-04-24 23:24 ` Matthew Wilcox
@ 2006-04-25 0:31 ` Stephen Frost
2006-04-25 1:04 ` Anne Thrax
4 siblings, 0 replies; 6+ messages in thread
From: Stephen Frost @ 2006-04-25 0:31 UTC (permalink / raw)
To: kernel-janitors
[-- 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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5
2006-04-24 4:09 [KJ] [PATCH] net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5 Anne Thrax
` (3 preceding siblings ...)
2006-04-25 0:31 ` Stephen Frost
@ 2006-04-25 1:04 ` Anne Thrax
4 siblings, 0 replies; 6+ messages in thread
From: Anne Thrax @ 2006-04-25 1:04 UTC (permalink / raw)
To: kernel-janitors
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread