On Tue, Mar 8, 2016 at 5:44 PM, Nikolay Aleksandrov
<niko...@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....@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();
>> }
>>
>
