netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [patch] skbuff: use kfree_skb() instead of kfree()
@ 2011-07-20  7:23 Dan Carpenter
  2011-07-20  7:25 ` Dan Carpenter
  2011-07-20  8:51 ` [patch net-next-2.6 v2] skbuff: fix error handling in pskb_copy() Dan Carpenter
  0 siblings, 2 replies; 10+ messages in thread
From: Dan Carpenter @ 2011-07-20  7:23 UTC (permalink / raw)
  To: Shirley Ma
  Cc: David S. Miller, Eric Dumazet, Michał Mirosław,
	open list:NETWORKING [GENERAL], kernel-janitors

"n" was allocated with alloc_skb() so we should free it with
kfree_skb() instead of regular kfree().

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d220119..cc0c6f0 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -799,7 +799,7 @@ struct sk_buff *pskb_copy(struct sk_buff *skb, gfp_t gfp_mask)
 
 		if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
 			if (skb_copy_ubufs(skb, gfp_mask)) {
-				kfree(n);
+				kfree_skb(n);
 				goto out;
 			}
 			skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [patch] skbuff: use kfree_skb() instead of kfree()
  2011-07-20  7:23 [patch] skbuff: use kfree_skb() instead of kfree() Dan Carpenter
@ 2011-07-20  7:25 ` Dan Carpenter
  2011-07-20  7:42   ` Eric Dumazet
  2011-07-20  8:51 ` [patch net-next-2.6 v2] skbuff: fix error handling in pskb_copy() Dan Carpenter
  1 sibling, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2011-07-20  7:25 UTC (permalink / raw)
  To: Shirley Ma
  Cc: David S. Miller, Eric Dumazet, Michał Mirosław,
	open list:NETWORKING [GENERAL], kernel-janitors

Crap.  Sorry, I shouldn't have sent that.  We shouldn't return the
freed "n" here.  I'll send a v2 shortly.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch] skbuff: use kfree_skb() instead of kfree()
  2011-07-20  7:25 ` Dan Carpenter
@ 2011-07-20  7:42   ` Eric Dumazet
  2011-07-20  8:01     ` Dan Carpenter
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-07-20  7:42 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Shirley Ma, David S. Miller, Michał Mirosław,
	open list:NETWORKING [GENERAL], kernel-janitors

Le mercredi 20 juillet 2011 à 10:25 +0300, Dan Carpenter a écrit :
> Crap.  Sorry, I shouldn't have sent that.  We shouldn't return the
> freed "n" here.  I'll send a v2 shortly.

Also, dont forget to say its a patch for net-next-2.6

The recommended way is to use 

[PATCH net-next-2.6] ...




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch] skbuff: use kfree_skb() instead of kfree()
  2011-07-20  7:42   ` Eric Dumazet
@ 2011-07-20  8:01     ` Dan Carpenter
  2011-07-20  8:21       ` Eric Dumazet
  0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2011-07-20  8:01 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Shirley Ma, David S. Miller, Michał Mirosław,
	open list:NETWORKING [GENERAL], kernel-janitors

On Wed, Jul 20, 2011 at 09:42:03AM +0200, Eric Dumazet wrote:
> Le mercredi 20 juillet 2011 à 10:25 +0300, Dan Carpenter a écrit :
> > Crap.  Sorry, I shouldn't have sent that.  We shouldn't return the
> > freed "n" here.  I'll send a v2 shortly.
> 
> Also, dont forget to say its a patch for net-next-2.6

If you're using linux-next, is there a way to tell which tree a
patch came from?  Obviously in this case it's core networking, but
in other cases how does that work?

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch] skbuff: use kfree_skb() instead of kfree()
  2011-07-20  8:01     ` Dan Carpenter
@ 2011-07-20  8:21       ` Eric Dumazet
  2011-07-20  8:37         ` Julia Lawall
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-07-20  8:21 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Shirley Ma, David S. Miller, Michał Mirosław,
	open list:NETWORKING [GENERAL], kernel-janitors

Le mercredi 20 juillet 2011 à 11:01 +0300, Dan Carpenter a écrit :
> On Wed, Jul 20, 2011 at 09:42:03AM +0200, Eric Dumazet wrote:
> > Le mercredi 20 juillet 2011 à 10:25 +0300, Dan Carpenter a écrit :
> > > Crap.  Sorry, I shouldn't have sent that.  We shouldn't return the
> > > freed "n" here.  I'll send a v2 shortly.
> > 
> > Also, dont forget to say its a patch for net-next-2.6
> 
> If you're using linux-next, is there a way to tell which tree a
> patch came from?  Obviously in this case it's core networking, but
> in other cases how does that work?

In this particular case, David will know for sure since patch is very
recent, but I wanted to make a general advice.

Keep in mind David has to review dozens of patches _per_ day, so netdev
related patches need some extra cooperation from submitters to help the
maintainer.

This extra cooperation means to test the patch on either net-next-2.6 or
net-2.6 tree ;)




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch] skbuff: use kfree_skb() instead of kfree()
  2011-07-20  8:21       ` Eric Dumazet
@ 2011-07-20  8:37         ` Julia Lawall
  2011-07-20 21:03           ` Julie Sullivan
  0 siblings, 1 reply; 10+ messages in thread
From: Julia Lawall @ 2011-07-20  8:37 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Dan Carpenter, Shirley Ma, David S. Miller,
	Michał Mirosław, open list:NETWORKING [GENERAL],
	kernel-janitors

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1236 bytes --]

On Wed, 20 Jul 2011, Eric Dumazet wrote:

> Le mercredi 20 juillet 2011 à 11:01 +0300, Dan Carpenter a écrit :
> > On Wed, Jul 20, 2011 at 09:42:03AM +0200, Eric Dumazet wrote:
> > > Le mercredi 20 juillet 2011 à 10:25 +0300, Dan Carpenter a écrit :
> > > > Crap.  Sorry, I shouldn't have sent that.  We shouldn't return the
> > > > freed "n" here.  I'll send a v2 shortly.
> > > 
> > > Also, dont forget to say its a patch for net-next-2.6
> > 
> > If you're using linux-next, is there a way to tell which tree a
> > patch came from?  Obviously in this case it's core networking, but
> > in other cases how does that work?
> 
> In this particular case, David will know for sure since patch is very
> recent, but I wanted to make a general advice.
> 
> Keep in mind David has to review dozens of patches _per_ day, so netdev
> related patches need some extra cooperation from submitters to help the
> maintainer.
> 
> This extra cooperation means to test the patch on either net-next-2.6 or
> net-2.6 tree ;)

Maybe there is some way to integrate such a suggestion in get_maintainers 
or checkpatch?  Otherwise, those who work on the code in a more breadth 
first way don't have much chance of knowing or remembering this advice.

julia

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [patch net-next-2.6 v2] skbuff: fix error handling in pskb_copy()
  2011-07-20  7:23 [patch] skbuff: use kfree_skb() instead of kfree() Dan Carpenter
  2011-07-20  7:25 ` Dan Carpenter
@ 2011-07-20  8:51 ` Dan Carpenter
  2011-07-20  8:59   ` Eric Dumazet
  1 sibling, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2011-07-20  8:51 UTC (permalink / raw)
  To: Shirley Ma
  Cc: David S. Miller, Eric Dumazet, Michał Mirosław,
	open list:NETWORKING [GENERAL], kernel-janitors

There are two problems:
1) "n" was allocated with alloc_skb() so we should free it with
   kfree_skb() instead of regular kfree().
2) We return the freed pointer instead of NULL.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index d220119..2beda82 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -799,7 +799,8 @@ struct sk_buff *pskb_copy(struct sk_buff *skb, gfp_t gfp_mask)
 
 		if (skb_shinfo(skb)->tx_flags & SKBTX_DEV_ZEROCOPY) {
 			if (skb_copy_ubufs(skb, gfp_mask)) {
-				kfree(n);
+				kfree_skb(n);
+				n = NULL;
 				goto out;
 			}
 			skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [patch net-next-2.6 v2] skbuff: fix error handling in pskb_copy()
  2011-07-20  8:51 ` [patch net-next-2.6 v2] skbuff: fix error handling in pskb_copy() Dan Carpenter
@ 2011-07-20  8:59   ` Eric Dumazet
  2011-07-21 21:48     ` David Miller
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-07-20  8:59 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Shirley Ma, David S. Miller, Michał Mirosław,
	open list:NETWORKING [GENERAL], kernel-janitors

Le mercredi 20 juillet 2011 à 11:51 +0300, Dan Carpenter a écrit :
> There are two problems:
> 1) "n" was allocated with alloc_skb() so we should free it with
>    kfree_skb() instead of regular kfree().
> 2) We return the freed pointer instead of NULL.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Thanks Dan

Reviewed-by: Eric Dumazet <eric.dumazet@gmail.com>



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch] skbuff: use kfree_skb() instead of kfree()
  2011-07-20  8:37         ` Julia Lawall
@ 2011-07-20 21:03           ` Julie Sullivan
  0 siblings, 0 replies; 10+ messages in thread
From: Julie Sullivan @ 2011-07-20 21:03 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Eric Dumazet, Dan Carpenter, Shirley Ma, David S. Miller,
	Michał Mirosław, open list:NETWORKING [GENERAL],
	kernel-janitors

>> > > Also, dont forget to say its a patch for net-next-2.6
>> >
>> > If you're using linux-next, is there a way to tell which tree a
>> > patch came from?  Obviously in this case it's core networking, but
>> > in other cases how does that work?
>>
>> In this particular case, David will know for sure since patch is very
>> recent, but I wanted to make a general advice.
>>
>> Keep in mind David has to review dozens of patches _per_ day, so netdev
>> related patches need some extra cooperation from submitters to help the
>> maintainer.
>>
>> This extra cooperation means to test the patch on either net-next-2.6 or
>> net-2.6 tree ;)
>
> Maybe there is some way to integrate such a suggestion in get_maintainers
> or checkpatch?  Otherwise, those who work on the code in a more breadth
> first way don't have much chance of knowing or remembering this advice.
>
> julia

I think Julia's observation is really on the nail, I wish there were
some way of doing this? If new or random testers or reviewers out
there aren't tracking/following a particular tree/project already -
i.e. if they don't _know_ beforehand, aren't they going to just assume
using linux-next is correct (at least that's what I do)?
Knowing what branch to most productively test patches against
beforehand might encourage more testers and submissions and also could
make maintainer's jobs a bit easier.

Cheers
Julie

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch net-next-2.6 v2] skbuff: fix error handling in pskb_copy()
  2011-07-20  8:59   ` Eric Dumazet
@ 2011-07-21 21:48     ` David Miller
  0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2011-07-21 21:48 UTC (permalink / raw)
  To: eric.dumazet; +Cc: error27, xma, mirq-linux, netdev, kernel-janitors

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 20 Jul 2011 10:59:46 +0200

> Le mercredi 20 juillet 2011 à 11:51 +0300, Dan Carpenter a écrit :
>> There are two problems:
>> 1) "n" was allocated with alloc_skb() so we should free it with
>>    kfree_skb() instead of regular kfree().
>> 2) We return the freed pointer instead of NULL.
>> 
>> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> Thanks Dan
> 
> Reviewed-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks everyone.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2011-07-21 21:49 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-20  7:23 [patch] skbuff: use kfree_skb() instead of kfree() Dan Carpenter
2011-07-20  7:25 ` Dan Carpenter
2011-07-20  7:42   ` Eric Dumazet
2011-07-20  8:01     ` Dan Carpenter
2011-07-20  8:21       ` Eric Dumazet
2011-07-20  8:37         ` Julia Lawall
2011-07-20 21:03           ` Julie Sullivan
2011-07-20  8:51 ` [patch net-next-2.6 v2] skbuff: fix error handling in pskb_copy() Dan Carpenter
2011-07-20  8:59   ` Eric Dumazet
2011-07-21 21:48     ` David Miller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).