[PATCH net-next 0/6] bridge: support sending rntl info when we set attributes through sysfs/ioctl

[PATCH net-next 0/6] bridge: support sending rntl info when we set attributes through sysfs/ioctl

[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) {

  $ 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: add rtnl_lock in fdb_flush in br_sysfs_br.c
  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 | 73 ++++++++++++++++++++----------------------------
 net/bridge/br_sysfs_if.c |  5 ++--
 net/bridge/br_vlan.c     | 30 ++++----------------
 4 files changed, 62 insertions(+), 86 deletions(-)

-- 
2.1.0

[PATCH net-next 1/6] bridge: add rtnl_lock in fdb_flush in br_sysfs_br.c

[Xin Long]

In fdb_delete, it will send rtnl msg, so before that, we should
hold rtnl_lock in the function that call it in sysfs.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/bridge/br_sysfs_br.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 6b80914..095a751 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -345,7 +345,12 @@ static ssize_t flush_store(struct device *d,
 	if (!ns_capable(dev_net(br->dev)->user_ns, CAP_NET_ADMIN))
 		return -EPERM;
 
+	if (!rtnl_trylock())
+		return restart_syscall();
+
 	br_fdb_flush(br);
+	rtnl_unlock();
+
 	return len;
 }
 static DEVICE_ATTR_WO(flush);
-- 
2.1.0

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

[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 <lucien.xin@gmail.com>
---
 net/bridge/br_sysfs_br.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 095a751..c846bf9 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -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);
 
-- 
2.1.0

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

[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 <lucien.xin@gmail.com>
---
 net/bridge/br_sysfs_br.c | 24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index c846bf9..5d9fee2 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -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);
 
-- 
2.1.0

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

[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.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/bridge/br_sysfs_br.c | 17 ++++++++---------
 net/bridge/br_vlan.c     | 30 +++++-------------------------
 2 files changed, 13 insertions(+), 34 deletions(-)

diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 5d9fee2..96f04b3 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -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,
diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
index 9309bb4..e001152 100644
--- a/net/bridge/br_vlan.c
+++ b/net/bridge/br_vlan.c
@@ -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;
 }
 
-- 
2.1.0

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

[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_if, and we also move br_ifinfo_notify out
of store_flag.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/bridge/br_sysfs_if.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index efe415a..1e04d4d 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -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();
 	}
-- 
2.1.0

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

[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 ioctl.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/bridge/br_ioctl.c | 40 ++++++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index 263b4de..f8fc624 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -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)
-- 
2.1.0

Re: [PATCH net-next 1/6] bridge: add rtnl_lock in fdb_flush in br_sysfs_br.c

[Nikolay Aleksandrov]

On 03/16/2016 02:34 PM, Xin Long wrote:
> In fdb_delete, it will send rtnl msg, so before that, we should
> hold rtnl_lock in the function that call it in sysfs.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/bridge/br_sysfs_br.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 

IIRC rtnl_notify() doesn't require rtnl lock to be held so this patch is not
needed

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

[Nikolay Aleksandrov]

On 03/16/2016 02:34 PM, Xin Long wrote:
> There are some repetitive codes in forward_delay_store, we can remove
> them by calling store_bridge_parm.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/bridge/br_sysfs_br.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 

I actually have a similar patch in my tree. :-)

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

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

[Nikolay Aleksandrov]

On 03/16/2016 02:34 PM, Xin Long wrote:
> There are some repetitive codes in stp_state_store, we can remove
> them by calling store_bridge_parm.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/bridge/br_sysfs_br.c | 24 +++++++-----------------
>  1 file changed, 7 insertions(+), 17 deletions(-)
> 

LGTM. Note: it introduces a bug (missing rtnl) until patch 04 is applied.

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

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

[Nikolay Aleksandrov]

On 03/16/2016 02:34 PM, Xin Long wrote:
> 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.
> 
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
>  net/bridge/br_sysfs_br.c | 17 ++++++++---------
>  net/bridge/br_vlan.c     | 30 +++++-------------------------
>  2 files changed, 13 insertions(+), 34 deletions(-)
> 

What about the group_addr option ? Changing it will not generate a notification.

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

[Nikolay Aleksandrov]

On 03/16/2016 02:34 PM, Xin Long wrote:
> 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 <lucien.xin@gmail.com>
> ---
>  net/bridge/br_sysfs_if.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 

Generally looks good, but it creates an inconsistency between bridge fdb_flush
and port fdb_flush since the latter will generate a notification while the
bridge flush will not.

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

[Nikolay Aleksandrov]

On 03/16/2016 02:34 PM, Xin Long wrote:
> 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 <lucien.xin@gmail.com>
> ---
>  net/bridge/br_ioctl.c | 40 ++++++++++++++++++++++++----------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 

LGTM,

Acked-by: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>

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

[Xin Long]

On Wed, Mar 16, 2016 at 10:14 PM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 03/16/2016 02:34 PM, Xin Long wrote:
>> 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.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>>  net/bridge/br_sysfs_br.c | 17 ++++++++---------
>>  net/bridge/br_vlan.c     | 30 +++++-------------------------
>>  2 files changed, 13 insertions(+), 34 deletions(-)
>>
>
> What about the group_addr option ? Changing it will not generate a notification.
>
>

group_addr is not a string-to-long convert in sysfs. so it's hard to use
store_bridge_parm, that's why I didn't modify it.

in group_addr_store():
it also tries to hold rtnl_lock. maybe we can send rtnl msg there.
what do you think?

when I cooked this patch, I was wondering why br_recalculate_fwd_mask
"Must be protected by RTNL."

Re: [PATCH net-next 0/6] bridge: support sending rntl info when we set attributes through sysfs/ioctl

[Nikolay Aleksandrov]

On 03/16/2016 02:34 PM, Xin Long wrote:
> 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.
> 

Please also include the bridge maintainers (CCed).

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

[Nikolay Aleksandrov]

On 03/16/2016 03:29 PM, Xin Long wrote:
> On Wed, Mar 16, 2016 at 10:14 PM, Nikolay Aleksandrov
> <nikolay@cumulusnetworks.com> wrote:
>> On 03/16/2016 02:34 PM, Xin Long wrote:
>>> 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.
>>>
>>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>>> ---
>>>  net/bridge/br_sysfs_br.c | 17 ++++++++---------
>>>  net/bridge/br_vlan.c     | 30 +++++-------------------------
>>>  2 files changed, 13 insertions(+), 34 deletions(-)
>>>
>>
>> What about the group_addr option ? Changing it will not generate a notification.
>>
>>
> 
> group_addr is not a string-to-long convert in sysfs. so it's hard to use
> store_bridge_parm, that's why I didn't modify it.
> 
> in group_addr_store():
> it also tries to hold rtnl_lock. maybe we can send rtnl msg there.
> what do you think?
Sounds good.

> 
> when I cooked this patch, I was wondering why br_recalculate_fwd_mask
> "Must be protected by RTNL."
> 
vlan_enabled and vlan_proto are changed under rtnl, also this can race with
changing via netlink

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

[Xin Long]

On Wed, Mar 16, 2016 at 10:23 PM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 03/16/2016 02:34 PM, Xin Long wrote:
>> 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 <lucien.xin@gmail.com>
>> ---
>>  net/bridge/br_sysfs_if.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>
> Generally looks good, but it creates an inconsistency between bridge fdb_flush
> and port fdb_flush since the latter will generate a notification while the
> bridge flush will not.
>
yeah, because port fdb_flush is called by brport_store(), in the
common function.
do you think it''s redundant if we add a notification in bridge
fdb_flush to keep
consistence with port fdb_flush?

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

[Xin Long]

On Wed, Mar 16, 2016 at 10:45 PM, Xin Long <lucien.xin@gmail.com> wrote:
> yeah, because port fdb_flush is called by brport_store(), in the
> common function.
> do you think it''s redundant if we add a notification in bridge
> fdb_flush to keep
> consistence with port fdb_flush?
just change it on patch 1/6.

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

[Nikolay Aleksandrov]

On 03/16/2016 03:45 PM, Xin Long wrote:
> On Wed, Mar 16, 2016 at 10:23 PM, Nikolay Aleksandrov
> <nikolay@cumulusnetworks.com> wrote:
>> On 03/16/2016 02:34 PM, Xin Long wrote:
>>> 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 <lucien.xin@gmail.com>
>>> ---
>>>  net/bridge/br_sysfs_if.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>
>> Generally looks good, but it creates an inconsistency between bridge fdb_flush
>> and port fdb_flush since the latter will generate a notification while the
>> bridge flush will not.
>>
> yeah, because port fdb_flush is called by brport_store(), in the
> common function.
Right.

> do you think it''s redundant if we add a notification in bridge
> fdb_flush to keep
> consistence with port fdb_flush?
> 
Hmm, technically we're doing this via a sysfs option and the netlink fdb flush
one will generate a notification, so I'd say let's make them all consistent and
make them all generate a notification, and also making the bridge fdb_flush use
the bridge_store_parm should be trivial.

Thanks,
 Nik

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

[Xin Long]

On Wed, Mar 16, 2016 at 10:49 PM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 03/16/2016 03:45 PM, Xin Long wrote:
>> do you think it''s redundant if we add a notification in bridge
>> fdb_flush to keep
>> consistence with port fdb_flush?
>>
> Hmm, technically we're doing this via a sysfs option and the netlink fdb flush
> one will generate a notification, so I'd say let's make them all consistent and
> make them all generate a notification, and also making the bridge fdb_flush use
> the bridge_store_parm should be trivial.
>
okay, I will also make this one use bridge_store_parm.

Thanks

Re: [PATCH net-next 0/6] bridge: support sending rntl info when we set attributes through sysfs/ioctl

[Xin Long]

On Wed, Mar 16, 2016 at 10:30 PM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 03/16/2016 02:34 PM, Xin Long wrote:
>> 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.
>>
>
> Please also include the bridge maintainers (CCed).
>
okay,  I will post v2 with CC maintainers.
thanks

Re: [PATCH net-next 0/6] bridge: support sending rntl info when we set attributes through sysfs/ioctl

[David Miller]

From: Xin Long <lucien.xin@gmail.com>
Date: Wed, 16 Mar 2016 21:34:43 +0800

> 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.

This is too late for net-next, please wait until after the merge window.

Thanks.