linux-kernel - Re: [PATCH] net: phy: fix auto-negotiation in case of 'down-shift'

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: <2dc7ad93-5719-dd8a-44a9-8667a22a3b19@gmail.com>
Date:   Tue, 24 Nov 2020 16:26:43 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Antonio Borneo <antonio.borneo@...com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     Andrew Lunn <andrew@...n.ch>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Yonglong Liu <liuyonglong@...wei.com>, stable@...r.kernel.org,
        linuxarm@...wei.com, Salil Mehta <salil.mehta@...wei.com>,
        linux-stm32@...md-mailman.stormreply.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: fix auto-negotiation in case of 'down-shift'

Am 24.11.2020 um 16:17 schrieb Antonio Borneo:
> On Tue, 2020-11-24 at 14:56 +0000, Russell King - ARM Linux admin wrote:
>> On Tue, Nov 24, 2020 at 03:38:48PM +0100, Antonio Borneo wrote:
>>> If the auto-negotiation fails to establish a gigabit link, the phy
>>> can try to 'down-shift': it resets the bits in MII_CTRL1000 to
>>> stop advertising 1Gbps and retries the negotiation at 100Mbps.
>>>
>>> From commit 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode
>>> in genphy_read_status") the content of MII_CTRL1000 is not checked
>>> anymore at the end of the negotiation, preventing the detection of
>>> phy 'down-shift'.
>>> In case of 'down-shift' phydev->advertising gets out-of-sync wrt
>>> MII_CTRL1000 and still includes modes that the phy have already
>>> dropped. The link partner could still advertise higher speeds,
>>> while the link is established at one of the common lower speeds.
>>> The logic 'and' in phy_resolve_aneg_linkmode() between
>>> phydev->advertising and phydev->lp_advertising will report an
>>> incorrect mode.
>>>
>>> Issue detected with a local phy rtl8211f connected with a gigabit
>>> capable router through a two-pairs network cable.
>>>
>>> After auto-negotiation, read back MII_CTRL1000 and mask-out from
>>> phydev->advertising the modes that have been eventually discarded
>>> due to the 'down-shift'.
>>
>> Sorry, but no. While your solution will appear to work, in
>> introduces unexpected changes to the user visible APIs.
>>
>>>  	if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>>> +		if (phydev->is_gigabit_capable) {
>>> +			adv = phy_read(phydev, MII_CTRL1000);
>>> +			if (adv < 0)
>>> +				return adv;
>>> +			/* update advertising in case of 'down-shift' */
>>> +			mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
>>> +							adv);
>>
>> If a down-shift occurs, this will cause the configured advertising
>> mask to lose the 1G speed, which will be visible to userspace.
> 
> You are right, it gets propagated to user that 1Gbps is not advertised
> 
>> Userspace doesn't expect the advertising mask to change beneath it.
>> Since updates from userspace are done using a read-modify-write of
>> the ksettings, this can have the undesired effect of removing 1G
>> from the configured advertising mask.
>>
>> We've had other PHYs have this behaviour; the correct solution is for
>> the PHY driver to implement reading the resolution from the PHY rather
>> than relying on the generic implementation if it can down-shift
> 
> If it's already upstream, could you please point to one of the phy driver
> that already implements this properly?
> 

See e.g. aqr107_read_rate(), used by aqr107_read_status().

> Thanks
> Antonio
>