| 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
| ||
|
Message-ID: <20231215214523.ntk5kec32mb5vqjs@skbuf>
Date: Fri, 15 Dec 2023 23:45:23 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com,
Andrew Lunn <andrew@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
Russell King <linux@...linux.org.uk>,
linux-arm-kernel@...ts.infradead.org,
Christophe Leroy <christophe.leroy@...roup.eu>,
Herve Codina <herve.codina@...tlin.com>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Köry Maincent <kory.maincent@...tlin.com>,
Jesse Brandeburg <jesse.brandeburg@...el.com>,
Jonathan Corbet <corbet@....net>,
Marek Behún <kabel@...nel.org>,
Piergiorgio Beruto <piergiorgio.beruto@...il.com>,
Oleksij Rempel <o.rempel@...gutronix.de>,
Nicolò Veronese <nicveronese@...il.com>
Subject: Re: [PATCH net-next v4 01/13] net: phy: Introduce ethernet link
topology representation
On Fri, Dec 15, 2023 at 06:12:23PM +0100, Maxime Chevallier wrote:
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index d8e9335d415c..89daaccc9276 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1491,6 +1500,11 @@ int phy_attach_direct(struct net_device *dev, struct phy_device *phydev,
>
> if (phydev->sfp_bus_attached)
> dev->sfp_bus = phydev->sfp_bus;
> +
> + err = phy_link_topo_add_phy(&dev->link_topo, phydev,
> + PHY_UPSTREAM_MAC, dev);
> + if (err)
> + goto error;
> }
>
> /* Some Ethernet drivers try to connect to a PHY device before
> @@ -1816,6 +1830,7 @@ void phy_detach(struct phy_device *phydev)
> if (dev) {
> phydev->attached_dev->phydev = NULL;
> phydev->attached_dev = NULL;
> + phy_link_topo_del_phy(&dev->link_topo, phydev);
> }
> phydev->phylink = NULL;
>
> diff --git a/drivers/net/phy/phy_link_topology.c b/drivers/net/phy/phy_link_topology.c
> new file mode 100644
> index 000000000000..22f6372d002c
> --- /dev/null
> +++ b/drivers/net/phy/phy_link_topology.c
> +int phy_link_topo_add_phy(struct phy_link_topology *topo,
> + struct phy_device *phy,
> + enum phy_upstream upt, void *upstream)
> +{
> + struct phy_device_node *pdn;
> + int ret;
> +
> + /* Protects phy and upstream */
> + ASSERT_RTNL();
Something to think for the PHY library maintainers. This is probably
the first time when the rtnl_lock() requirement is asserted at
phy_attach_direct() time.
I haven't done too much with the patch set yet, so I don't understand
exactly from the comment what this is protecting. But I get the
following assertion failure with DSA:
[ 4.157160] ------------[ cut here ]------------
[ 4.161805] RTNL: assertion failed at drivers/net/phy/phy_link_topology.c (35)
[ 4.169124] WARNING: CPU: 0 PID: 26 at drivers/net/phy/phy_link_topology.c:35 phy_link_topo_add_phy+0x128/0x130
[ 4.179263] Modules linked in:
[ 4.209232] pc : phy_link_topo_add_phy+0x128/0x130
[ 4.214040] lr : phy_link_topo_add_phy+0x128/0x130
[ 4.293822] Call trace:
[ 4.296271] phy_link_topo_add_phy+0x128/0x130
[ 4.300730] phy_attach_direct+0xbc/0x3c4
[ 4.304752] phylink_fwnode_phy_connect+0xa8/0xf8
[ 4.309473] phylink_of_phy_connect+0x1c/0x28
[ 4.313844] dsa_user_create+0x318/0x5ac
[ 4.317778] dsa_port_setup+0x100/0x144
[ 4.321626] dsa_register_switch+0xe90/0x11f8
[ 4.325997] sja1105_probe+0x2bc/0x2e4
[ 4.329759] spi_probe+0xa4/0xc4
[ 4.332995] really_probe+0x16c/0x3fc
[ 4.336669] __driver_probe_device+0xa4/0x168
[ 4.341041] driver_probe_device+0x3c/0x220
[ 4.345238] __device_attach_driver+0x128/0x1cc
[ 4.349784] bus_for_each_drv+0xf4/0x14c
[ 4.353719] __device_attach+0xfc/0x1bc
[ 4.357567] device_initial_probe+0x14/0x20
[ 4.361764] bus_probe_device+0x94/0x100
[ 4.385371] ---[ end trace 0000000000000000 ]---
Someone please correct me if I'm wrong, but at least up until now, calling
this unlocked has been quite harmless, because we call dsa_user_phy_setup()
before register_netdevice(), and thus, the net_device is pretty much
inaccessible to the world when we attach it to the PHY.
And, while having the phydev->attached_dev pointer populated technically
makes the net_device now accessible from the PHY, this is a moot point,
because no user space command targets the PHY directly. They all target
the netdev, and through that, netdev->phydev. The netdev is still
unregistered, so it's ok to not have rtnl_lock().
It is rather going to be something that concerns those drivers which call
phy_attach_direct() after registering, for example from ndo_open().
Interestingly, phylink_disconnect_phy() has an ASSERT_RTNL() in it
even though the phylink_attach_phy() derivatives do not. I'm unable
to ascertain whether a previous unregister_netdevice() call makes this
requirement redundant or not.
> +
> + pdn = kzalloc(sizeof(*pdn), GFP_KERNEL);
> + if (!pdn)
> + return -ENOMEM;
> +
> + pdn->phy = phy;
> + switch (upt) {
> + case PHY_UPSTREAM_MAC:
> + pdn->upstream.netdev = (struct net_device *)upstream;
> + if (phy_on_sfp(phy))
> + pdn->parent_sfp_bus = pdn->upstream.netdev->sfp_bus;
> + break;
> + case PHY_UPSTREAM_PHY:
> + pdn->upstream.phydev = (struct phy_device *)upstream;
> + if (phy_on_sfp(phy))
> + pdn->parent_sfp_bus = pdn->upstream.phydev->sfp_bus;
> + break;
> + default:
> + ret = -EINVAL;
> + goto err;
> + }
> + pdn->upstream_type = upt;
> +
> + ret = xa_alloc_cyclic(&topo->phys, &phy->phyindex, pdn, xa_limit_32b,
> + &topo->next_phy_index, GFP_KERNEL);
> + if (ret)
> + goto err;
> +
> + return 0;
> +
> +err:
> + kfree(pdn);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(phy_link_topo_add_phy);
Powered by blists - more mailing lists