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

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

[Myeonghun Pak]

rtl8139_remove_one() cancels tp->thread before unregister_netdev().
rtl8139_open() or the TX watchdog can queue it again before the device
is closed, leaving work pending when __rtl8139_cleanup_dev() frees
the netdev and its embedded work.

Move the cancel after unregister_netdev(), which closes the device
and stops the watchdog. The work then sees the device is down and
does not re-arm itself. Synchronous cancellation in rtl8139_close()
could deadlock because it runs under RTNL, which the work also acquires.
Disabling the work before unregister_netdev() would not suffice because
rtl8139_tx_timeout() reinitializes it.

Found by a static analysis tool under development.

Fixes: 83cbb4d25771 ("8139too: RTNL and flush_scheduled_work deadlock")
Cc: stable@vger.kernel.org
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>

---

 drivers/net/ethernet/realtek/8139too.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c
index 8241bcf76664..efcf46fd9842 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);
+
 	__rtl8139_cleanup_dev (dev);
 	pci_disable_device (pdev);
 }

base-commit: 0edbc945eb9edd0086f25b151a79cfe4adc0dfa8
-- 
2.43.0

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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