net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5
PATCH 1/1 net/ipv4/netfilter/ipt_recent.c 2.6.17-rc2-git5
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
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 1 line · Anne Thrax
Replaced a
kfree()with akfree_skb()when freeing ansk_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.
Quoted 12 lines
---
/usr/src/linux-2.6.17-rc2-git5-orig/net/ipv4/netfilter/ipt_recent.c2006-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(quoted diff) +1
-1@@ -323,7 +323,7 @@ kfree(skb->nh.iph); out_free_skb: - kfree(skb); + kfree_skb(skb); out_free_info: kfree(info);
Quoted 4 lines
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 7 lines
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 [email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 10 lines · Anne Thrax
Quoted 6 lines
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 justkmalloc()The function isn't there for nothing, and it does do some checking that would not be done throughkmalloc()andkfree().
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
Quoted 23 lines
---
/usr/src/linux-2.6.17-rc2-git5-orig/net/ipv4/netfilter/ipt_recent.c2006-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(quoted diff) +1
-1@@ -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:(quoted diff) +1
-1- kfree(skb); + kfree_skb(skb);
out_free_info:kfree(info);
Quoted 8 lines
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 thansizeof(sk_buff)? Unless there is something I'm missing here, why don't we just remove the size parameter altogether?
_______________________________________________
Kernel-janitors mailing list [email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 2 lines · * Matthew Wilcox ([email protected]) 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
Quoted 23 lines
Quoted 23 lines
---
/usr/src/linux-2.6.17-rc2-git5-orig/net/ipv4/netfilter/ipt_recent.c2006-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(quoted diff) +1
-1@@ -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:(quoted diff) +1
-1- kfree(skb); + kfree_skb(skb);
out_free_info:kfree(info);
Quoted 8 lines
Quoted 8 lines
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 thansizeof(sk_buff)? Unless there is something I'm missing here, why don't we just remove the size parameter altogether?
_______________________________________________
Kernel-janitors mailing list [email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
_______________________________________________
Kernel-janitors mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors
Quoted 5 lines
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
[email protected]
https://lists.osdl.org/mailman/listinfo/kernel-janitors