netdev - Re: [net-next 2/2] net: phy: Fallback to C22 access if needed in phy_mii

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: <Zump8Hmlxt3uqVFE@shell.armlinux.org.uk>
Date: Tue, 17 Sep 2024 17:10:24 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
	netdev@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [net-next 2/2] net: phy: Fallback to C22 access if needed in
 phy_mii_ioctl()

On Fri, Sep 06, 2024 at 11:36:08PM +0200, Niklas Söderlund wrote:
> On 2024-09-06 23:04:50 +0200, Andrew Lunn wrote:
> > On Fri, Sep 06, 2024 at 11:39:55AM +0200, Niklas Söderlund wrote:
> > > If a C45 only PHY is attached to a driver that only knows how to talk
> > > C22 phylib will fallback and use indirect access. This frees the driver
> > > from having to implement this themself.
> > > 
> > > The IOCTL implementation for SIOCGMIIREG and SIOCSMIIREG do not use
> > > these convenience functions and instead fail if a C45 PHY is used
> > > together with a driver that only knows how to speak C22.
> > > 
> > > Fix this by using the two convince functions that knows when to fallback
> > > to indirect access to read/write to the MDIO bus when needed.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@...natech.se>
> > > ---
> > >  drivers/net/phy/phy.c | 18 ++++++++++++------
> > >  1 file changed, 12 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > > index 4f3e742907cb..89f52bb123aa 100644
> > > --- a/drivers/net/phy/phy.c
> > > +++ b/drivers/net/phy/phy.c
> > > @@ -342,9 +342,12 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
> > >  		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
> > >  			prtad = mdio_phy_id_prtad(mii_data->phy_id);
> > >  			devad = mdio_phy_id_devad(mii_data->phy_id);
> > > -			ret = mdiobus_c45_read(phydev->mdio.bus, prtad, devad,
> > > -					       mii_data->reg_num);
> > >  
> > > +			mutex_lock(&phydev->mdio.bus->mdio_lock);
> > > +			ret = mmd_phy_read(phydev->mdio.bus, prtad,
> > > +					   phydev->is_c45, devad,
> > 
> > Using phydev->is_c45 is probably wrong.
> > 
> > mii_data->phy_id is the device on the bus you want to access. It does
> > not need to be the same device as the MAC is using. Just because the
> > device the MAC is using is a c45 device does not mean the device you
> > are trying to access is.
> 
> I think phydev->is_c45. The outer if clause is checking if the PHY I 
> want to talk to is C45, or not. The phydev->is_c45 just express if the 
> MAC can talk C45, or if it only supports C22. This is the variable 
> mmd_phy_read() uses to determine if it shall fallback to indirect C45.

Nevertheless, this is wrong for two reasons:

1) the MII access API is a raw bus access debugging API, so its up to
userspace to do the necessary accesses. Yes, it's racy... it isn't
supposed to be used for production purposes.

2) the MII access API gives access to any MDIO device on the bus that
the _associated_ PHY is attached to. This includes non-PHY devices.
Just because the PHY that is attached to the netdev *might* support
C45-over-C22 (not every PHY does), this is unique to PHYs and is not
true of every MDIO device. So "upgrading" a C45 access to a
C45-over-C22 could end up corrupting other devices on the bus.

So, I'm afraid that I disagree with your change.

> I agree it's not the best. But is it not better to have the IOCTLs 
> working as they already exists, rather then have it function depending 
> on the combination of if the MAC and PHY both speak C22 or C45?

If the bus supports C22, and userspace asks for a C22 transaction, a
C22 transaction will happen on the bus.

If the bus supports C45, and userspace asks for a C45 transaction, a
C45 transaction will happen on the bus.

Otherwise, an error is returned.

Just because PHYs have this special C45-over-C22 thing, does not mean
that the low level debugging API should use C45-over-C22 for C45
transactions when the bus does not support C45.

I re-iterate, the MII bus access ioctls provide access to any device
on the the bus not specifically the PHY that you have attached to
the netdev. Using the properties of the attached PHY is meaningless
here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!