bridge: support sending rntl info when we set attributes through sysfs/ioctl

PATCH Superseded Xin Long 3 participants 10 messages Apr 5, 2016 – Apr 7, 2016

Subsystems (3)

Networking Network drivers Ethernet bridge

lore

Share & tools

lore

Xin Long

author

2016-04-05 03:32 UTC #0 lore raw

v2 cover bridge: support sending rntl info when we set attributes through sysfs/ioctl

This patchset is used to support sending rntl info to user in some places, and ensure that whenever those attributes change internally or from sysfs, that a netlink notification is sent out to listeners.

It also make some adjustment in bridge sysfs so that we can implement this easily.

I've done some tests on this patchset, like:
[br_sysfs]
1. change all the attribute values of br or brif:

  $ echo $value > /sys/class/net/br0/bridge/{*}
  $ echo $value > /sys/class/net/br0/brif/eth1/{*}

2. meanwhile, on another terminal to observe the msg:

  $ bridge monitor

[br_ioctl]
1. in bridge-utils package, do some changes in br_set, let brctl command use ioctl to set attribute:

         if ((ret = set_sysfs(path, value)) < 0) { -->
         if (1) {

  $ brctl set*

2. meanwhile, on another terminal to observe the msg:

  $ bridge monitor

This test covers all the attributes that brctl and sysfs support to set.

Xin Long (6):
bridge: simplify the flush_store by calling store_bridge_parm
bridge: simplify the forward_delay_store by calling store_bridge_parm
bridge: simplify the stp_state_store by calling store_bridge_parm
bridge: a netlink notification should be sent when those attributes
are changed by br_sysfs_br
bridge: a netlink notification should be sent when those attributes
are changed by br_sysfs_if
bridge: a netlink notification should be sent when those attributes
are changed by ioctl

 net/bridge/br_ioctl.c    | 40 +++++++++++++----------
 net/bridge/br_sysfs_br.c | 83 +++++++++++++++++++-----------------------------
 net/bridge/br_sysfs_if.c |  5 +--
 net/bridge/br_vlan.c     | 30 +++--------------
 4 files changed, 65 insertions(+), 93 deletions(-)

Signature · 1 line

2.1.0

Xin Long

author

2016-04-05 03:32 UTC #1 lore raw

PATCH v2 1/6 bridge: simplify the flush_store by calling store_bridge_parm

There are some repetitive codes in flush_store, we can remove them by calling store_bridge_parm, also, it would send rtnl notification after we add it in store_bridge_parm in the following patches.

Signed-off-by Xin Long <[email protected]>

---
 net/bridge/br_sysfs_br.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

net/bridge/br_sysfs_br.c +7 -7

index 6b80914..c48f6b0 100644
@@ -336,17 +336,17 @@ static ssize_t group_addr_store(struct device *d,
 
 static DEVICE_ATTR_RW(group_addr);
 
+static int set_flush(struct net_bridge *br, unsigned long val)
+{
+	br_fdb_flush(br);
+	return 0;
+}
+
 static ssize_t flush_store(struct device *d,
 			   struct device_attribute *attr,
 			   const char *buf, size_t len)
 {
-	struct net_bridge *br = to_bridge(d);
-
-	if (!ns_capable(dev_net(br->dev)->user_ns, CAP_NET_ADMIN))
-		return -EPERM;
-
-	br_fdb_flush(br);
-	return len;
+	return store_bridge_parm(d, buf, len, set_flush);
 }
 static DEVICE_ATTR_WO(flush);

Signature · 1 line

2.1.0

Xin Long

author

2016-04-05 03:32 UTC #2 lore raw

PATCH v2 2/6 bridge: simplify the forward_delay_store by calling store_bridge_parm

There are some repetitive codes in forward_delay_store, we can remove them by calling store_bridge_parm.

Signed-off-by Xin Long <[email protected]>

---
 net/bridge/br_sysfs_br.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

net/bridge/br_sysfs_br.c +10 ~~-17~~

index c48f6b0..137cd3b 100644
@@ -160,29 +160,22 @@ static ssize_t group_fwd_mask_show(struct device *d,
 	return sprintf(buf, "%#x\n", br->group_fwd_mask);
 }
 
-
-static ssize_t group_fwd_mask_store(struct device *d,
-				    struct device_attribute *attr,
-				    const char *buf,
-				    size_t len)
+static int set_group_fwd_mask(struct net_bridge *br, unsigned long val)
 {
-	struct net_bridge *br = to_bridge(d);
-	char *endp;
-	unsigned long val;
-
-	if (!ns_capable(dev_net(br->dev)->user_ns, CAP_NET_ADMIN))
-		return -EPERM;
-
-	val = simple_strtoul(buf, &endp, 0);
-	if (endp == buf)
-		return -EINVAL;
-
 	if (val & BR_GROUPFWD_RESTRICTED)
 		return -EINVAL;
 
 	br->group_fwd_mask = val;
 
-	return len;
+	return 0;
+}
+
+static ssize_t group_fwd_mask_store(struct device *d,
+				    struct device_attribute *attr,
+				    const char *buf,
+				    size_t len)
+{
+	return store_bridge_parm(d, buf, len, set_group_fwd_mask);
 }
 static DEVICE_ATTR_RW(group_fwd_mask);

Signature · 1 line

2.1.0

Xin Long

author

2016-04-05 03:32 UTC #3 lore raw

PATCH v2 3/6 bridge: simplify the stp_state_store by calling store_bridge_parm

There are some repetitive codes in stp_state_store, we can remove them by calling store_bridge_parm.

Signed-off-by Xin Long <[email protected]>

---
 net/bridge/br_sysfs_br.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

net/bridge/br_sysfs_br.c +7 ~~-17~~

index 137cd3b..9918763 100644
@@ -128,27 +128,17 @@ static ssize_t stp_state_show(struct device *d,
 }
 
 
+static int set_stp_state(struct net_bridge *br, unsigned long val)
+{
+	br_stp_set_enabled(br, val);
+	return 0;
+}
+
 static ssize_t stp_state_store(struct device *d,
 			       struct device_attribute *attr, const char *buf,
 			       size_t len)
 {
-	struct net_bridge *br = to_bridge(d);
-	char *endp;
-	unsigned long val;
-
-	if (!ns_capable(dev_net(br->dev)->user_ns, CAP_NET_ADMIN))
-		return -EPERM;
-
-	val = simple_strtoul(buf, &endp, 0);
-	if (endp == buf)
-		return -EINVAL;
-
-	if (!rtnl_trylock())
-		return restart_syscall();
-	br_stp_set_enabled(br, val);
-	rtnl_unlock();
-
-	return len;
+	return store_bridge_parm(d, buf, len, set_stp_state);
 }
 static DEVICE_ATTR_RW(stp_state);

Signature · 1 line

2.1.0

Xin Long

author

2016-04-05 03:32 UTC #4 lore raw

PATCH v2 4/6 bridge: a netlink notification should be sent when those attributes are changed by br_sysfs_br

Now when we change the attributes of bridge or br_port by netlink, a relevant netlink notification will be sent, but if we change them by ioctl or sysfs, no notification will be sent.

We should ensure that whenever those attributes change internally or from sysfs/ioctl, that a netlink notification is sent out to listeners.

Also, NetworkManager will use this in the future to listen for out-of-band bridge master attribute updates and incorporate them into the runtime configuration.

This patch is used for br_sysfs_br. and we also need to remove some rtnl_trylock in old functions so that we can call it in a common one.

For group_addr_store, we cannot make it use store_bridge_parm, because it's not a string-to-long convert, we will add notification on it individually.

Signed-off-by Xin Long <[email protected]>

---
 net/bridge/br_sysfs_br.c | 18 +++++++++---------
 net/bridge/br_vlan.c     | 30 +++++-------------------------
 2 files changed, 14 insertions(+), 34 deletions(-)

net/bridge/br_sysfs_br.c +9 -9

index 9918763..74d56dfd 100644
@@ -43,7 +43,14 @@ static ssize_t store_bridge_parm(struct device *d,
 	if (endp == buf)
 		return -EINVAL;
 
+	if (!rtnl_trylock())
+		return restart_syscall();
+
 	err = (*set)(br, val);
+	if (!err)
+		netdev_state_change(br->dev);
+	rtnl_unlock();
+
 	return err ? err : len;
 }

@@ -101,15 +108,7 @@ static ssize_t ageing_time_show(struct device *d,
 
 static int set_ageing_time(struct net_bridge *br, unsigned long val)
 {
-	int ret;
-
-	if (!rtnl_trylock())
-		return restart_syscall();
-
-	ret = br_set_ageing_time(br, val);
-	rtnl_unlock();
-
-	return ret;
+	return br_set_ageing_time(br, val);
 }
 
 static ssize_t ageing_time_store(struct device *d,

@@ -311,6 +310,7 @@ static ssize_t group_addr_store(struct device *d,
 
 	br->group_addr_set = true;
 	br_recalculate_fwd_mask(br);
+	netdev_state_change(br->dev);
 
 	rtnl_unlock();

net/bridge/br_vlan.c +5 ~~-25~~

index 9309bb4..e001152 100644
@@ -651,15 +651,7 @@ int __br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
 
 int br_vlan_filter_toggle(struct net_bridge *br, unsigned long val)
 {
-	int err;
-
-	if (!rtnl_trylock())
-		return restart_syscall();
-
-	err = __br_vlan_filter_toggle(br, val);
-	rtnl_unlock();
-
-	return err;
+	return __br_vlan_filter_toggle(br, val);
 }
 
 int __br_vlan_set_proto(struct net_bridge *br, __be16 proto)

@@ -713,18 +705,10 @@ err_filt:
 
 int br_vlan_set_proto(struct net_bridge *br, unsigned long val)
 {
-	int err;
-
 	if (val != ETH_P_8021Q && val != ETH_P_8021AD)
 		return -EPROTONOSUPPORT;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
-
-	err = __br_vlan_set_proto(br, htons(val));
-	rtnl_unlock();
-
-	return err;
+	return __br_vlan_set_proto(br, htons(val));
 }
 
 static bool vlan_default_pvid(struct net_bridge_vlan_group *vg, u16 vid)

@@ -855,21 +839,17 @@ int br_vlan_set_default_pvid(struct net_bridge *br, unsigned long val)
 	if (val >= VLAN_VID_MASK)
 		return -EINVAL;
 
-	if (!rtnl_trylock())
-		return restart_syscall();
-
 	if (pvid == br->default_pvid)
-		goto unlock;
+		goto out;
 
 	/* Only allow default pvid change when filtering is disabled */
 	if (br->vlan_enabled) {
 		pr_info_once("Please disable vlan filtering to change default_pvid\n");
 		err = -EPERM;
-		goto unlock;
+		goto out;
 	}
 	err = __br_vlan_set_default_pvid(br, pvid);
-unlock:
-	rtnl_unlock();
+out:
 	return err;
 }

Signature · 1 line

2.1.0

Xin Long

author

2016-04-05 03:32 UTC #5 lore raw

PATCH v2 5/6 bridge: a netlink notification should be sent when those attributes are changed by br_sysfs_if

Now when we change the attributes of bridge or br_port by netlink, a relevant netlink notification will be sent, but if we change them by ioctl or sysfs, no notification will be sent.

We should ensure that whenever those attributes change internally or from sysfs/ioctl, that a netlink notification is sent out to listeners.

Also, NetworkManager will use this in the future to listen for out-of-band bridge master attribute updates and incorporate them into the runtime configuration.

This patch is used for br_sysfs_if, and we also move br_ifinfo_notify out of store_flag.

Signed-off-by Xin Long <[email protected]>

---
 net/bridge/br_sysfs_if.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

net/bridge/br_sysfs_if.c +3 -2

index efe415a..1e04d4d 100644
@@ -61,7 +61,6 @@ static int store_flag(struct net_bridge_port *p, unsigned long v,
 	if (flags != p->flags) {
 		p->flags = flags;
 		br_port_flags_change(p, mask);
-		br_ifinfo_notify(RTM_NEWLINK, p);
 	}
 	return 0;
 }

@@ -253,8 +252,10 @@ static ssize_t brport_store(struct kobject *kobj,
 			spin_lock_bh(&p->br->lock);
 			ret = brport_attr->store(p, val);
 			spin_unlock_bh(&p->br->lock);
-			if (ret == 0)
+			if (!ret) {
+				br_ifinfo_notify(RTM_NEWLINK, p);
 				ret = count;
+			}
 		}
 		rtnl_unlock();
 	}

Signature · 1 line

2.1.0

Xin Long

author

2016-04-05 03:32 UTC #6 lore raw

PATCH v2 6/6 bridge: a netlink notification should be sent when those attributes are changed by ioctl

Now when we change the attributes of bridge or br_port by netlink, a relevant netlink notification will be sent, but if we change them by ioctl or sysfs, no notification will be sent.

We should ensure that whenever those attributes change internally or from sysfs/ioctl, that a netlink notification is sent out to listeners.

Also, NetworkManager will use this in the future to listen for out-of-band bridge master attribute updates and incorporate them into the runtime configuration.

This patch is used for ioctl.

Signed-off-by Xin Long <[email protected]>

---
 net/bridge/br_ioctl.c | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

net/bridge/br_ioctl.c +24 ~~-16~~

index 263b4de..f8fc624 100644
@@ -112,7 +112,9 @@ static int add_del_if(struct net_bridge *br, int ifindex, int isadd)
 static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
 	struct net_bridge *br = netdev_priv(dev);
+	struct net_bridge_port *p = NULL;
 	unsigned long args[4];
+	int ret = -EOPNOTSUPP;
 
 	if (copy_from_user(args, rq->ifr_data, sizeof(args)))
 		return -EFAULT;

@@ -182,25 +184,29 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 
-		return br_set_forward_delay(br, args[1]);
+		ret = br_set_forward_delay(br, args[1]);
+		break;
 
 	case BRCTL_SET_BRIDGE_HELLO_TIME:
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 
-		return br_set_hello_time(br, args[1]);
+		ret = br_set_hello_time(br, args[1]);
+		break;
 
 	case BRCTL_SET_BRIDGE_MAX_AGE:
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 
-		return br_set_max_age(br, args[1]);
+		ret = br_set_max_age(br, args[1]);
+		break;
 
 	case BRCTL_SET_AGEING_TIME:
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 
-		return br_set_ageing_time(br, args[1]);
+		ret = br_set_ageing_time(br, args[1]);
+		break;
 
 	case BRCTL_GET_PORT_INFO:
 	{

@@ -240,20 +246,19 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 			return -EPERM;
 
 		br_stp_set_enabled(br, args[1]);
-		return 0;
+		ret = 0;
+		break;
 
 	case BRCTL_SET_BRIDGE_PRIORITY:
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;
 
 		br_stp_set_bridge_priority(br, args[1]);
-		return 0;
+		ret = 0;
+		break;
 
 	case BRCTL_SET_PORT_PRIORITY:
 	{
-		struct net_bridge_port *p;
-		int ret;
-
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;

@@ -263,14 +268,11 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		else
 			ret = br_stp_set_port_priority(p, args[2]);
 		spin_unlock_bh(&br->lock);
-		return ret;
+		break;
 	}
 
 	case BRCTL_SET_PATH_COST:
 	{
-		struct net_bridge_port *p;
-		int ret;
-
 		if (!ns_capable(dev_net(dev)->user_ns, CAP_NET_ADMIN))
 			return -EPERM;

@@ -280,8 +282,7 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 		else
 			ret = br_stp_set_path_cost(p, args[2]);
 		spin_unlock_bh(&br->lock);
-
-		return ret;
+		break;
 	}
 
 	case BRCTL_GET_FDB_ENTRIES:

@@ -289,7 +290,14 @@ static int old_dev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 				       args[2], args[3]);
 	}
 
-	return -EOPNOTSUPP;
+	if (!ret) {
+		if (p)
+			br_ifinfo_notify(RTM_NEWLINK, p);
+		else
+			netdev_state_change(br->dev);
+	}
+
+	return ret;
 }
 
 static int old_deviceless(struct net *net, void __user *uarg)

Signature · 1 line

2.1.0

Toshiaki Makita

2016-04-05 05:08 UTC #7 lore raw

Re: [PATCHv2 net-next 3/6] bridge: simplify the stp_state_store by calling store_bridge_parm

In reply to Xin Long

Quoted 13 lines · Xin Long

There are some repetitive codes in stp_state_store, we can remove them by calling store_bridge_parm.
Signed-off-by Xin Long <[email protected]>
---
 net/bridge/br_sysfs_br.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)
net/bridge/br_sysfs_br.c +0 -0
index 137cd3b..9918763 100644

net/bridge/br_sysfs_br.c static ssize_t stp_state_show(struct device *d, · line 132

Quoted 5 lines

net/bridge/br_sysfs_br.c +2 -0

@@ -128,27 +128,17 @@ static ssize_t stp_state_show(struct device *d,
 }
 
 
+static int set_stp_state(struct net_bridge *br, unsigned long val)
+{

You forgot to add rtnl lock here?
The missing lock is restored in patch 4, but at this point bisect could break..

net/bridge/br_sysfs_br.c static ssize_t stp_state_show(struct device *d, · line 135

Quoted 3 lines

net/bridge/br_sysfs_br.c +3 -0
+	br_stp_set_enabled(br, val);
+	return 0;
+}

Toshiaki Makita

David Miller

maintainer

2016-04-06 20:10 UTC #8 lore raw

Re: [PATCHv2 net-next 3/6] bridge: simplify the stp_state_store by calling store_bridge_parm

From: Toshiaki Makita <[email protected]>
Date: Tue, 5 Apr 2016 14:08:13 +0900

Quoted 17 lines · Xin Long

Quoted 16 lines
There are some repetitive codes in stp_state_store, we can remove them by calling store_bridge_parm.
Signed-off-by Xin Long <[email protected]>
---
 net/bridge/br_sysfs_br.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)
net/bridge/br_sysfs_br.c +0 -0
index 137cd3b..9918763 100644
@@ -128,27 +128,17 @@ static ssize_t stp_state_show(struct device *d,
 }
 
 

Quoted 6 lines

net/bridge/br_sysfs_br.c static ssize_t stp_state_show(struct device *d, · line 132
Quoted 2 lines
net/bridge/br_sysfs_br.c +2 -0
+static int set_stp_state(struct net_bridge *br, unsigned long val)
+{
You forgot to add rtnl lock here?
The missing lock is restored in patch 4, but at this point bisect could break..

Agreed, this has to be fixed.

Xin Long

author

2016-04-07 03:49 UTC #9 lore raw

Re: [PATCHv2 net-next 3/6] bridge: simplify the stp_state_store by calling store_bridge_parm

Quoted 26 lines · David Miller

From: Toshiaki Makita <[email protected]>
Date: Tue, 5 Apr 2016 14:08:13 +0900
Quoted 23 lines · Xin Long
Quoted 13 lines
There are some repetitive codes in stp_state_store, we can remove them by calling store_bridge_parm.
Signed-off-by Xin Long <[email protected]>
---
 net/bridge/br_sysfs_br.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)
net/bridge/br_sysfs_br.c +0 -0
index 137cd3b..9918763 100644
net/bridge/br_sysfs_br.c static ssize_t stp_state_show(struct device *d, · line 132
Quoted 5 lines
net/bridge/br_sysfs_br.c +2 -0
@@ -128,27 +128,17 @@ static ssize_t stp_state_show(struct device *d,
 }
 
 
+static int set_stp_state(struct net_bridge *br, unsigned long val)
+{
You forgot to add rtnl lock here?
The missing lock is restored in patch 4, but at this point bisect could break..

OK, if I will fix this, if there's no other issues on this patchset, I will post v3 later.
Thanks.

Quoted 2 lines

Agreed, this has to be fixed.