[PATCH net 0/2] net: phy: Unbind fixes

[PATCH net 0/2] net: phy: Unbind fixes

[Florian Fainelli]

This patch series fixes a couple of issues with the unbinding of the PHY
drivers and then bringing down a network interface. The first is a NULL
pointer de-reference and the second was an incorrect warning being
triggered.

Florian Fainelli (2):
  net: phy: Avoid NPD upon phy_detach() when driver is unbound
  net: phy: Do not warn in phy_stop() on PHY_DOWN

 drivers/net/phy/phy.c        | 2 +-
 drivers/net/phy/phy_device.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH net 1/2] net: phy: Avoid NPD upon phy_detach() when driver is unbound

[Florian Fainelli]

If we have unbound the PHY driver prior to calling phy_detach() (often
via phy_disconnect()) then we can cause a NULL pointer de-reference
accessing the driver owner member. The steps to reproduce are:

echo unimac-mdio-0:01 > /sys/class/net/eth0/phydev/driver/unbind
ip link set eth0 down

Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8adfbad0a1e8..81eb76a8295b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1682,7 +1682,8 @@ void phy_detach(struct phy_device *phydev)
 
 	phy_led_triggers_unregister(phydev);
 
-	module_put(phydev->mdio.dev.driver->owner);
+	if (phydev->mdio.dev.driver)
+		module_put(phydev->mdio.dev.driver->owner);
 
 	/* If the device had no specific driver before (i.e. - it
 	 * was using the generic driver), we unbind the device
-- 
2.25.1

Re: [PATCH net 1/2] net: phy: Avoid NPD upon phy_detach() when driver is unbound

[Andrew Lunn]

On Wed, Sep 16, 2020 at 08:43:09PM -0700, Florian Fainelli wrote:
> If we have unbound the PHY driver prior to calling phy_detach() (often
> via phy_disconnect()) then we can cause a NULL pointer de-reference
> accessing the driver owner member. The steps to reproduce are:
> 
> echo unimac-mdio-0:01 > /sys/class/net/eth0/phydev/driver/unbind
> ip link set eth0 down

Hi Florian

How forceful is this unbind? Can we actually block it while the
interface is up? Or returning -EBUSY would make sense.

	  Andrew

Re: [PATCH net 1/2] net: phy: Avoid NPD upon phy_detach() when driver is unbound

[Florian Fainelli]

On 9/17/2020 6:15 AM, Andrew Lunn wrote:
> On Wed, Sep 16, 2020 at 08:43:09PM -0700, Florian Fainelli wrote:
>> If we have unbound the PHY driver prior to calling phy_detach() (often
>> via phy_disconnect()) then we can cause a NULL pointer de-reference
>> accessing the driver owner member. The steps to reproduce are:
>>
>> echo unimac-mdio-0:01 > /sys/class/net/eth0/phydev/driver/unbind
>> ip link set eth0 down
> 
> Hi Florian
> 
> How forceful is this unbind? Can we actually block it while the
> interface is up? Or returning -EBUSY would make sense.

It it not forceful, you can unbind the PHY driver from underneath the 
net_device and nothing bad happens, really. This is not a very realistic 
or practical use case, but several years ago, I went into making sure we 
would not create NPD if that happened.
-- 
Florian

Re: [PATCH net 1/2] net: phy: Avoid NPD upon phy_detach() when driver is unbound

[Andrew Lunn]

On Wed, Sep 16, 2020 at 08:43:09PM -0700, Florian Fainelli wrote:
> If we have unbound the PHY driver prior to calling phy_detach() (often
> via phy_disconnect()) then we can cause a NULL pointer de-reference
> accessing the driver owner member. The steps to reproduce are:
> 
> echo unimac-mdio-0:01 > /sys/class/net/eth0/phydev/driver/unbind
> ip link set eth0 down
> 
> Fixes: cafe8df8b9bc ("net: phy: Fix lack of reference count on PHY driver")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH net 2/2] net: phy: Do not warn in phy_stop() on PHY_DOWN

[Florian Fainelli]

When phy_is_started() was added to catch incorrect PHY states,
phy_stop() would not be qualified against PHY_DOWN. It is possible to
reach that state when the PHY driver has been unbound and the network
device is then brought down.

Fixes: 2b3e88ea6528 ("net: phy: improve phy state checking")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 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 735a806045ac..8947d58f2a25 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -996,7 +996,7 @@ void phy_stop(struct phy_device *phydev)
 {
 	struct net_device *dev = phydev->attached_dev;
 
-	if (!phy_is_started(phydev)) {
+	if (!phy_is_started(phydev) && phydev->state != PHY_DOWN) {
 		WARN(1, "called from state %s\n",
 		     phy_state_to_str(phydev->state));
 		return;
-- 
2.25.1

Re: [PATCH net 2/2] net: phy: Do not warn in phy_stop() on PHY_DOWN

[Andrew Lunn]

On Wed, Sep 16, 2020 at 08:43:10PM -0700, Florian Fainelli wrote:
> When phy_is_started() was added to catch incorrect PHY states,
> phy_stop() would not be qualified against PHY_DOWN. It is possible to
> reach that state when the PHY driver has been unbound and the network
> device is then brought down.
> 
> Fixes: 2b3e88ea6528 ("net: phy: improve phy state checking")
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net 0/2] net: phy: Unbind fixes

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 16 Sep 2020 20:43:08 -0700

> This patch series fixes a couple of issues with the unbinding of the PHY
> drivers and then bringing down a network interface. The first is a NULL
> pointer de-reference and the second was an incorrect warning being
> triggered.

Series applied and queued up for -stable, thanks.