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

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

[Myeonghun Pak]

rtl8150_disconnect() kills the RX tasklet before killing the URBs.
Endpoint flushing does not wait for completion handlers: a
read_bulk_callback() that passed the unplug check can still schedule
the tasklet after the first kill. During driver unbind, disabled
endpoints make RX resubmissions fail, so rx_fixup() can keep
rescheduling itself and access freed driver data after free_netdev().

Kill the tasklet again after unlink_all_urbs() has waited for all
completion handlers. tasklet_kill() drains any late tasklet and prevents
self-rescheduling; disabled endpoints prevent it from leaving an RX
URB in flight. Keep the first kill to stop an existing reschedule loop
before unregister_netdev().

Found by a static analysis tool under development.

Fixes: eff674a514bd ("USB: rtl8150_disconnect() needs tasklet_kill()")
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/usb/rtl8150.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index d51e43170e03..98c36c377956 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);
 		free_all_urbs(dev);
 		free_skb_pool(dev);
 		dev_kfree_skb(dev->rx_skb);

base-commit: 0edbc945eb9edd0086f25b151a79cfe4adc0dfa8
-- 
2.43.0

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

[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:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

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: usb: rtl8150: fix rx tasklet use-after-free on disconnect

[Oliver Neukum]

[-- Attachment #1: Type: text/plain, Size: 1703 bytes --]

On 10.11.26 05:30, Myeonghun Pak wrote:

Hi,

> Kill the tasklet again after unlink_all_urbs() has waited for all
> completion handlers. tasklet_kill() drains any late tasklet and prevents
> self-rescheduling; disabled endpoints prevent it from leaving an RX
> URB in flight. Keep the first kill to stop an existing reschedule loop
> before unregister_netdev().
> 
> Found by a static analysis tool under development.

Good catch; nice tool.
But the fix is not correct.

> Fixes: eff674a514bd ("USB: rtl8150_disconnect() needs tasklet_kill()")
> 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/usb/rtl8150.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index d51e43170e03..98c36c377956 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);

The tasklet, however, can also submit the URB again, which in turn
can submit the tasklet and so on. There is an unavoidable window
in that sequence, as tasklet_kill() will kill the tasklet, but it may
have run in the window.

Hence you need to also, in addition to your patch, actually prevent the
resubmission of the URBs. The attached fix will do so.

How do we do this? Shall I submit? Do you want a joint submission?

	Regards
		Oliver

[-- Attachment #2: 0001-net-usb-rtl8150-break-cycle-between-tasklet-and-URB.patch --]
[-- Type: text/x-patch, Size: 1907 bytes --]

From c8b88d45caef0c2fdfa0b1a51f77c2db567658bf Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@suse.com>
Date: Tue, 10 Nov 2026 11:26:34 +0100
Subject: [PATCH] net: usb: rtl8150: break cycle between tasklet and URB

The tasklet can resubmit the URB, which in turn can
resubmit the tasklet. The cycle needs to be broken
the simplest way to do so is to prevent the resubmission
of the URBs by poisoning them.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/rtl8150.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index d51e43170e03..635e65a0de43 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -353,11 +353,18 @@ static void free_all_urbs(rtl8150_t * dev)
 	usb_free_urb(dev->intr_urb);
 }
 
+static void reenable_all_urbs(rtl8150_t * dev)
+{
+	usb_unpoison_urb(dev->rx_urb);
+	usb_unpoison_urb(dev->tx_urb);
+	usb_unpoison_urb(dev->intr_urb);
+}
+
 static void unlink_all_urbs(rtl8150_t * dev)
 {
-	usb_kill_urb(dev->rx_urb);
-	usb_kill_urb(dev->tx_urb);
-	usb_kill_urb(dev->intr_urb);
+	usb_poison_urb(dev->rx_urb);
+	usb_poison_urb(dev->tx_urb);
+	usb_poison_urb(dev->intr_urb);
 }
 
 static inline struct sk_buff *pull_skb(rtl8150_t *dev)
@@ -518,7 +525,7 @@ static void intr_callback(struct urb *urb)
 	res = usb_submit_urb (urb, GFP_ATOMIC);
 	if (res == -ENODEV)
 		netif_device_detach(dev->netdev);
-	else if (res)
+	else if (res != -EPERM)
 		dev_err(&dev->udev->dev,
 			"can't resubmit intr, %s-%s/input0, status %d\n",
 			dev->udev->bus->bus_name, dev->udev->devpath, res);
@@ -751,6 +758,7 @@ static int rtl8150_open(struct net_device *netdev)
 	if (!dev->rx_skb)
 		return -ENOMEM;
 
+	reenable_all_urbs(dev);
 	set_registers(dev, IDR, 6, netdev->dev_addr);
 
 	usb_fill_bulk_urb(dev->rx_urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
-- 
2.56.0

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

[sashiko-bot]

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