[PATCH 0/2] net: macb: Fixes for macb driver

[PATCH 0/2] net: macb: Fixes for macb driver

[Claudiu Beznea]

Hi,

This series tries to address stack traces printed when unbinding the
macb driver.

Thank you,
Claudiu Beznea

Claudiu Beznea (2):
  net: phy: Check phydev->drv before dereferencing it
  net: macb: Unregister nedev before MDIO bus in remove

 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 drivers/net/phy/phy.c                    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

-- 
2.39.2

[PATCH 1/2] net: phy: Check phydev->drv before dereferencing it

[Claudiu Beznea]

The macb driver calls mdiobus_unregister() and mdiobus_free() in its remove
function before calling unregister_netdev(). unregister_netdev() calls the
driver-specific struct net_device_ops::ndo_stop function (macb_close()),
and macb_close() calls phylink_disconnect_phy(). This, in turn, will call:

phy_disconnect() ->
  phy_free_interrupt() ->
    phy_disable_interrupts() ->
      phy_config_interrupt()

which dereference phydev->drv, which was already freed by:
mdiobus_unregister() ->
  phy_mdio_device_remove() ->
    device_del() ->
      bus_remove_device() ->
        device_release_driver_internal() ->
          phy_remove()

from macb_close().

Although the sequence in the macb driver is not correct, check phydev->drv      
before dereferencing it in phy_config_interrupt() to avoid scenarios
like the one described.

Fixes: 00db8189d984 ("This patch adds a PHY Abstraction Layer to the Linux Kernel")
Signed-off-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>
---
 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 a5fa077650e8..dd98a4b3ef81 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -165,7 +165,7 @@ EXPORT_SYMBOL_GPL(phy_get_rate_matching);
 static int phy_config_interrupt(struct phy_device *phydev, bool interrupts)
 {
 	phydev->interrupts = interrupts ? 1 : 0;
-	if (phydev->drv->config_intr)
+	if (phydev->drv && phydev->drv->config_intr)
 		return phydev->drv->config_intr(phydev);
 
 	return 0;
-- 
2.39.2

Re: [PATCH 1/2] net: phy: Check phydev->drv before dereferencing it

[Russell King (Oracle)]

On Sun, Nov 26, 2023 at 04:10:45PM +0200, Claudiu Beznea wrote:
> The macb driver calls mdiobus_unregister() and mdiobus_free() in its remove
> function before calling unregister_netdev(). unregister_netdev() calls the
> driver-specific struct net_device_ops::ndo_stop function (macb_close()),
> and macb_close() calls phylink_disconnect_phy(). This, in turn, will call:
> 
> phy_disconnect() ->
>   phy_free_interrupt() ->
>     phy_disable_interrupts() ->
>       phy_config_interrupt()
> 
> which dereference phydev->drv, which was already freed by:
> mdiobus_unregister() ->
>   phy_mdio_device_remove() ->
>     device_del() ->
>       bus_remove_device() ->
>         device_release_driver_internal() ->
>           phy_remove()
> 
> from macb_close().
> 
> Although the sequence in the macb driver is not correct, check phydev->drv      
> before dereferencing it in phy_config_interrupt() to avoid scenarios
> like the one described.

I don't know why I've ended up with two copies of this series, but as
said in the other posting of this patch (where details of why can be
found)... NAK.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH 2/2] net: macb: Unregister nedev before MDIO bus in remove

[Claudiu Beznea]

unregister_netdev() calls the struct net_device_ops::ndo_stop function,
which in the case of the macb driver is macb_close(). macb_close() calls,
in turn, PHY-specific APIs (e.g., phy_detach()). The call trace is as
follows:

macb_close() ->
  phylink_disconnect_phy() ->
    phy_disconnect() ->
      phy_detach()

phy_detach() will remove associated sysfs files by calling
kernfs_remove_by_name_ns(), which will hit
"kernfs: cannot remove 'attached_dev', no directory" WARN(), which will
throw a stack trace too.

To avoid this, call unregiser_netdev() before mdiobus_unregister() and
mdiobus_free().

Fixes: 7897b071ac3b ("net: macb: convert to phylink")
Signed-off-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>
---
 drivers/net/ethernet/cadence/macb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index cebae0f418f2..73d041af3de1 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -5165,11 +5165,11 @@ static void macb_remove(struct platform_device *pdev)
 
 	if (dev) {
 		bp = netdev_priv(dev);
+		unregister_netdev(dev);
 		phy_exit(bp->sgmii_phy);
 		mdiobus_unregister(bp->mii_bus);
 		mdiobus_free(bp->mii_bus);
 
-		unregister_netdev(dev);
 		tasklet_kill(&bp->hresp_err_tasklet);
 		pm_runtime_disable(&pdev->dev);
 		pm_runtime_dont_use_autosuspend(&pdev->dev);
-- 
2.39.2

Re: [PATCH 2/2] net: macb: Unregister nedev before MDIO bus in remove

[Russell King (Oracle)]

On Sun, Nov 26, 2023 at 04:10:46PM +0200, Claudiu Beznea wrote:
> unregister_netdev() calls the struct net_device_ops::ndo_stop function,
> which in the case of the macb driver is macb_close(). macb_close() calls,
> in turn, PHY-specific APIs (e.g., phy_detach()). The call trace is as
> follows:
> 
> macb_close() ->
>   phylink_disconnect_phy() ->
>     phy_disconnect() ->
>       phy_detach()
> 
> phy_detach() will remove associated sysfs files by calling
> kernfs_remove_by_name_ns(), which will hit
> "kernfs: cannot remove 'attached_dev', no directory" WARN(), which will
> throw a stack trace too.
> 
> To avoid this, call unregiser_netdev() before mdiobus_unregister() and
> mdiobus_free().

Definitely the correct fix, also to fix the issue in patch 1.
> 
> Fixes: 7897b071ac3b ("net: macb: convert to phylink")
> Signed-off-by: Claudiu Beznea <claudiu.beznea@tuxon.dev>

Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Thanks!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: macb: Unregister nedev before MDIO bus in remove

[Andrew Lunn]

> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index cebae0f418f2..73d041af3de1 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -5165,11 +5165,11 @@ static void macb_remove(struct platform_device *pdev)
>  
>  	if (dev) {
>  		bp = netdev_priv(dev);
> +		unregister_netdev(dev);
>  		phy_exit(bp->sgmii_phy);
>  		mdiobus_unregister(bp->mii_bus);
>  		mdiobus_free(bp->mii_bus);
>  
> -		unregister_netdev(dev);
>  		tasklet_kill(&bp->hresp_err_tasklet);


I don't know this driver...

What does this tasklet do? Is it safe for it to run after the netdev
is unregistered, and the PHY and the mdio bus is gone? Maybe this
tasklet_kill should be after the interrupt is disabled, but before the
netdev is unregistered?

If you have one bug here, there might be others.

	Andrew

Re: [PATCH 2/2] net: macb: Unregister nedev before MDIO bus in remove

[Russell King (Oracle)]

On Sun, Nov 26, 2023 at 06:13:55PM +0100, Andrew Lunn wrote:
> > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> > index cebae0f418f2..73d041af3de1 100644
> > --- a/drivers/net/ethernet/cadence/macb_main.c
> > +++ b/drivers/net/ethernet/cadence/macb_main.c
> > @@ -5165,11 +5165,11 @@ static void macb_remove(struct platform_device *pdev)
> >  
> >  	if (dev) {
> >  		bp = netdev_priv(dev);
> > +		unregister_netdev(dev);
> >  		phy_exit(bp->sgmii_phy);
> >  		mdiobus_unregister(bp->mii_bus);
> >  		mdiobus_free(bp->mii_bus);
> >  
> > -		unregister_netdev(dev);
> >  		tasklet_kill(&bp->hresp_err_tasklet);
> 
> 
> I don't know this driver...
> 
> What does this tasklet do? Is it safe for it to run after the netdev
> is unregistered, and the PHY and the mdio bus is gone? Maybe this
> tasklet_kill should be after the interrupt is disabled, but before the
> netdev is unregistered?

.. and while evaluating Andrew's comment, check whether the tasklet
is necessary for the device to successfully close or not - remembering
that unregister_netdev() will close down an open netdev.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 2/2] net: macb: Unregister nedev before MDIO bus in remove

[claudiu beznea]

On 26.11.2023 19:13, Andrew Lunn wrote:
>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
>> index cebae0f418f2..73d041af3de1 100644
>> --- a/drivers/net/ethernet/cadence/macb_main.c
>> +++ b/drivers/net/ethernet/cadence/macb_main.c
>> @@ -5165,11 +5165,11 @@ static void macb_remove(struct platform_device *pdev)
>>  
>>  	if (dev) {
>>  		bp = netdev_priv(dev);
>> +		unregister_netdev(dev);
>>  		phy_exit(bp->sgmii_phy);
>>  		mdiobus_unregister(bp->mii_bus);
>>  		mdiobus_free(bp->mii_bus);
>>  
>> -		unregister_netdev(dev);
>>  		tasklet_kill(&bp->hresp_err_tasklet);
> 
> 
> I don't know this driver...
> 
> What does this tasklet do? 

It handles bus errors that my happens while DMA fetches data from system
memory. It re-initializes the TX/RX, DMA buffers, clear interrupts,
stop/start all tx queues.

> Is it safe for it to run after the netdev
> is unregistered, and the PHY and the mdio bus is gone? 

Not really, as it accesses netdev specific data.

> Maybe this
> tasklet_kill should be after the interrupt is disabled, but before the
> netdev is unregistered?

That would be a better place, indeed.

Thank you,
Claudiu Beznea

> 
> If you have one bug here, there might be others.
> 
> 	Andrew