| Message ID | 38a10eecc6347a3c47b55c86b4783c49b6dbbc99.1458134414.git.lucien.xin@gmail.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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."
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
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; }
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(-)