netdev - Re: [PATCH net-next v13 05/13] net: ethtool: Allow passing a phy index for some commands

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: <20240616180231.338c2e6c@fedora>
Date: Sun, 16 Jun 2024 18:02:31 +0200
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, thomas.petazzoni@...tlin.com, Andrew Lunn
 <andrew@...n.ch>, 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>, Vladimir Oltean <vladimir.oltean@....com>,
 Köry Maincent <kory.maincent@...tlin.com>, Jesse Brandeburg
 <jesse.brandeburg@...el.com>, Marek Behún
 <kabel@...nel.org>, Piergiorgio Beruto <piergiorgio.beruto@...il.com>,
 Oleksij Rempel <o.rempel@...gutronix.de>, Nicolò Veronese
 <nicveronese@...il.com>, Simon Horman <horms@...nel.org>,
 mwojtas@...omium.org, Nathan Chancellor <nathan@...nel.org>, Antoine Tenart
 <atenart@...nel.org>
Subject: Re: [PATCH net-next v13 05/13] net: ethtool: Allow passing a phy
 index for some commands

Hello Jakub,

On Thu, 13 Jun 2024 18:26:13 -0700
Jakub Kicinski <kuba@...nel.org> wrote:

> On Fri,  7 Jun 2024 09:18:18 +0200 Maxime Chevallier wrote:
> > +		if (tb[ETHTOOL_A_HEADER_PHY_INDEX]) {
> > +			struct nlattr *phy_id;
> > +
> > +			phy_id = tb[ETHTOOL_A_HEADER_PHY_INDEX];
> > +			phydev = phy_link_topo_get_phy(dev,
> > +						       nla_get_u32(phy_id));  
> 
> Sorry for potentially repeating question (please put the answer in the
> commit message) - are phys guaranteed not to disappear, even if the
> netdev gets closed? this has no rtnl protection

I'll answer here so that people can correct me if I'm wrong, but I'll
also add it in the commit logs as well (and possibly with some fixes
depending on how this discussion goes)

While a PHY can be attached to/detached from a netdevice at open/close,
the phy_device itself will keep on living, as its lifetime is tied to
the underlying mdio_device (however phy_attach/detach take a ref on the
phy_device, preventing it from vanishing while it's attached to a
netdev)

I think the worst that could happen is that phy_detach() gets
called (at ndo_close() for example, but that's not the only possible
call site for that), and right after we manually unbind the PHY, which
will drop its last refcount, while we hold a pointer to it :

			phydev = phy_link_topo_get_phy()
 phy_detach(phydev)
 unbind on phydev
			/* access phydev */
			
PHY device lifetime is, from my understanding, not protected by
rtnl() so should a lock be added, I don't think rtnl_lock() would be
the one to use.

Maybe instead we should grab a reference to the phydev when we add it
to the topology ?

> 
> > +			if (!phydev) {
> > +				NL_SET_BAD_ATTR(extack, phy_id);
> > +				return -ENODEV;
> > +			}
> > +		} else {
> > +			/* If we need a PHY but no phy index is specified, fallback
> > +			 * to dev->phydev  
> 
> please double check the submission for going over 80 chars, this one
> appears to be particularly pointlessly over 80 chars...

Arg yes sorry about this one...
 
> > +			 */
> > +			phydev = dev->phydev;  

Thanks,

Maxime