netdev - Re: [PATCH net] net: dsa: link interfaces with the DSA master to get rid of lockdep warnings

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] [day] [month] [year] [list]

Message-ID: <20200716160638.4ddf469f@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 16 Jul 2020 16:06:38 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, andrew@...n.ch,
        f.fainelli@...il.com, vivien.didelot@...il.com,
        xiyou.wangcong@...il.com, ap420073@...il.com
Subject: Re: [PATCH net] net: dsa: link interfaces with the DSA master to
 get rid of lockdep warnings

On Mon, 13 Jul 2020 19:24:43 +0300 Vladimir Oltean wrote:
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index 743caabeaaa6..a951b2a7d79a 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -1994,6 +1994,13 @@ int dsa_slave_create(struct dsa_port *port)
>  			   ret, slave_dev->name);
>  		goto out_phy;
>  	}
> +	rtnl_lock();
> +	ret = netdev_upper_dev_link(master, slave_dev, NULL);
> +	rtnl_unlock();
> +	if (ret) {
> +		unregister_netdevice(slave_dev);

The error handling here looks sketchy.

First of all please move this unregister to the error path below, not
inside the body of the if.

Secondly as a rule of thumb the error path should resemble the destroy
function.

Here we have :

	unregister_netdevice(slave_dev);
out_phy:
	rtnl_lock();
	phylink_disconnect_phy(p->dp->pl);
	rtnl_unlock();
	phylink_destroy(p->dp->pl);
out_gcells:
	gro_cells_destroy(&p->gcells);
out_free:
	free_percpu(p->stats64);
	free_netdev(slave_dev);
	port->slave = NULL;
	return ret;

vs.

	netif_carrier_off(slave_dev);
	rtnl_lock();
	phylink_disconnect_phy(dp->pl);
	rtnl_unlock();

	dsa_slave_notify(slave_dev, DSA_PORT_UNREGISTER);
	unregister_netdev(slave_dev);
	phylink_destroy(dp->pl);
	gro_cells_destroy(&p->gcells);
	free_percpu(p->stats64);
	free_netdev(slave_dev);


Ordering is different, plus you're missing the dsa_slave_notify() and
netif_carrier_off().

> +		goto out_phy;
> +	}
>  
>  	return 0;
>  
> @@ -2013,11 +2020,13 @@ int dsa_slave_create(struct dsa_port *port)
>  
>  void dsa_slave_destroy(struct net_device *slave_dev)
>  {
> +	struct net_device *master = dsa_slave_to_master(slave_dev);
>  	struct dsa_port *dp = dsa_slave_to_port(slave_dev);
>  	struct dsa_slave_priv *p = netdev_priv(slave_dev);
>  
>  	netif_carrier_off(slave_dev);
>  	rtnl_lock();
> +	netdev_upper_dev_unlink(master, slave_dev);
>  	phylink_disconnect_phy(dp->pl);
>  	rtnl_unlock();