diff mbox

[net-2.6,2/6] net: remove kfree_skb on a NULL pointer in af_netlink.c

Message ID 20090918005729.25594.14261.stgit@localhost.localdomain
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Kirsher, Jeffrey T Sept. 18, 2009, 12:57 a.m. UTC
From: John Fastabend <john.r.fastabend@intel.com>

This removes a kfree_skb that is being called on a NULL pointer when
do_one_broadcast() is sucessful.  And moves the kfree_skb into
do_one_broadcast() for the error case.

Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 net/netlink/af_netlink.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller Sept. 18, 2009, 1:24 a.m. UTC | #1
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 17 Sep 2009 17:57:29 -0700

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> This removes a kfree_skb that is being called on a NULL pointer when
> do_one_broadcast() is sucessful.  And moves the kfree_skb into
> do_one_broadcast() for the error case.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

kfree_skb() on a NULL pointer is completely legal.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
John Fastabend Sept. 21, 2009, 12:04 p.m. UTC | #2
David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Thu, 17 Sep 2009 17:57:29 -0700
>
>   
>> From: John Fastabend <john.r.fastabend@intel.com>
>>
>> This removes a kfree_skb that is being called on a NULL pointer when
>> do_one_broadcast() is sucessful.  And moves the kfree_skb into
>> do_one_broadcast() for the error case.
>>
>> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>>     
>
> kfree_skb() on a NULL pointer is completely legal.
>   
OK, but this depends on the unlikely() macro in kfree_skb() to catch a 
case that is the expected non-error case. Would it be better to wrap the 
kfree_skb() in an if statement to avoid hitting the unlikely() macro?  
Or is the performance hit from the unlikely() macro so small this is not 
an issue?  Thanks for looking at these.

john.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Sept. 21, 2009, 8:54 p.m. UTC | #3
From: John Fastabend <john.r.fastabend@intel.com>
Date: Mon, 21 Sep 2009 12:04:35 +0000

>>   
> OK, but this depends on the unlikely() macro in kfree_skb() to catch a
> case that is the expected non-error case. Would it be better to wrap
> the kfree_skb() in an if statement to avoid hitting the unlikely()
> macro?  Or is the performance hit from the unlikely() macro so small
> this is not an issue?  Thanks for looking at these.
> 

Expands too much code inline, that's why we don't do it that
way.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 4e673d2..9934847 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1021,6 +1021,8 @@  static inline int do_one_broadcast(struct sock *sk,
 		netlink_overrun(sk);
 		if (nlk->flags & NETLINK_BROADCAST_SEND_ERROR)
 			p->delivery_failure = 1;
+		kfree_skb(p->skb2);
+		p->skb2 = NULL;
 	} else {
 		p->congested |= val;
 		p->delivered = 1;
@@ -1065,8 +1067,6 @@  int netlink_broadcast(struct sock *ssk, struct sk_buff *skb, u32 pid,
 
 	netlink_unlock_table();
 
-	kfree_skb(info.skb2);
-
 	if (info.delivery_failure)
 		return -ENOBUFS;