Re: [PATCH net] net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] net: usb: rtl8150: tasklet_kill() livelock in
  rtl8150_disconnect()
--

commit 80f52c420ee74481f60c886504dda8b0ad26ab4b
Author: Myeonghun Pak <mhun512@gmail.com>

net: usb: rtl8150: fix rx tasklet use-after-free on disconnect

This commit adds a second tasklet_kill(&dev->tl) call after
unlink_all_urbs(dev) in rtl8150_disconnect() to drain any RX tasklet
scheduled by a late URB completion handler.

> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index d51e43170e033..98c36c3779564 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -971,6 +971,8 @@ static void rtl8150_disconnect(struct usb_interface *intf)
>  		tasklet_kill(&dev->tl);
>  		unregister_netdev(dev->netdev);
>  		unlink_all_urbs(dev);
> +		/* A completion may have scheduled it after the first kill. */
> +		tasklet_kill(&dev->tl);

[Severity: High]
Can this tasklet_kill(&dev->tl) livelock and hang rtl8150_disconnect() in
TASK_UNINTERRUPTIBLE when a late read_bulk_callback() schedules dev->tl?

During driver unbind, usb_unbind_interface() disables the endpoints before
calling rtl8150_disconnect(), so usb_submit_urb(dev->rx_urb, GFP_ATOMIC)
returns -ENOENT rather than -ENODEV.  Unlike read_bulk_callback(),
rx_fixup() does not check RTL8150_UNPLUG and reschedules dev->tl whenever
usb_submit_urb() fails with a non--ENODEV error:

drivers/net/usb/rtl8150.c:rx_fixup() {
	...
	status = usb_submit_urb(dev->rx_urb, GFP_ATOMIC);
	if (status == -ENODEV) {
		netif_device_detach(dev->netdev);
	} else if (status) {
		set_bit(RX_URB_FAIL, &dev->flags);
		goto tlsched;
	} else {
		clear_bit(RX_URB_FAIL, &dev->flags);
	}

	return;
tlsched:
	tasklet_schedule(&dev->tl);
}

When read_bulk_callback() schedules dev->tl before tasklet_kill() runs,
TASKLET_STATE_SCHED is already set on entry to wait_on_bit_lock().
tasklet_action_common() clears TASKLET_STATE_SCHED, wakes the waiter, and
immediately invokes rx_fixup() in softirq context, which sets
TASKLET_STATE_SCHED back to 1 before rtl8150_disconnect() can acquire the
bit:

CPU0                                  CPU1 (<softirq>)
read_bulk_callback()
  tasklet_schedule(&dev->tl)
    /* sets TASKLET_STATE_SCHED */
rtl8150_disconnect()
  tasklet_kill(&dev->tl)
    wait_on_bit_lock(...,
      TASKLET_STATE_SCHED, ...)
      /* sleeps */
                                      tasklet_action_common()
                                        tasklet_clear_sched(t)
                                          clear_bit_unlock(...)
                                          wake_up_bit(...)
                                        rx_fixup()
                                          usb_submit_urb() -> -ENOENT
                                          tasklet_schedule(&dev->tl)
                                            /* re-sets TASKLET_STATE_SCHED */
      /* wakes up */
      test_and_set_bit(...) -> fails
      /* sleeps again */              <-- livelock

Checking test_bit(RTL8150_UNPLUG, &dev->flags) at the start of rx_fixup()
(as pegasus.c does) or using tasklet_disable() before killing the URBs
would prevent rx_fixup() from rescheduling itself during disconnect.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110043016.56986-1-mhun512@gmail.com?part=1