netdev - [RFC PATCH net] Revert "net: phy: Uniform PHY driver access"

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: <20210912192805.1394305-1-vladimir.oltean@nxp.com>
Date:   Sun, 12 Sep 2021 22:28:05 +0300
From:   Vladimir Oltean <vladimir.oltean@....com>
To:     netdev@...r.kernel.org
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Russell King <linux@...linux.org.uk>,
        Jakub Kicinski <kuba@...nel.org>,
        Gerhard Engleder <gerhard@...leder-embedded.com>,
        linux-kernel@...r.kernel.org
Subject: [RFC PATCH net] Revert "net: phy: Uniform PHY driver access"

This reverts commit 3ac8eed62596387214869319379c1fcba264d8c6.

I am not actually sure I follow the patch author's logic, because the
change does more than it says on the box, but this patch breaks
suspend/resume on NXP LS1028A and probably on any other systems which
have PHY devices with no driver bound, because the patch has removed the
"!phydev->drv" check without actually explaining why that is fine.

Examples why that is not fine:

- There is an MDIO bus with a PHY device that doesn't have a specific
  PHY driver loaded, because mdiobus_register() automatically creates a
  PHY device for it but there is no specific PHY driver in the system.
  Normally under those circumstances, the generic PHY driver will be
  bound lazily to it (at phy_attach_direct time), but since no one has
  attempted to connect to that PHY device (yet), it will not have a
  driver.

- There is an internal MDIO bus with PCS devices on it, for serial links
  such as SGMII. If the driver does not set bus->phy_mask = ~0 to
  prevent auto-scanning, PHY devices will get created for those PCSes
  too, and although those PHY devices are not used, they do not bother
  anybody either. PCS devices are usually managed in Linux as raw MDIO
  devices. Nonetheless, they do not have a PHY driver, nor does anybody
  attempt to connect to them (because they are not a PHY), and therefore
  this patch breaks that.

The stack trace is:

Unable to handle kernel NULL pointer dereference at virtual address 00000000000000e8
pc : mdio_bus_phy_suspend+0xd8/0xec
lr : dpm_run_callback+0x38/0x90
Call trace:
 mdio_bus_phy_suspend+0xd8/0xec
 dpm_run_callback+0x38/0x90
 __device_suspend+0x108/0x3cc
 dpm_suspend+0x140/0x210
 dpm_suspend_start+0x7c/0xa0
 suspend_devices_and_enter+0x13c/0x540
 pm_suspend+0x2a4/0x330

Fixes: 3ac8eed62596 ("net: phy: Uniform PHY driver access")
Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
---
 drivers/net/phy/phy_device.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9e2891d8e8dd..ba5ad86ec826 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -233,9 +233,11 @@ static DEFINE_MUTEX(phy_fixup_lock);
 
 static bool mdio_bus_phy_may_suspend(struct phy_device *phydev)
 {
+	struct device_driver *drv = phydev->mdio.dev.driver;
+	struct phy_driver *phydrv = to_phy_driver(drv);
 	struct net_device *netdev = phydev->attached_dev;
 
-	if (!phydev->drv->suspend)
+	if (!drv || !phydrv->suspend)
 		return false;
 
 	/* PHY not attached? May suspend if the PHY has not already been
-- 
2.25.1