netdev - Re: [PATCH net] net: phy: realtek: clear status if link is down

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: <Z4dCig1kd-BhSHqD@makrotopia.org>
Date: Wed, 15 Jan 2025 05:07:22 +0000
From: Daniel Golle <daniel@...rotopia.org>
To: Andrew Lunn <andrew@...n.ch>
Cc: Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] net: phy: realtek: clear status if link is down

Hi Andrew,

On Wed, Jan 15, 2025 at 03:50:33AM +0100, Andrew Lunn wrote:
> On Wed, Jan 15, 2025 at 12:46:11AM +0000, Daniel Golle wrote:
> > Clear speed, duplex and master/slave status in case the link is down
> > to avoid reporting bogus link(-partner) properties.
> > 
> > Fixes: 5cb409b3960e ("net: phy: realtek: clear 1000Base-T link partner advertisement")
> > Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> > ---
> >  drivers/net/phy/realtek.c | 20 ++++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> > index f65d7f1f348e..3f0e03e2abce 100644
> > --- a/drivers/net/phy/realtek.c
> > +++ b/drivers/net/phy/realtek.c
> > @@ -720,8 +720,12 @@ static int rtlgen_read_status(struct phy_device *phydev)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	if (!phydev->link)
> > +	if (!phydev->link) {
> > +		phydev->duplex = DUPLEX_UNKNOWN;
> > +		phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> > +		phydev->speed = SPEED_UNKNOWN;
> >  		return 0;
> > +	}
> >  
> 
> I must be missing something here...
> 
> 
> rtlgen_read_status() first calls genphy_read_status(phydev);
> [...]
> Why is that not sufficient ?

The problem are the stale NBase-T link-partner advertisement bits and the
subsequent call to phy_resolve_aneg_linkmode(), which results in bogus
speed and duplex, based on previously connected link partner advertising
2500Base-T, 5GBase-T or 10GBase-T modes.

The more elegant solution I found by now is to just always call
mii_10gbt_stat_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
before calling rtlgen_read_status().
In case the link is up, rtlgen_decode_physr() will anyway set speed and
duplex.

> > @@ -1041,8 +1045,12 @@ static int rtl822x_c45_read_status(struct phy_device *phydev)
> >  		mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising, val);
> >  	}
> >  
> > -	if (!phydev->link)
> > +	if (!phydev->link) {
> > +		phydev->duplex = DUPLEX_UNKNOWN;
> > +		phydev->master_slave_state = MASTER_SLAVE_STATE_UNKNOWN;
> > +		phydev->speed = SPEED_UNKNOWN;
> >  		return 0;
> > +	}
> 
> 
> rtl822x_c45_read_status() calls genphy_c45_read_link() which again
> clears state from phydev.

rtl822x_c45_read_status() calls genphy_c45_read_status(), which calls
genphy_c45_read_lpa(), and that doesn't clear either
ETHTOOL_LINK_MODE_1000baseT_Half_BIT nor ETHTOOL_LINK_MODE_1000baseT_Full_BIT
as there is no generic handling for 1000Base-T in Clause-45.

So also in the Clause-45 case, the subsequent call to
phy_resolve_aneg_linkmode() may then wrongly populate speed and duplex, this
time according to the stale 1000baseT bits.

Moving the call to rtl822x_c45_read_status() in rtl822x_c45_read_status() to
after the 1000baseT lpa bits have been taken care of fixes that part of the
issue.

Clearing master_slave_state in the C45 case is still necessary because it isn't
done by genphy_c45_read_status().

I will post a series replacing this patch for all 3 described changes.