[PATCH 0/2] rtnetlink locking and notification fixes

[PATCH 0/2] rtnetlink locking and notification fixes

[Laszlo Attila Toth]

Hi Dave,

These two patches remove unnecessary locks from rtnetlink, it was managed in
an inconsistent way, and change notification. The latter is always sent
if anything is changed but for compatibility the old nofification is also kept.

Regards,
    Attila

[PATCH 1/2] Remove unnecessary locks from rtnetlink

[Laszlo Attila Toth]

The do_setlink() function is protected by rtnl, additional locks are unnecessary.
and the set_operstate() function is called from protected parts. Locks removed
from both functions.

The set_operstate() is also called from rtnl_create_link() and from no other places.
In rtnl_create_link() none of the changes is protected by set_lock_bh() except
inside set_operstate(), different locking scheme is not necessary
for the operstate.

Signed-off-by: Laszlo Attila Toth <panther@balabit.hu>
---
 net/core/rtnetlink.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index ddbdde8..724e8f5 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -565,9 +565,7 @@ static void set_operstate(struct net_device *dev, unsigned char transition)
 	}
 
 	if (dev->operstate != operstate) {
-		write_lock_bh(&dev_base_lock);
 		dev->operstate = operstate;
-		write_unlock_bh(&dev_base_lock);
 		netdev_state_change(dev);
 	}
 }
@@ -882,9 +880,7 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
 
 	if (tb[IFLA_LINKMODE]) {
-		write_lock_bh(&dev_base_lock);
 		dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
-		write_unlock_bh(&dev_base_lock);
 	}
 
 	err = 0;
-- 
1.5.2.5

Re: [PATCH 1/2] Remove unnecessary locks from rtnetlink

[David Miller]

From: Laszlo Attila Toth <panther@balabit.hu>
Date: Fri,  1 Feb 2008 17:07:33 +0100

> The do_setlink() function is protected by rtnl, additional locks are unnecessary.
> and the set_operstate() function is called from protected parts. Locks removed
> from both functions.
> 
> The set_operstate() is also called from rtnl_create_link() and from no other places.
> In rtnl_create_link() none of the changes is protected by set_lock_bh() except
> inside set_operstate(), different locking scheme is not necessary
> for the operstate.
> 
> Signed-off-by: Laszlo Attila Toth <panther@balabit.hu>

The protection using dev_base_lock() is needed.

When analyzing cases like this you need to also look at other code
paths outside of rtnetlink that access ->operstate and ->link_mode,
you obviously didn't do this.

For example, net/core/net-sysfs.c takes a read lock on dev_base_lock
in order to fetch a stable copy of both netif_running() and
dev->operstate at the same time.

Similar write locking to protect dev->operstate is made by
net/core/link_watch.c:rfc2863_policy(), for the same reason rtnetlink
has to make this locking.

You therefore cannot remove it.

This invalidates your second patch so I'm dropping that as well.

[PATCH] rtnetlink.c: send a single notification on device state changes

[Laszlo Attila Toth]

In do_setlink() a single notification is sent at the end of the function
if any modification occured. If the address has been changed, another
notification is sent.

Both of them is required because originally only the NETDEV_CHANGEADDR
notification was sent and although device state change implies address
change, some programs may expect the original notification. It remains
for compatibity.

If set_operstate() is called from do_setlink(), it doesn't send
a notification, only if it is called from rtnl_create_link() as earlier.

Signed-off-by: Laszlo Attila Toth <panther@balabit.hu>
---
 net/core/rtnetlink.c |   36 ++++++++++++++++++++++++++----------
 1 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 61ac8d0..ecb02af 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -504,7 +504,7 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 
 EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo);
 
-static void set_operstate(struct net_device *dev, unsigned char transition)
+static int set_operstate(struct net_device *dev, unsigned char transition, bool send_notification)
 {
 	unsigned char operstate = dev->operstate;
 
@@ -527,8 +527,12 @@ static void set_operstate(struct net_device *dev, unsigned char transition)
 		write_lock_bh(&dev_base_lock);
 		dev->operstate = operstate;
 		write_unlock_bh(&dev_base_lock);
-		netdev_state_change(dev);
-	}
+
+		if (send_notification)
+			netdev_state_change(dev);
+		return 1;
+	} else
+		return 0;
 }
 
 static void copy_rtnl_link_stats(struct rtnl_link_stats *a,
@@ -822,6 +826,7 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 	if (tb[IFLA_BROADCAST]) {
 		nla_memcpy(dev->broadcast, tb[IFLA_BROADCAST], dev->addr_len);
 		send_addr_notify = 1;
+		modified = 1;
 	}
 
 	if (ifm->ifi_flags || ifm->ifi_change) {
@@ -834,16 +839,23 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		dev_change_flags(dev, flags);
 	}
 
-	if (tb[IFLA_TXQLEN])
-		dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
+	if (tb[IFLA_TXQLEN]) {
+		if (dev->tx_queue_len != nla_get_u32(tb[IFLA_TXQLEN])) {
+			dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
+			modified = 1;
+		}
+	}
 
 	if (tb[IFLA_OPERSTATE])
-		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
+		modified |= set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]), false);
 
 	if (tb[IFLA_LINKMODE]) {
-		write_lock_bh(&dev_base_lock);
-		dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
-		write_unlock_bh(&dev_base_lock);
+		if (dev->link_mode != nla_get_u8(tb[IFLA_LINKMODE])) {
+			write_lock_bh(&dev_base_lock);
+			dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
+			write_lock_bh(&dev_base_lock);
+			modified = 1;
+		}
 	}
 
 	err = 0;
@@ -857,6 +869,10 @@ errout:
 
 	if (send_addr_notify)
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
+
+	if (modified)
+		netdev_state_change(dev);
+
 	return err;
 }
 
@@ -974,7 +990,7 @@ struct net_device *rtnl_create_link(struct net *net, char *ifname,
 	if (tb[IFLA_TXQLEN])
 		dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
 	if (tb[IFLA_OPERSTATE])
-		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
+		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]), true);
 	if (tb[IFLA_LINKMODE])
 		dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
 
-- 
1.5.2.5

Re: [PATCH] rtnetlink.c: send a single notification on device state changes

[David Miller]

From: Laszlo Attila Toth <panther@balabit.hu>
Date: Thu,  7 Feb 2008 12:57:50 +0100

> In do_setlink() a single notification is sent at the end of the function
> if any modification occured. If the address has been changed, another
> notification is sent.
> 
> Both of them is required because originally only the NETDEV_CHANGEADDR
> notification was sent and although device state change implies address
> change, some programs may expect the original notification. It remains
> for compatibity.
> 
> If set_operstate() is called from do_setlink(), it doesn't send
> a notification, only if it is called from rtnl_create_link() as earlier.
> 
> Signed-off-by: Laszlo Attila Toth <panther@balabit.hu>

Looks good, applied.

Let's hope this doesn't break some stupid application
unintentionally.

Re: [PATCH 1/2] Remove unnecessary locks from rtnetlink

[Laszlo Attila Toth]

David Miller írta:
> From: Laszlo Attila Toth <panther@balabit.hu>
> Date: Fri,  1 Feb 2008 17:07:33 +0100
> 
>> The do_setlink() function is protected by rtnl, additional locks are unnecessary.
>> and the set_operstate() function is called from protected parts. Locks removed
>> from both functions.
>>
>> The set_operstate() is also called from rtnl_create_link() and from no other places.
>> In rtnl_create_link() none of the changes is protected by set_lock_bh() except
>> inside set_operstate(), different locking scheme is not necessary
>> for the operstate.
>>
>> Signed-off-by: Laszlo Attila Toth <panther@balabit.hu>
> 
> The protection using dev_base_lock() is needed.
> 
> When analyzing cases like this you need to also look at other code
> paths outside of rtnetlink that access ->operstate and ->link_mode,
> you obviously didn't do this.
> 
> For example, net/core/net-sysfs.c takes a read lock on dev_base_lock
> in order to fetch a stable copy of both netif_running() and
> dev->operstate at the same time.
> 
> Similar write locking to protect dev->operstate is made by
> net/core/link_watch.c:rfc2863_policy(), for the same reason rtnetlink
> has to make this locking.
> 
> You therefore cannot remove it.

Thanks for your answer, yes, unfortunatelly I checked only inside 
rtnetlink.c

--
Attila

[PATCH 2/2] rtnetlink: send a single notification on device state changes

[Laszlo Attila Toth]

In do_setlink() a single notification is sent at the end of the function
if any modification occured. If the address has been changed, another
notification is sent.

Both of them is required because originally only the NETDEV_CHANGEADDR
notification was sent and although device state change implies address
change, some programs may expect the original notification. It remains
for compatibity.

Signed-off-by: Laszlo Attila Toth <panther@balabit.hu>
---
 net/core/rtnetlink.c |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 724e8f5..d67b950 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -545,7 +545,7 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
 
 EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo);
 
-static void set_operstate(struct net_device *dev, unsigned char transition)
+static int set_operstate(struct net_device *dev, unsigned char transition)
 {
 	unsigned char operstate = dev->operstate;
 
@@ -566,8 +566,9 @@ static void set_operstate(struct net_device *dev, unsigned char transition)
 
 	if (dev->operstate != operstate) {
 		dev->operstate = operstate;
-		netdev_state_change(dev);
-	}
+		return 1;
+	} else
+		return 0;
 }
 
 static void copy_rtnl_link_stats(struct rtnl_link_stats *a,
@@ -861,6 +862,7 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 	if (tb[IFLA_BROADCAST]) {
 		nla_memcpy(dev->broadcast, tb[IFLA_BROADCAST], dev->addr_len);
 		send_addr_notify = 1;
+		modified = 1;
 	}
 
 	if (ifm->ifi_flags || ifm->ifi_change) {
@@ -873,14 +875,21 @@ static int do_setlink(struct net_device *dev, struct ifinfomsg *ifm,
 		dev_change_flags(dev, flags);
 	}
 
-	if (tb[IFLA_TXQLEN])
-		dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
+	if (tb[IFLA_TXQLEN]) {
+		if (dev->tx_queue_len != nla_get_u32(tb[IFLA_TXQLEN])) {
+			dev->tx_queue_len = nla_get_u32(tb[IFLA_TXQLEN]);
+			modified = 1;
+		}
+	}
 
 	if (tb[IFLA_OPERSTATE])
-		set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
+		modified |= set_operstate(dev, nla_get_u8(tb[IFLA_OPERSTATE]));
 
 	if (tb[IFLA_LINKMODE]) {
-		dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
+		if (dev->link_mode != nla_get_u8(tb[IFLA_LINKMODE])) {
+			dev->link_mode = nla_get_u8(tb[IFLA_LINKMODE]);
+			modified = 1;
+		}
 	}
 
 	err = 0;
@@ -894,6 +903,10 @@ errout:
 
 	if (send_addr_notify)
 		call_netdevice_notifiers(NETDEV_CHANGEADDR, dev);
+
+	if (modified)
+		netdev_state_change(dev);
+
 	return err;
 }
 
-- 
1.5.2.5