[net-2.6 PATCH 1/6] net: initialize rmem_alloc and omem_alloc to 0 in netlink socket

[net-2.6 PATCH 1/6] net: initialize rmem_alloc and omem_alloc to 0 in netlink socket

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

The rmem_alloc and omem_alloc socket fields are not
initialized.  This sets each variable to zero when a socket
is created.  Note the sk_wmem_alloc is already initialized
in sock_init_data.

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 |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index c5aab6a..4e673d2 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -423,6 +423,9 @@ static int __netlink_create(struct net *net, struct socket *sock,
 	}
 	init_waitqueue_head(&nlk->wait);
 
+	atomic_set(&sk->sk_rmem_alloc, 0);
+	atomic_set(&sk->sk_omem_alloc, 0);
+
 	sk->sk_destruct = netlink_sock_destruct;
 	sk->sk_protocol = protocol;
 	return 0;

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

[Jeff Kirsher]

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(-)

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;
 

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

[David Miller]

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.

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

[John Fastabend]

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.

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

[David Miller]

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.

[net-2.6 PATCH 3/6] net: fix vlan_get_size to include vlan_flags size

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

Fix vlan_get_size to include vlan->flags.  Currently, the
size of the vlan flags is not included in the nlmsg size.

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

 net/8021q/vlan_netlink.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/8021q/vlan_netlink.c b/net/8021q/vlan_netlink.c
index 343146e..a915048 100644
--- a/net/8021q/vlan_netlink.c
+++ b/net/8021q/vlan_netlink.c
@@ -169,6 +169,7 @@ static size_t vlan_get_size(const struct net_device *dev)
 	struct vlan_dev_info *vlan = vlan_dev_info(dev);
 
 	return nla_total_size(2) +	/* IFLA_VLAN_ID */
+	       sizeof(struct ifla_vlan_flags) + /* IFLA_VLAN_FLAGS */
 	       vlan_qos_map_size(vlan->nr_ingress_mappings) +
 	       vlan_qos_map_size(vlan->nr_egress_mappings);
 }

[net-2.6 PATCH 4/6] net: fix nlmsg len size for skb when error bit is set.

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

Currently, the nlmsg->len field is not set correctly in  netlink_ack()
for ack messages that include the nlmsg of the error frame.  This
corrects the length field passed to __nlmsg_put to use the correct
payload size.

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 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 9934847..aa74011 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1788,7 +1788,7 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 	}
 
 	rep = __nlmsg_put(skb, NETLINK_CB(in_skb).pid, nlh->nlmsg_seq,
-			  NLMSG_ERROR, sizeof(struct nlmsgerr), 0);
+			  NLMSG_ERROR, payload, 0);
 	errmsg = nlmsg_data(rep);
 	errmsg->error = err;
 	memcpy(&errmsg->msg, nlh, err ? nlh->nlmsg_len : sizeof(*nlh));

[net-2.6 PATCH 5/6] net: fix sock locking for sk_err field in netlink.

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

This adds the sock lock around setting the sk_err field
in sock struct. Without the lock multiple threads may
write to this field.

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 |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index aa74011..1669dfc 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -732,7 +732,9 @@ static void netlink_overrun(struct sock *sk)
 
 	if (!(nlk->flags & NETLINK_RECV_NO_ENOBUFS)) {
 		if (!test_and_set_bit(0, &nlk_sk(sk)->state)) {
+			lock_sock(sk);
 			sk->sk_err = ENOBUFS;
+			release_sock(sk);
 			sk->sk_error_report(sk);
 		}
 	}
@@ -1101,7 +1103,9 @@ static inline int do_one_set_err(struct sock *sk,
 	    !test_bit(p->group - 1, nlk->groups))
 		goto out;
 
+	lock_sock(sk);
 	sk->sk_err = p->code;
+	release_sock(sk);
 	sk->sk_error_report(sk);
 out:
 	return 0;
@@ -1780,7 +1784,9 @@ void netlink_ack(struct sk_buff *in_skb, struct nlmsghdr *nlh, int err)
 				    in_skb->sk->sk_protocol,
 				    NETLINK_CB(in_skb).pid);
 		if (sk) {
+			lock_sock(sk);
 			sk->sk_err = ENOBUFS;
+			release_sock(sk);
 			sk->sk_error_report(sk);
 			sock_put(sk);
 		}

Re: [net-2.6 PATCH 5/6] net: fix sock locking for sk_err field in netlink.

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 17 Sep 2009 17:58:32 -0700

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> This adds the sock lock around setting the sk_err field
> in sock struct. Without the lock multiple threads may
> write to this field.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

This isn't right.

Writes to sk->sk_err can occur asynchronously just fine and
without any locking.

The only requirement is that consumers of the sk_err value
use sock_error() which uses xchg() to get and clear the
value atomically.

[net-2.6 PATCH 6/6] net: fix double skb free in dcbnl

[Jeff Kirsher]

From: John Fastabend <john.r.fastabend@intel.com>

netlink_unicast() calls kfree_skb even in the error case.

dcbnl calls netlink_unicast() which when it fails free's the
skb and returns an error value.  dcbnl is free'ing the skb
again when this error occurs.  This patch removes the double
free.

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

 net/dcb/dcbnl.c |   15 +++++++--------
 1 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index e0879bf..ac1205d 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -194,7 +194,7 @@ static int dcbnl_reply(u8 value, u8 event, u8 cmd, u8 attr, u32 pid,
 	nlmsg_end(dcbnl_skb, nlh);
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		return -EINVAL;
 
 	return 0;
 nlmsg_failure:
@@ -275,7 +275,7 @@ static int dcbnl_getpfccfg(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 nlmsg_failure:
@@ -316,12 +316,11 @@ static int dcbnl_getperm_hwaddr(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 
 nlmsg_failure:
-err:
 	kfree_skb(dcbnl_skb);
 err_out:
 	return -EINVAL;
@@ -383,7 +382,7 @@ static int dcbnl_getcap(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 nlmsg_failure:
@@ -460,7 +459,7 @@ static int dcbnl_getnumtcs(struct net_device *netdev, struct nlattr **tb,
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret) {
 		ret = -EINVAL;
-		goto err;
+		goto err_out;
 	}
 
 	return 0;
@@ -799,7 +798,7 @@ static int __dcbnl_pg_getcfg(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 
@@ -1063,7 +1062,7 @@ static int dcbnl_bcn_getcfg(struct net_device *netdev, struct nlattr **tb,
 
 	ret = rtnl_unicast(dcbnl_skb, &init_net, pid);
 	if (ret)
-		goto err;
+		goto err_out;
 
 	return 0;
 

Re: [net-2.6 PATCH 1/6] net: initialize rmem_alloc and omem_alloc to 0 in netlink socket

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Thu, 17 Sep 2009 17:57:09 -0700

> From: John Fastabend <john.r.fastabend@intel.com>
> 
> The rmem_alloc and omem_alloc socket fields are not
> initialized.  This sets each variable to zero when a socket
> is created.  Note the sk_wmem_alloc is already initialized
> in sock_init_data.
> 
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

It's set to zero implicitly by the memset() done at sock_alloc()
time.

Re-setting it again here explicitly will just add unnecessary
memory traffic.