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: <20240623032106.3e854124@fedora>
Date: Sun, 23 Jun 2024 03:21:06 +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, Andrew, Russell,

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

After scratching my head maybe a bit too hard and re-reading the
replies from Andrew and Russell, I think there's indeed a problem. The
SFP case as described by Russell, from my understanding, leads me to
believe that the way PHY's are tracked by phy_link_topology is correct,
but that doesn't mean that what I try do to in this exact patch is
right.

After the phydev has been retrieved from the topology and stored in the
req_info, nothing guarantees that the PHY won't vanish between the
moment we get it here and the moment we use it in the ethnl command
handling (SFP removal being a good example, and probably(?) the only
problematic case).

A solution would be, as Russell says, to make sure we get the PHY and
do whatever we need to do with it with rtnl held. Fortunately that
shouldn't require significant rework of individual netlink commands
that use the phydev, as they already manipulate it while holding rtnl().

So, I'll ditch this idea of storing the phydev pointer in
the req_info, I'll just store the phy_index (if it was passed by user)
and grab the phy whenever we need to.

Let me know if you find some flaw in my analysis, and thanks for
spotting this.

Best regards,

Maxime