netdev - RE: [PATCH net-next v4 4/5] 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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 19 Nov 2019 16:22:46 +0000
From:   Ioana Ciornei <ioana.ciornei@....com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Laurentiu Tudor <laurentiu.tudor@....com>,
        "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>
Subject: RE: [PATCH net-next v4 4/5] dpaa2-eth: add MAC/PHY support through
 phylink

> Subject: Re: [PATCH net-next v4 4/5] dpaa2-eth: add MAC/PHY support
> through phylink
> 
> On Thu, Oct 31, 2019 at 01:18:31AM +0200, Ioana Ciornei wrote:
> > The dpaa2-eth driver now has support for connecting to its associated
> > PHY device found through standard OF bindings.
> >
> > This happens when the DPNI object (that the driver probes on) gets
> > connected to a DPMAC. When that happens, the device tree is looked up
> > by the DPMAC ID, and the associated PHY bindings are found.
> >
> > The old logic of handling the net device's link state by hand still
> > needs to be kept, as the DPNI can be connected to other devices on the
> > bus than a DPMAC: other DPNI, DPSW ports, etc. This logic is only
> > engaged when there is no DPMAC (and therefore no phylink instance)
> > attached.
> >
> > The MC firmware support multiple type of DPMAC links: TYPE_FIXED,
> > TYPE_PHY. The TYPE_FIXED mode does not require any DPMAC
> management
> > from Linux side, and as such, the driver will not handle such a DPMAC.
> >
> > Although PHYLINK typically handles SFP cages and in-band AN modes, for
> > the moment the driver only supports the RGMII interfaces found on the
> > LX2160A. Support for other modes will come later.
> >
> > Signed-off-by: Ioana Ciornei <ioana.ciornei@....com>
> > Reviewed-by: Andrew Lunn <andrew@...n.ch>
> 
> ...
> 
> > @@ -3363,6 +3425,13 @@ static irqreturn_t dpni_irq0_handler_thread(int
> irq_num, void *arg)
> >  	if (status & DPNI_IRQ_EVENT_ENDPOINT_CHANGED) {
> >  		set_mac_addr(netdev_priv(net_dev));
> >  		update_tx_fqids(priv);
> > +
> > +		rtnl_lock();
> > +		if (priv->mac)
> > +			dpaa2_eth_disconnect_mac(priv);
> > +		else
> > +			dpaa2_eth_connect_mac(priv);
> > +		rtnl_unlock();
> 
> Sorry, but this locking is deadlock prone.
> 
> You're taking rtnl_lock().
> dpaa2_eth_connect_mac() calls dpaa2_mac_connect().
> dpaa2_mac_connect() calls phylink_create().
> phylink_create() calls phylink_register_sfp().
> phylink_register_sfp() calls sfp_bus_add_upstream().
> sfp_bus_add_upstream() calls rtnl_lock() *** DEADLOCK ***.

I recently discovered this also when working on adding support for SFPs.

> 
> Neither phylink_create() nor phylink_destroy() may be called holding the rtnl
> lock.
> 

Just to summarise:

* phylink_create() and phylink_destroy() should NOT be called with the rtnl lock held
* phylink_disconnect_phy() should be called with the lock
* depending on when the netdev is registered the phylink_of_phy_connect()
may be called with or without the rtnl lock

I'll have to move the lock and unlock around the actual _connect() and _disconnect_phy()
calls so that even the case where the DPMAC is connected at runtime
(the EVENT_ENDPOINT_CHANGED referred above) is treated correctly.

Thanks,
Ioana