Re: [PATCH net] net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Possible ABBA deadlock between rtnl_mutex and the TX-timeout
  work, added to the suspend path.
- [Medium] The new RTNL section does not cover the netif_running() check
  that decides whether to call emac_stop()/emac_open().

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is High
  severity and involves the same work/RTNL pair.
- [High] This problem wasn't introduced by this patch, but it is High
  severity and sits in the function the patch modifies.

--

net: spacemit: Hold RTNL lock in emac_resume() and emac_suspend()

This patch takes rtnl_lock() around the emac_open() call in emac_resume()
and around the emac_stop() call in emac_suspend(). The aim is to silence
a lockdep "suspicious RCU usage" warning from phy_disconnect(), which
reads a pointer with rtnl_dereference().

> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
> index 71a80838bffd0..aca25a9e1e9ef 100644
> --- a/drivers/net/ethernet/spacemit/k1_emac.c
> +++ b/drivers/net/ethernet/spacemit/k1_emac.c
> @@ -2033,7 +2033,10 @@ static int emac_resume(struct device *dev)
>  	if (!netif_running(ndev))
>  		return 0;
>  
> +	rtnl_lock();
>  	ret = emac_open(ndev);
> +	rtnl_unlock();
> +
>  	if (ret) {
>  		clk_disable_unprepare(priv->bus_clk);
>  		return ret;

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. It sits
in the function being changed, though. What state is the device left
in when emac_open() fails here?

By this point the unwind in emac_open() has freed the TX/RX rings. One
of two things has also happened:

- emac_up() failed, and its error path called phy_disconnect() and
  pm_runtime_put_sync().
- emac_phy_connect() failed, and ndev->phydev is still NULL from the
  previous suspend.

The IRQ is not requested and NAPI is disabled. emac_resume() then gates
bus_clk and returns, but __LINK_STATE_START and IFF_UP are still set.

The next "ip link set down", unregister, or emac_suspend() (which sees
netif_running() as true) would then run:

emac_stop()
  emac_down()
    phy_disconnect(ndev->phydev)   <- phydev is NULL
      phy_is_started(phydev)       <- dereferences phydev->state

Can this oops?

Past that point, emac_down() would:

- write MAC registers with the bus clock gated
- call free_irq() on an IRQ that is not requested
- call napi_disable() on a NAPI that is already disabled
- underflow the runtime PM usage count

Also, emac_clean_rx_desc_ring() would index the NULL rx_desc_buf, and
emac_suspend() would call clk_disable_unprepare(priv->bus_clk) a second
time.

Separately, emac_alloc_tx_resources() and emac_alloc_rx_resources()
kfree() the buffer array when dma_alloc_coherent() fails, but they do
not set the pointer to NULL:

	if (!tx_ring->desc_addr) {
		kfree(tx_ring->tx_desc_buf);
		return -ENOMEM;
	}

Could a later teardown free tx_desc_buf (or rx_desc_buf) a second time
through that stale pointer?

> @@ -2056,7 +2059,9 @@ static int emac_suspend(struct device *dev)
>  		return 0;
>  	}
>  
> +	rtnl_lock();
>  	emac_stop(ndev);
> +	rtnl_unlock();

[Severity: Medium]
emac_suspend() checks netif_running() to decide whether to call
emac_stop(). That check runs before rtnl_lock() and is not repeated
under the lock:

	if (!ndev || !netif_running(ndev)) {
		clk_disable_unprepare(priv->bus_clk);
		return 0;
	}

	rtnl_lock();
	emac_stop(ndev);

Can another RTNL holder close the device in between?

Here is one example. The netdev is moved into a netns, and that netns
is torn down as system sleep starts. cleanup_net runs on netns_wq,
which is not freezable. It holds RTNL and calls dev_close(), which
calls emac_stop().

Meanwhile emac_suspend() has already seen netif_running() as true. It
blocks in rtnl_lock(), then runs emac_stop() a second time. There,
emac_down() calls phy_disconnect(ndev->phydev) with phydev already
NULL, and phy_is_started() dereferences it.

emac_resume() has the mirror case. It also tests netif_running() before
rtnl_lock(), so it could call emac_open() on a device that was just
closed. That would leak the rings, the IRQ and the PHY attach.

Would it make sense to take rtnl_lock() before the netif_running()
test, with netif_device_detach() / netif_device_attach() inside the
same locked section?

[Severity: Medium]
Does holding RTNL here invert the lock order with tx_timeout_task?
emac_stop()->emac_down() does:

	cancel_work_sync(&priv->tx_timeout_task);

and the work handler takes RTNL first:

emac_tx_timeout_task() {
	...
	rtnl_lock();
	...
}

emac_tx_timeout() queues the work with schedule_work(). That uses the
system workqueue, which is not freezable, so the work can run during
system suspend:

CPU1 (worker)                      CPU2 (suspend)
emac_tx_timeout_task()
                                   emac_suspend()
                                     rtnl_lock()
  rtnl_lock()  <- blocks
                                     emac_stop()
                                       emac_down()
                                         cancel_work_sync()
                                           <- waits for CPU1

cancel_work_sync() goes through __flush_work()->start_flush_work().
That finds the executing worker with find_worker_executing_work() and
waits on a barrier queued behind it.

Can this hang suspend? Lockdep may also report a circular dependency
between rtnl_mutex and the work's lockdep_map, which would swap one
splat for another.

The ndo_stop path (dev_close() under RTNL, then emac_stop()) already
has this pattern. This patch adds it to the system sleep path too.

Could the synchronous cancel be kept out of any RTNL-held section?
Options include cancelling the work before taking RTNL, using
rtnl_trylock() and requeueing in the work, or a disable_work() based
scheme.

[Severity: High]
This isn't a bug introduced by this patch, but it involves the same
work and RTNL pair. Can emac_tx_timeout_task() wait on itself?

emac_tx_timeout_task()          <- running as priv->tx_timeout_task
  rtnl_lock()
  dev_close(ndev)
    __dev_close_many()
      emac_stop()               <- ndo_stop
        emac_down()
          cancel_work_sync(&priv->tx_timeout_task)

At this point the work is executing, not queued. start_flush_work()
finds the calling worker itself through find_worker_executing_work().
It then inserts a barrier that can only run after the current work
returns, so wait_for_completion() in __flush_work() never returns.

Would every TX timeout recovery leave the worker stuck while holding
rtnl_mutex? That would block every RTNL user. With this patch, the next
emac_suspend() would also block forever in its new rtnl_lock().

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