Re: [PATCH] ieee802154: atusb: drop the URB reference after a failed resubmit

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 (2) · 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] The fix is incomplete, and the commit message describes the bug
  wrongly.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  atusb_disconnect() (drivers/net/ieee802154/atusb.c) kills and frees…
- [High] This problem wasn't introduced by this patch, but the lifetime
  of the delayed work atusb->work (embedded in hw->priv) is guarded only…

--

ieee802154: atusb: drop the URB reference after a failed resubmit

This patch adds a usb_free_urb() call to atusb_work_urbs(). The call runs
after a failed atusb_submit_rx_urb() has put the URB back on the idle
anchor. Its purpose is to drop the reference that usb_get_from_anchor()
hands to the caller.

> diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c
> index 5f7fc4ee7a071..c924f17171f46 100644
> --- a/drivers/net/ieee802154/atusb.c
> +++ b/drivers/net/ieee802154/atusb.c
> @@ -183,6 +183,7 @@ static void atusb_work_urbs(struct work_struct *work)
>  	} while (!ret);
>  
>  	usb_anchor_urb(urb, &atusb->idle_urbs);
> +	usb_free_urb(urb);

[Severity: High]
Doesn't the reference from usb_get_from_anchor() also leak when
atusb_submit_rx_urb() succeeds?

The commit message says:

    When atusb_submit_rx_urb() fails, that reference is not consumed,

On success, atusb_submit_rx_urb() doesn't consume it either.
usb_anchor_urb(urb, &atusb->rx_urbs) takes its own reference, and
usb_hcd_submit_urb() takes another. The loop then fetches the next URB
without dropping the caller's reference:

	do {
		urb = usb_get_from_anchor(&atusb->idle_urbs);
		if (!urb)
			return;
		ret = atusb_submit_rx_urb(atusb, urb);
	} while (!ret);

The refcount through one successful RX cycle:

  on idle_urbs                                   1
  usb_get_from_anchor() (caller owns it)         1
  usb_anchor_urb(urb, &atusb->rx_urbs)           2
  usb_hcd_submit_urb()                           3
  __usb_hcd_giveback_urb()->usb_unanchor_urb()   2
  atusb_in()->usb_anchor_urb(idle_urbs)          3
  __usb_hcd_giveback_urb()->usb_put_urb()        2

So the URB goes back on idle_urbs with 2 references instead of 1, and
each later successful cycle adds one more. atusb_in() reschedules the
work for every received frame, so this is the normal RX path, not an
error path.

On disconnect, atusb_in() does not re-anchor URBs that were killed with
-ENOENT, and atusb_free_urbs() drops only one reference per idle URB.
Wouldn't every URB that was ever submitted successfully then leak on each
disconnect or unbind?

Would it be better to drop the reference after every
atusb_submit_rx_urb() call, success or failure, re-anchoring to idle_urbs
first on failure? carl9170_usb_submit_rx_urb() and btusb's play_deferred()
both do this:

	urb = usb_get_from_anchor(&ar->rx_pool);
	if (urb) {
		usb_anchor_urb(urb, &ar->rx_anch);
		err = usb_submit_urb(urb, gfp);
		if (unlikely(err)) {
			usb_unanchor_urb(urb);
			usb_anchor_urb(urb, &ar->rx_pool);
		} else {
			...
		}
		usb_free_urb(urb);
	}

That would replace the failure-only drop, and the commit message would
need updating to match.

>  	dev_warn_ratelimited(&usb_dev->dev,
>  			     "atusb_in: can't allocate/submit URB (%d)\n", ret);
>  	schedule_delayed_work(&atusb->work,

[Severity: High]
This isn't a bug introduced by this patch, but can atusb_xmit() use
atusb->tx_urb after disconnect has freed it?

atusb_disconnect() frees tx_urb before it unregisters the hw:

drivers/net/ieee802154/atusb.c:atusb_disconnect() {
    ...
	usb_kill_urb(atusb->tx_urb);
	usb_free_urb(atusb->tx_urb);

	ieee802154_unregister_hw(atusb->hw);
    ...
}

The wpan interface stays registered and up until
ieee802154_unregister_hw()->ieee802154_remove_interfaces() runs. A frame
sent in that window goes through:

ieee802154_subif_start_xmit()
  ieee802154_hot_tx()
    ieee802154_tx()
      drv_xmit_async()
        atusb_xmit()

None of these functions checks for teardown. atusb_xmit() then writes
into the freed URB and submits it:

	usb_fill_control_urb(atusb->tx_urb, usb_dev,
			     usb_sndctrlpipe(usb_dev, 0),
			     (unsigned char *)&atusb->tx_dr, skb->data,
			     skb->len, atusb_xmit_complete, NULL);
	ret = usb_submit_urb(atusb->tx_urb, GFP_ATOMIC);

After a sysfs unbind the device is still attached, so the submit can
succeed and the host controller works on freed memory.

Should ieee802154_unregister_hw() be called before the URBs are killed
and freed?

[Severity: High]
This is a pre-existing issue, but can atusb->work still be queued when
ieee802154_free_hw() frees it?

The delayed work is embedded in hw->priv. Its lifetime is guarded only by
the plain atusb->shutdown flag, which is read without a lock, and by one
cancel_delayed_work_sync() early in atusb_disconnect():

	atusb->shutdown = 1;
	cancel_delayed_work_sync(&atusb->work);

	usb_kill_anchored_urbs(&atusb->rx_urbs);

There seem to be three ways for the work to be queued after that cancel,
or freed without ever being cancelled.

First, atusb_in() checks the flag and then acts on it with nothing
holding it stable:

	usb_anchor_urb(urb, &atusb->idle_urbs);
	if (!atusb->shutdown)
		schedule_delayed_work(&atusb->work, 0);

CPU1 (atusb_in)                 CPU2 (atusb_disconnect)
reads shutdown == 0
                                atusb->shutdown = 1;
                                cancel_delayed_work_sync() (nothing queued)
schedule_delayed_work()
                                usb_kill_anchored_urbs(&atusb->rx_urbs)

usb_kill_anchored_urbs() only waits for the completion handler to finish.
It does not cancel the work that the handler queued.

Second, atusb_start() queues the work without checking shutdown:

drivers/net/ieee802154/atusb.c:atusb_start() {
    ...
	schedule_delayed_work(&atusb->work, 0);
    ...
}

The netdev stays registered until ieee802154_unregister_hw(), which runs
after the cancel. Bringing the interface up in that window queues work
that nothing cancels, because atusb_stop() only kills idle_urbs.

Third, the atusb_probe() error path that runs after
ieee802154_register_hw() has succeeded:

	ieee802154_unregister_hw(hw);
fail:
	atusb_free_urbs(atusb);
	usb_kill_urb(atusb->tx_urb);
	usb_free_urb(atusb->tx_urb);
	ieee802154_free_hw(hw);

Once register_hw has succeeded, userspace can bring wpan0 up. That runs
atusb_start() and then the work, which submits RX URBs to rx_urbs. If the
final atusb_get_and_clear_error() then fails, this path never calls
cancel_delayed_work_sync() and never kills rx_urbs. The in-flight RX URBs
hold skbs with SKB_ATUSB(skb) == atusb.

Could the queued work, or those URB completions, then touch the freed
atusb?

Should the work be cancelled again after the last point that can queue
it, and should rx_urbs be killed, on both the disconnect path and the
probe error path?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261104021458.1468481-1-vulab%40iscas.ac.cn