netdev - Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6845805d-4dae-0a3a-c56c-6feb86f4b553@gmail.com>
Date:   Wed, 4 Jan 2017 07:13:23 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Zefir Kurtisi <zefir.kurtisi@...atec.com>, netdev@...r.kernel.org
Cc:     andrew@...n.ch
Subject: Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state



On 01/04/2017 07:04 AM, Zefir Kurtisi wrote:
> While in RUNNING state, phy_state_machine() checks for link changes by
> comparing phydev->link before and after calling phy_read_status().
> This works as long as it is guaranteed that phydev->link is never
> changed outside the phy_state_machine().
> 
> If in some setups this happens, it causes the state machine to miss
> a link loss and remain RUNNING despite phydev->link being 0.
> 
> This has been observed running a dsa setup with a process continuously
> polling the link states over ethtool each second (SNMPD RFC-1213
> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
> call phy_read_status() and with that modify the link status - and
> with that bricking the phy state machine.

That's the interesting part of the analysis, how does this brick the PHY
state machine? Is the PHY driver changing the link status in the
read_status callback that it implements?

> 
> This patch adds a fail-safe check while in RUNNING, which causes to
> move to CHANGELINK when the link is gone and we are still RUNNING.
> 
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@...atec.com>
> ---
>  drivers/net/phy/phy.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 28548af..0f9a61e 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -966,6 +966,15 @@ void phy_state_machine(struct work_struct *work)
>  			if (old_link != phydev->link)
>  				phydev->state = PHY_CHANGELINK;
>  		}
> +		/*
> +		 * Failsafe: check that nobody set phydev->link=0 between two
> +		 * poll cycles, otherwise we won't leave RUNNING state as long
> +		 * as link remains down.
> +		 */
> +		if (!phydev->link && phydev->state == PHY_RUNNING) {
> +			phydev->state = PHY_CHANGELINK;
> +			dev_warn(&phydev->dev, "no link in PHY_RUNNING\n");
> +		}
>  		break;
>  	case PHY_CHANGELINK:
>  		err = phy_read_status(phydev);
> 

-- 
Florian