Re: [PATCH net 1/3] net: atl1: disable work before freeing the netdev

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 (1) · Medium: 1 · 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] The commit message offers atl2_remove() as a correct sibling:
  "atl2_remove() cancels both works and deletes the timer before…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  atl1_reset_dev_task()…

--

net: atl1: disable work before freeing the netdev

This patch changes atl1_remove() to call disable_work_sync() on
reset_dev_task and link_chg_task before unregister_netdev(). After that,
it deletes the PHY config timer. The goal is to stop either work from
running after free_netdev().

> Disable both works synchronously before unregistering the interface,
> then delete the PHY configuration timer.  disable_work_sync() drains a
> queued or running instance and rejects a later queue_work(), which a
> plain cancel_work_sync() would not.  Deleting the timer after the works
> have stopped also prevents a running link work from rearming it.
> atl2_remove() cancels both works and deletes the timer before
> unregistering for the same reason.

[Severity: Medium]
Is atl2_remove() a correct precedent here? By the reasoning in this
message, it looks like atl2 still has the same use-after-free:

drivers/net/ethernet/atheros/atlx/atl2.c:atl2_remove() {
    ...
	set_bit(__ATL2_DOWN, &adapter->flags);

	timer_delete_sync(&adapter->watchdog_timer);
	timer_delete_sync(&adapter->phy_config_timer);
	cancel_work_sync(&adapter->reset_task);
	cancel_work_sync(&adapter->link_chg_task);

	unregister_netdev(netdev);
    ...
}

When the cancel_work_sync() calls run, the IRQ is still requested. It is
only freed by atl2_close(), from inside unregister_netdev(), and
dev_watchdog is still active as well. That leaves these paths able to
queue work again after the cancels:

  atl2_intr()
    schedule_work(&adapter->reset_task);      (link-down and DMA errors)
  atl2_intr() -> atl2_check_for_link()
    schedule_work(&adapter->link_chg_task);   (ISR_PHY or ISR_MANUAL)
  dev_watchdog -> atl2_tx_timeout()
    schedule_work(&adapter->reset_task);

None of these check __ATL2_DOWN. atl2_close() and atl2_down() never
cancel the works, so anything queued in that window can still be pending
at free_netdev().

The ordering is also the reverse of what the message says is needed.
atl2 deletes the timers before it cancels the works, and relies on
__ATL2_DOWN to block rearming. But atl2_reset_task() ->
atl2_reinit_locked() -> atl2_up() runs:

	clear_bit(__ATL2_DOWN, &adapter->flags);

atl2_up() also sets MASTER_CTRL_MANUAL_INT, which can raise a manual
interrupt that queues link_chg_task again.

No other patch in the series touches atl2.c. Should the message stop
describing atl2_remove() as equivalent, or should atl2_remove() get the
same conversion?

> diff --git a/drivers/net/ethernet/atheros/atlx/atl1.c b/drivers/net/ethernet/atheros/atlx/atl1.c
> index 2883e9462f6fd..2bd35c3cfc820 100644
> --- a/drivers/net/ethernet/atheros/atlx/atl1.c
> +++ b/drivers/net/ethernet/atheros/atlx/atl1.c
> @@ -3132,6 +3132,18 @@ static void atl1_remove(struct pci_dev *pdev)
>  
>  	adapter = netdev_priv(netdev);
>  
> +	/*
> +	 * reset_dev_task is queued from the interrupt handler and from the
> +	 * tx watchdog; link_chg_task is queued from the interrupt handler
> +	 * when ISR_GPHY reports a link event.  Disable both before
> +	 * unregistering the interface so that neither a pending nor a late
> +	 * queue attempt can run after free_netdev().
> +	 */
> +	disable_work_sync(&adapter->reset_dev_task);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Can
atl1_reset_dev_task() race with atl1_close() and the ethtool
reconfiguration paths? It takes no lock and does not check
netif_running():

drivers/net/ethernet/atheros/atlx/atl1.c:atl1_reset_dev_task() {
    ...
	netif_device_detach(netdev);
	atl1_down(adapter);
	atl1_up(adapter);
	netif_device_attach(netdev);
}

atl1_close() (ndo_stop), atl1_change_mtu(), atl1_set_link_ksettings(),
atl1_set_ringparam() and atl1_nway_reset() all run atl1_down() and
atl1_up() under RTNL. Nothing serializes them against this work, and
atl1_close() does not cancel it:

drivers/net/ethernet/atheros/atlx/atl1.c:atl1_close() {
    ...
	atl1_down(adapter);
	atl1_free_ring_resources(adapter);
	return 0;
}

Here is one possible sequence:

  atlx_tx_timeout() or atl1_intr()
    queue reset_dev_task
  ip link set dev ethX down
    __dev_close_many()           (no netif_device_present() check)
      atl1_close()
        atl1_down()              napi_disable(), free_irq(), ring cleanup
        atl1_free_ring_resources()
  kworker: atl1_reset_dev_task()
    atl1_down()
      napi_disable()
        netdev_lock(n->dev);
        napi_disable_locked(n);  spins forever, SCHED never clears

When that happens, the kworker blocks while holding the netdev instance
lock. A later atl1_open() -> atl1_up() -> napi_enable() -> netdev_lock()
would then deadlock with RTNL held.

If the work instead runs at the same time as close or ethtool -G, both
CPUs end up in atl1_down(). That can call free_irq() twice and double
free skbs in atl1_clean_rx_ring() and atl1_clean_tx_ring(). It can also
let atl1_set_ringparam() or atl1_close() free the descriptor rings while
the worker's atl1_up() is refilling them in atl1_alloc_rx_buffers().

With this patch applied, this disable_work_sync() would also wait forever
on a worker stuck that way. Should the reset work take rtnl_lock() and
check netif_running(), as other drivers do in their reset tasks?

> +	disable_work_sync(&adapter->link_chg_task);
> +	timer_delete_sync(&adapter->phy_config_timer);
> +	adapter->phy_timer_pending = false;
> +

[ ... ]

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