Re: [PATCH net] net: 8139too: cancel the thread work after unregister_netdev()

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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).

- [High] After this patch, the case the commit says it fixes (the thread
  work still live when __rtl8139_cleanup_dev() calls free_netdev()) can…

--

net: 8139too: cancel the thread work after unregister_netdev()

This patch moves cancel_delayed_work_sync(&tp->thread) in
rtl8139_remove_one() so it runs after unregister_netdev(). Before the move,
rtl8139_open() or the TX watchdog could queue the work again after the
cancel and before the netdev is freed.

> diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c
> index 8241bcf766644..efcf46fd9842f 100644
> --- a/drivers/net/ethernet/realtek/8139too.c
> +++ b/drivers/net/ethernet/realtek/8139too.c
> @@ -1103,10 +1103,10 @@ static void rtl8139_remove_one(struct pci_dev *pdev)
>  
>  	assert (dev != NULL);
>  
> -	cancel_delayed_work_sync(&tp->thread);
> -
>  	unregister_netdev (dev);
>  
> +	cancel_delayed_work_sync(&tp->thread);
> +

[Severity: High]
Can a running instance of tp->thread still be live when
__rtl8139_cleanup_dev() calls free_netdev()?

The commit message says rtl8139_tx_timeout() reinitializes the work, and
that code is still present:

drivers/net/ethernet/realtek/8139too.c:rtl8139_tx_timeout() {
    ...
	tp->watchdog_fired = 1;
	if (!tp->have_thread) {
		INIT_DELAYED_WORK(&tp->thread, rtl8139_thread);
		schedule_delayed_work(&tp->thread, next_tick);
	}
}

On every HAS_LNK_CHNG chip except CH_8139_K, have_thread stays 0,
because rtl8139_start_thread() returns early:

	else if (tp->drv_flags & HAS_LNK_CHNG)
		return;

So this INIT path runs on most RTL8139 boards.

dev_watchdog() does not refresh trans_start, and only the work itself
wakes the stopped queue, via rtl8139_tx_timeout_task(). While the queue
stays stopped, ndo_tx_timeout fires again every TX_TIMEOUT (6*HZ).

Here is one possible sequence:

dev_watchdog()->rtl8139_tx_timeout()
  INIT_DELAYED_WORK() + schedule_delayed_work()   /* instance A */

rtl8139_thread() (instance A)
  rtnl_lock();                                    /* blocks */

dev_watchdog()->rtl8139_tx_timeout()
  INIT_DELAYED_WORK()   /* work->data reset to WORK_STRUCT_NO_POOL */
  schedule_delayed_work()                         /* instance B */

rtl8139_remove_one()
  unregister_netdev()
  cancel_delayed_work_sync()
    __cancel_work_sync()->start_flush_work()
      pool = get_work_pool(work);  /* NULL, or B's pool */
  __rtl8139_cleanup_dev()
    free_netdev(dev);

rtl8139_thread() (instance A)
  /* gets RTNL */
  if (!netif_running(dev))         /* dev and tp already freed */

cancel_delayed_work_sync() finds running instances only through the pool
stored in work->data. Once the INIT has orphaned instance A, the cancel
does not wait for it. Wouldn't that leave the use-after-free this patch
is meant to close?

The same re-init looks like it can also corrupt state while the device
is running. next_tick is a file-scope static. rtl8139_thread_iter() sets
it to HZ * 60 whenever any have_thread=1 device (RTL8129 or 8139K) is in
the system. A have_thread=0 device that then hits a TX timeout would call
INIT_DELAYED_WORK() on a timer that is still armed. timer_setup() clears
the timer linkage while the old timer wheel bucket still points to it.

Likewise, if the work is queued but not yet running, INIT_LIST_HEAD() is
applied to an entry that is still linked on pool->worklist.

Would it be better to also drop the INIT_DELAYED_WORK() from
rtl8139_tx_timeout() and keep only schedule_delayed_work()? Probe already
initializes tp->thread. The INIT line was added by 83cbb4d25771, the
commit named in the Fixes: tag.

>  	__rtl8139_cleanup_dev (dev);
>  	pci_disable_device (pdev);
>  }

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