netdev - [PATCH net] net: phy: make phy_error() report which PHY has failed

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-next>] [day] [month] [year] [list]

Message-Id: <E1ihCLZ-0001Vo-Nw@rmk-PC.armlinux.org.uk>
Date:   Tue, 17 Dec 2019 12:53:05 +0000
From:   Russell King <rmk+kernel@...linux.org.uk>
To:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: [PATCH net] net: phy: make phy_error() report which PHY has failed

phy_error() is called from phy_interrupt() or phy_state_machine(), and
uses WARN_ON() to print a backtrace. The backtrace is not useful when
reporting a PHY error.

However, a system may contain multiple ethernet PHYs, and phy_error()
gives no clue which one caused the problem.

Replace WARN_ON() with a call to phydev_err() so that we can see which
PHY had an error, and also inform the user that we are halting the PHY.

Fixes: fa7b28c11bbf ("net: phy: print stack trace in phy_error")
Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
---
There is another related problem in this area. If an error is detected
while the PHY is running, phy_error() moves to PHY_HALTED state. If we
try to take the network device down, then:

void phy_stop(struct phy_device *phydev)
{
        if (!phy_is_started(phydev)) {
                WARN(1, "called from state %s\n",
                     phy_state_to_str(phydev->state));
                return;
        }

triggers, and we never do any of the phy_stop() cleanup. I'm not sure
what the best way to solve this is - introducing a PHY_ERROR state may
be a solution, but I think we want some phy_is_started() sites to
return true for it and others to return false.

Heiner - you introduced the above warning, could you look at improving
this case so we don't print a warning and taint the kernel when taking
a network device down after phy_error() please?

Thanks.

 drivers/net/phy/phy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 49300fb59757..06fbca959383 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -663,7 +663,7 @@ void phy_stop_machine(struct phy_device *phydev)
  */
 static void phy_error(struct phy_device *phydev)
 {
-	WARN_ON(1);
+	phydev_err(phydev, "Error detected, halting PHY\n");

 	mutex_lock(&phydev->lock);
 	phydev->state = PHY_HALTED;
-- 
2.20.1