Message ID | 20090918005729.25594.14261.stgit@localhost.localdomain |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
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
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
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 --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;