netdev - Re: [PATCHv2 net] bridge: a netlink notification should be sent whenever those attributes change

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <56DE9EF3.4010501@cumulusnetworks.com>
Date:	Tue, 8 Mar 2016 10:44:19 +0100
From:	Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
To:	Xin Long <lucien.xin@...il.com>,
	network dev <netdev@...r.kernel.org>
Cc:	davem@...emloft.net, hannes@...essinduktion.org
Subject: Re: [PATCHv2 net] bridge: a netlink notification should be sent
 whenever those attributes change

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@...il.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();
>  	}
>