Re: [PATCH] net: lantiq: fix netdev leak on remove

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 (1) · Medium: 0 (1) · Low: 0

- [High] ltq_etop_remove() (drivers/net/ethernet/lantiq_etop.c:710-714)
  tears resources down in inverted order: netif_tx_stop_all_queues();…

Pre-existing issues:
- [High] ltq_etop_init() is registered as .ndo_init
  (drivers/net/ethernet/lantiq_etop.c:667), so it runs from inside…
- [Medium] In ltq_etop_remove() the PPE power domain is gated off by
  ltq_etop_hw_exit() (ltq_pmu_disable(PMU_PPE),…

--

net: lantiq: fix netdev leak on remove

The patch adds a free_netdev() call to ltq_etop_remove() so that the
net_device allocated by alloc_etherdev_mq() in ltq_etop_probe() is released
on driver removal, matching the free_netdev() already present on the probe
failure path.

> diff --git a/drivers/net/ethernet/lantiq_etop.c b/drivers/net/ethernet/lantiq_etop.c
> index 83ce3bfefa5c2..ecf2f3cbf4d9d 100644
> --- a/drivers/net/ethernet/lantiq_etop.c
> +++ b/drivers/net/ethernet/lantiq_etop.c
> @@ -711,6 +711,7 @@ static void ltq_etop_remove(struct platform_device *pdev)
>  		ltq_etop_hw_exit(dev);
>  		ltq_etop_mdio_cleanup(dev);
>  		unregister_netdev(dev);
> +		free_netdev(dev);

[Severity: High]

Should the teardown order in ltq_etop_remove() be corrected before the
free_netdev() is added here?

netif_tx_stop_all_queues() does not clear IFF_UP and does not disable NAPI
(napi_enable() is in ltq_etop_open(), napi_disable() only in
ltq_etop_stop()), so the device is still up and NAPI is still enabled when
the PHY and the DMA rings are torn down.

First, does ltq_etop_mdio_cleanup() leave dev->phydev NULL for the later
close?

drivers/net/ethernet/lantiq_etop.c:ltq_etop_mdio_cleanup() {
	phy_disconnect(dev->phydev);
	mdiobus_unregister(priv->mii_bus);
	mdiobus_free(priv->mii_bus);
}

phy_disconnect() -> phy_detach() does:

drivers/net/phy/phy_device.c:phy_detach() {
	...
		phydev->attached_dev->phydev = NULL;
	...
}

unregister_netdev() then closes the still-up device:

net/core/dev.c:unregister_netdevice_many_notify() {
	...
		if (!(dev->flags & IFF_UP))
			continue;
	...
	netif_close_many(&close_head, true);
	...
}

which calls ndo_stop == ltq_etop_stop():

drivers/net/ethernet/lantiq_etop.c:ltq_etop_stop() {
	netif_tx_stop_all_queues(dev);
	phy_stop(dev->phydev);
	...
}

and phy_stop() dereferences its argument immediately:

drivers/net/phy/phy.c:phy_stop() {
	struct net_device *dev = phydev->attached_dev;
	...
}

Does removing the driver while the interface is up therefore oops in
phy_stop(NULL) under RTNL, and does that also mean the free_netdev() added
here is never reached in exactly the case the commit message is about?

Second, can a pending NAPI poll now touch freed memory? ltq_etop_hw_exit()
-> ltq_etop_free_channel() runs with NAPI still enabled:

drivers/net/ethernet/lantiq_etop.c:ltq_etop_free_channel() {
	ltq_dma_free(&ch->dma);
	if (ch->dma.irq)
		free_irq(ch->dma.irq, priv);
	...
}

ltq_dma_free() releases the descriptor ring:

arch/mips/lantiq/xway/dma.c:ltq_dma_free() {
	ltq_dma_close(ch);
	dma_free_coherent(ch->dev, LTQ_DESC_NUM * LTQ_DESC_SIZE,
		ch->desc_base, ch->phys);
	...
}

free_irq() waits for an in-flight hardirq but not for an already scheduled
poll, and both ltq_etop_poll_rx() and ltq_etop_poll_tx() dereference
&ch->dma.desc_base[ch->dma.desc], ch->skb[] and priv->lock. With the new
free_netdev() the private area holding priv->ch[].napi, priv->lock and
ch->skb[] is freed too, so does this turn the stale descriptor access into a
use-after-free of the netdev private data?

Would the sequence unregister_netdev() (which closes the device and runs
napi_disable() while the PHY and rings are still valid), then
ltq_etop_mdio_cleanup(), then ltq_etop_hw_exit(), then free_netdev() avoid
both of these?

[Severity: Medium]

This is a pre-existing ordering problem rather than something introduced
here, but it affects whether the new free_netdev() is reached: does
ltq_etop_hw_exit() gate the power domain that the subsequent MDIO and close
paths still need?

drivers/net/ethernet/lantiq_etop.c:ltq_etop_hw_exit() {
	ltq_pmu_disable(PMU_PPE);
	...
}

ltq_etop_hw_init() establishes the dependency by calling
ltq_pmu_enable(PMU_PPE) before touching LTQ_ETOP_CFG and
LQ_PPE32_ENET_MAC_CFG, and LTQ_ETOP_MDIO lives in the same block.

After the domain is gated, ltq_etop_mdio_cleanup() -> phy_disconnect() ->
phy_detach() -> phy_suspend() still issues a BMCR write, and
mdiobus_unregister() drives further PHY accesses, all through the
timeout-free accessors:

drivers/net/ethernet/lantiq_etop.c:ltq_etop_mdio_rd() {
	while (ltq_etop_r32(LTQ_ETOP_MDIO) & MDIO_REQUEST)
		;
	...
}

If MDIO_REQUEST never clears with the block powered down, does the unbind or
rmmod thread spin here forever and never reach the free_netdev() added by
this patch? Note that phy_detach()/phy_suspend() run even when the interface
was never brought up. Would moving ltq_etop_hw_exit() after
unregister_netdev() and after the MDIO cleanup, so the domain is gated last,
address this?

>  	}
>  }
>  

One more observation about a path that this patch does not touch, but which
interacts with the free_netdev() being added:

[Severity: High]

This isn't a bug introduced by this patch, but ltq_etop_init() is installed
as .ndo_init in ltq_eth_netdev_ops, so it runs from inside
register_netdevice() with RTNL held and dev->reg_state ==
NETREG_UNINITIALIZED, while its error label performs the core's unwind
itself:

drivers/net/ethernet/lantiq_etop.c:ltq_etop_init() {
	...
	err = ltq_etop_mdio_init(dev);
	if (err)
		goto err_netdev;
	return 0;

err_netdev:
	unregister_netdev(dev);
	free_netdev(dev);
err_hw:
	ltq_etop_hw_exit(dev);
	return err;
}

Does unregister_netdev() here re-acquire RTNL via rtnl_net_dev_lock() and
deadlock the probing thread?

If it did not, free_netdev() takes the kvfree(dev) path for an
UNINITIALIZED device, and control then falls through to err_hw, where
ltq_etop_hw_exit() dereferences netdev_priv(dev) and walks priv->ch[] on
the freed allocation.

Finally, register_netdevice() propagates the ndo_init error to
ltq_etop_probe():

net/core/dev.c:register_netdevice() {
	...
		ret = dev->netdev_ops->ndo_init(dev);
		if (ret) {
			if (ret > 0)
				ret = -EIO;
			goto err_free_name;
		}
	...
}

drivers/net/ethernet/lantiq_etop.c:ltq_etop_probe() {
	...
	err = register_netdev(dev);
	if (err)
		goto err_free;
	...
err_free:
	free_netdev(dev);
	...
}

so is the same net_device freed a second time? The driver sets neither
needs_free_netdev nor a priv_destructor, so neither free looks conditional.
The path appears reachable: ltq_etop_mdio_init() returns -ENXIO when
mdiobus_register() fails or when ltq_etop_mdio_probe() finds no PHY, and
-ENOMEM when mdiobus_alloc() fails. Should the unregister_netdev() and
free_netdev() calls in ltq_etop_init() simply be dropped and the cleanup
left to the core plus probe?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260916020930.2606675-1-lgs201920130244%40gmail.com