* [PATCHv2 net] bridge: a netlink notification should be sent whenever those attributes change
@ 2016-03-08 3:27 Xin Long
2016-03-08 9:44 ` Nikolay Aleksandrov
0 siblings, 1 reply; 3+ messages in thread
From: Xin Long @ 2016-03-08 3:27 UTC (permalink / raw)
To: network dev; +Cc: davem, hannes, nikolay
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.
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
net/bridge/br_ioctl.c | 40 ++++++++++++++++++++++++----------------
net/bridge/br_sysfs_br.c | 5 +++++
net/bridge/br_sysfs_if.c | 6 +++++-
3 files changed, 34 insertions(+), 17 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)
diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
index 6b80914..09608e6 100644
--- a/net/bridge/br_sysfs_br.c
+++ b/net/bridge/br_sysfs_br.c
@@ -44,6 +44,11 @@ static ssize_t store_bridge_parm(struct device *d,
return -EINVAL;
err = (*set)(br, val);
+ if (!err) {
+ rtnl_lock();
+ netdev_state_change(br->dev);
+ rtnl_unlock();
+ }
return err ? err : len;
}
diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
index efe415a..64c8422 100644
--- a/net/bridge/br_sysfs_if.c
+++ b/net/bridge/br_sysfs_if.c
@@ -253,8 +253,12 @@ 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) {
+ rtnl_lock();
+ br_ifinfo_notify(RTM_NEWLINK, p);
+ rtnl_unlock();
ret = count;
+ }
}
rtnl_unlock();
}
--
2.1.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCHv2 net] bridge: a netlink notification should be sent whenever those attributes change
2016-03-08 3:27 [PATCHv2 net] bridge: a netlink notification should be sent whenever those attributes change Xin Long
@ 2016-03-08 9:44 ` Nikolay Aleksandrov
2016-03-08 11:32 ` Xin Long
0 siblings, 1 reply; 3+ messages in thread
From: Nikolay Aleksandrov @ 2016-03-08 9:44 UTC (permalink / raw)
To: Xin Long, network dev; +Cc: davem, hannes
On 03/08/2016 04:27 AM, 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.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
> ---
> net/bridge/br_ioctl.c | 40 ++++++++++++++++++++++++----------------
> net/bridge/br_sysfs_br.c | 5 +++++
> net/bridge/br_sysfs_if.c | 6 +++++-
> 3 files changed, 34 insertions(+), 17 deletions(-)
>
The patch should be targeted at net-next, not net.
> 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
[snip]
> static int old_deviceless(struct net *net, void __user *uarg)
> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
> index 6b80914..09608e6 100644
> --- a/net/bridge/br_sysfs_br.c
> +++ b/net/bridge/br_sysfs_br.c
> @@ -44,6 +44,11 @@ static ssize_t store_bridge_parm(struct device *d,
> return -EINVAL;
>
> err = (*set)(br, val);
> + if (!err) {
> + rtnl_lock();
This will deadlock, there's a reason rtnl_trylock() is used in sysfs options.
As I said the br_sysfs_br.c would need more work.
> + netdev_state_change(br->dev);
> + rtnl_unlock();
> + }
> return err ? err : len;
> }
>
> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
> index efe415a..64c8422 100644
> --- a/net/bridge/br_sysfs_if.c
> +++ b/net/bridge/br_sysfs_if.c
> @@ -253,8 +253,12 @@ 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) {
> + rtnl_lock();
rtnl is already held here, this way you deadlock. Again I'm really
curious how this patch was tested, you'd have deadlocked immediately.
On your next submission I'd suggest you add some notes on how you tested
the patch.
> + br_ifinfo_notify(RTM_NEWLINK, p);
> + rtnl_unlock();
> ret = count;
> + }
> }
> rtnl_unlock();
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCHv2 net] bridge: a netlink notification should be sent whenever those attributes change
2016-03-08 9:44 ` Nikolay Aleksandrov
@ 2016-03-08 11:32 ` Xin Long
0 siblings, 0 replies; 3+ messages in thread
From: Xin Long @ 2016-03-08 11:32 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: network dev, davem, Hannes Frederic Sowa
On Tue, Mar 8, 2016 at 5:44 PM, Nikolay Aleksandrov
<nikolay@cumulusnetworks.com> wrote:
> On 03/08/2016 04:27 AM, 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.
>>
>> Signed-off-by: Xin Long <lucien.xin@gmail.com>
>> ---
>> net/bridge/br_ioctl.c | 40 ++++++++++++++++++++++++----------------
>> net/bridge/br_sysfs_br.c | 5 +++++
>> net/bridge/br_sysfs_if.c | 6 +++++-
>> 3 files changed, 34 insertions(+), 17 deletions(-)
>>
>
> The patch should be targeted at net-next, not net.
got it.
>
>> 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
> [snip]
>> static int old_deviceless(struct net *net, void __user *uarg)
>> diff --git a/net/bridge/br_sysfs_br.c b/net/bridge/br_sysfs_br.c
>> index 6b80914..09608e6 100644
>> --- a/net/bridge/br_sysfs_br.c
>> +++ b/net/bridge/br_sysfs_br.c
>> @@ -44,6 +44,11 @@ static ssize_t store_bridge_parm(struct device *d,
>> return -EINVAL;
>>
>> err = (*set)(br, val);
>> + if (!err) {
>> + rtnl_lock();
>
> This will deadlock, there's a reason rtnl_trylock() is used in sysfs options.
> As I said the br_sysfs_br.c would need more work.
>
>> + netdev_state_change(br->dev);
>> + rtnl_unlock();
>> + }
>> return err ? err : len;
>> }
>>
>> diff --git a/net/bridge/br_sysfs_if.c b/net/bridge/br_sysfs_if.c
>> index efe415a..64c8422 100644
>> --- a/net/bridge/br_sysfs_if.c
>> +++ b/net/bridge/br_sysfs_if.c
>> @@ -253,8 +253,12 @@ 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) {
>> + rtnl_lock();
>
> rtnl is already held here, this way you deadlock. Again I'm really
> curious how this patch was tested, you'd have deadlocked immediately.
> On your next submission I'd suggest you add some notes on how you tested
> the patch.
sorry, I thought here is similar, :D. I will do it next submission
thanks.
>
>> + br_ifinfo_notify(RTM_NEWLINK, p);
>> + rtnl_unlock();
>> ret = count;
>> + }
>> }
>> rtnl_unlock();
>> }
>>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-03-08 11:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-08 3:27 [PATCHv2 net] bridge: a netlink notification should be sent whenever those attributes change Xin Long
2016-03-08 9:44 ` Nikolay Aleksandrov
2016-03-08 11:32 ` Xin Long
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).