netdev - Re: [PATCH net-next 3/4] dpaa2-eth: add MAC/PHY support through phylink

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: <20191022162428.GX25745@shell.armlinux.org.uk>
Date:   Tue, 22 Oct 2019 17:24:28 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Ioana Ciornei <ioana.ciornei@....com>, davem@...emloft.net,
        netdev@...r.kernel.org, laurentiu.tudor@....com,
        f.fainelli@...il.com
Subject: Re: [PATCH net-next 3/4] dpaa2-eth: add MAC/PHY support through
 phylink

On Tue, Oct 22, 2019 at 03:06:49AM +0200, Andrew Lunn wrote:
> Hi Ioana
> 
> > +static int dpaa2_eth_connect_mac(struct dpaa2_eth_priv *priv)
> > +{
> > +	struct fsl_mc_device *dpni_dev, *dpmac_dev;
> > +	struct dpaa2_mac *mac;
> > +	int err;
> > +
> > +	dpni_dev = to_fsl_mc_device(priv->net_dev->dev.parent);
> > +	dpmac_dev = fsl_mc_get_endpoint(dpni_dev);
> > +	if (!dpmac_dev || dpmac_dev->dev.type != &fsl_mc_bus_dpmac_type)
> > +		return 0;
> > +
> > +	if (dpaa2_mac_is_type_fixed(dpmac_dev, priv->mc_io))
> > +		return 0;
> > +
> > +	mac = kzalloc(sizeof(struct dpaa2_mac), GFP_KERNEL);
> > +	if (!mac)
> > +		return -ENOMEM;
> > +
> > +	mac->mc_dev = dpmac_dev;
> > +	mac->mc_io = priv->mc_io;
> > +	mac->net_dev = priv->net_dev;
> > +
> > +	err = dpaa2_mac_connect(mac);
> > +	if (err) {
> > +		netdev_err(priv->net_dev, "Error connecting to the MAC endpoint\n");
> > +		kfree(mac);
> > +		return err;
> > +	}
> > +	priv->mac = mac;
> > +
> > +	return 0;
> > +}
> > +
> > +static void dpaa2_eth_disconnect_mac(struct dpaa2_eth_priv *priv)
> > +{
> > +	if (!priv->mac)
> > +		return;
> > +
> > +	rtnl_lock();
> > +	dpaa2_mac_disconnect(priv->mac);
> > +	kfree(priv->mac);
> > +	priv->mac = NULL;
> > +	rtnl_unlock();
> > +}
> 
> dpaa2_eth_connect_mac() does not take the rtnl lock.
> dpaa2_eth_disconnect_mac() does. This asymmetry makes me think
> something is wrong. But it could be correct....

The way the driver is written, it's fine.

dpaa2_eth_connect_mac() is called prior to the netdev being registered.
At that point, nothing is published.

dpaa2_eth_disconnect_mac() is called _prior_ to the netdev being
unregistered, so there could be live accesses happening to the phy
and phylink.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up