Skip to contentSkip to conversation

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

PATCH In review Xin Long 3 participants 15 messages Apr 8, 2016 – Apr 14, 2016

Subsystems (3)
Networking Network drivers Ethernet bridge
lore
Share & tools
lore

Xin Long

author
#0 lore raw

v3 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 | 84 ++++++++++++++++++++----------------------------
 net/bridge/br_sysfs_if.c |  5 +--
 net/bridge/br_vlan.c     | 30 +++--------------
 4 files changed, 66 insertions(+), 93 deletions(-)
Signature · 1 line
2.1.0
author
#1 lore raw

PATCH v3 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
author
#2 lore raw

PATCH v3 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
author
#3 lore raw

PATCH v3 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, 9 insertions(+), 15 deletions(-)
net/bridge/br_sysfs_br.c +9 -15
index 137cd3b..f9d484e 100644
@@ -128,27 +128,21 @@ static ssize_t stp_state_show(struct device *d,
 }
 
 
-static ssize_t stp_state_store(struct device *d,
-			       struct device_attribute *attr, const char *buf,
-			       size_t len)
+static int set_stp_state(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 (!rtnl_trylock())
 		return restart_syscall();
 	br_stp_set_enabled(br, val);
 	rtnl_unlock();
 
-	return len;
+	return 0;
+}
+
+static ssize_t stp_state_store(struct device *d,
+			       struct device_attribute *attr, const char *buf,
+			       size_t len)
+{
+	return store_bridge_parm(d, buf, len, set_stp_state);
 }
 static DEVICE_ATTR_RW(stp_state);
Signature · 1 line
2.1.0
author
#4 lore raw

PATCH v3 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 | 21 +++++++++------------
 net/bridge/br_vlan.c     | 30 +++++-------------------------
 2 files changed, 14 insertions(+), 37 deletions(-)
net/bridge/br_sysfs_br.c +9 -12
index f9d484e..70bddfd 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,

@@ -130,10 +129,7 @@ static ssize_t stp_state_show(struct device *d,
 
 static int set_stp_state(struct net_bridge *br, unsigned long val)
 {
-	if (!rtnl_trylock())
-		return restart_syscall();
 	br_stp_set_enabled(br, val);
-	rtnl_unlock();
 
 	return 0;
 }

@@ -315,6 +311,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
author
#5 lore raw

PATCH v3 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
author
#6 lore raw

PATCH v3 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
Reviewed-by Nikolay Aleksandrov on v3 6/6
#7 lore raw

Re: [PATCHv3 net-next 6/6] bridge: a netlink notification should be sent when those attributes are changed by ioctl

Quoted 11 lines · Xin Long

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.

Quoted 7 lines

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

LGTM,

Reviewed-by Nikolay Aleksandrov <[email protected]>
Reviewed-by Nikolay Aleksandrov on v3 5/6
#8 lore raw

Re: [PATCHv3 net-next 5/6] bridge: a netlink notification should be sent when those attributes are changed by br_sysfs_if

In reply to Xin Long
Quoted 11 lines · Xin Long

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.

Quoted 8 lines

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

Note: there's a slight behaviour change, before if the flags were the same a notification wouldn't be sent, now it would. Anyway I don't see a problem as this is true for other attributes which are set to the same value.

Reviewed-by Nikolay Aleksandrov <[email protected]>
#9 lore raw

Re: [PATCHv3 net-next 4/6] bridge: a netlink notification should be sent when those attributes are changed by br_sysfs_br

In reply to Xin Long
Quoted 18 lines · Xin Long

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.

Quoted 6 lines
Signed-off-by Xin Long <[email protected]>
---
 net/bridge/br_sysfs_br.c | 21 +++++++++------------
 net/bridge/br_vlan.c     | 30 +++++-------------------------
 2 files changed, 14 insertions(+), 37 deletions(-)
Signed-off-by Nikolay Aleksandrov <[email protected]>
Reviewed-by Nikolay Aleksandrov on v3 3/6
#10 lore raw

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

In reply to Xin Long
Quoted 8 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, 9 insertions(+), 15 deletions(-)
Reviewed-by Nikolay Aleksandrov <[email protected]>
Reviewed-by Nikolay Aleksandrov on v3 2/6
#11 lore raw

Re: [PATCHv3 net-next 2/6] bridge: simplify the forward_delay_store by calling store_bridge_parm

In reply to Xin Long
Quoted 8 lines · Xin Long

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(-)
Reviewed-by Nikolay Aleksandrov <[email protected]>
Reviewed-by Nikolay Aleksandrov on v3 1/6
#12 lore raw

Re: [PATCHv3 net-next 1/6] bridge: simplify the flush_store by calling store_bridge_parm

In reply to Xin Long
Quoted 9 lines · Xin Long

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(-)
Reviewed-by Nikolay Aleksandrov <[email protected]>
#13 lore raw
In reply to Xin Long
Quoted 22 lines · Xin Long

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) {
Quoted 7 lines
  $ 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.

Overall the set looks good to me, just one comment for future posts - please include the changes between versions of the set in your cover letter and individual patches. I had to go back to your previous postings and read my own comments and compare them with this set.

Thank you,
Nik

maintainer
#14 lore raw
In reply to Xin Long

From: Xin Long <[email protected]>
Date: Sat, 9 Apr 2016 00:03:27 +0800

Quoted 8 lines

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:

...

Quoted 1 line

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

Series applied, thanks.