Re: [PATCH net-next 28/37] can: ems_usb: use usb_kill_urb() to stop the intr URB

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Now that unlink_all_urbs() stops the intr URB with
  usb_kill_urb(), there is a new race window on close.

Pre-existing issues:
- [Critical] unlink_all_urbs() in drivers/net/can/usb/ems_usb.c (lines
  755-757) can free the RX coherent buffers twice.
- [Medium] ems_usb_start() does not undo partial setup when it fails.

--

can: ems_usb: use usb_kill_urb() to stop the intr URB

This commit changes unlink_all_urbs() to stop the intr URB with
usb_kill_urb() instead of usb_unlink_urb(). The goal is to make sure
ems_usb_read_interrupt_callback() has returned before
ems_usb_disconnect() frees dev->intr_in_buffer and the candev.

Link: https://patch.msgid.link/20260923030522.409344-1-fanwu01@zju.edu.cn

> diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c
> index 24cf8f651f8f..2d31f0b86859 100644
> --- a/drivers/net/can/usb/ems_usb.c
> +++ b/drivers/net/can/usb/ems_usb.c
> @@ -748,7 +748,7 @@ static void unlink_all_urbs(struct ems_usb *dev)
>  {
>  	int i;
>  
> -	usb_unlink_urb(dev->intr_urb);
> +	usb_kill_urb(dev->intr_urb);

[Severity: Low]
Can this now log a spurious error during a normal ifdown?

usb_kill_urb() raises urb->reject before it unlinks the URB. Suppose the
intr URB completes with status 0 while ems_usb_close()->unlink_all_urbs()
is inside usb_kill_urb(). In that case ems_usb_read_interrupt_callback()
takes the case 0 path and resubmits:

ems_usb_read_interrupt_callback() {
    ...
	err = usb_submit_urb(urb, GFP_ATOMIC);

	if (err == -ENODEV)
		netif_device_detach(netdev);
	else if (err)
		netdev_err(netdev, "failed resubmitting intr urb: %d\n", err);
}

usb_hcd_link_urb_to_ep() in drivers/usb/core/hcd.c then rejects the
resubmit:

	/* Check that the URB isn't being killed */
	if (unlikely(atomic_read(&urb->reject))) {
		rc = -EPERM;
		goto done;
	}

As a result, "failed resubmitting intr urb: -1" gets printed. The
netif_device_present() check at the top of the callback doesn't catch
this, because the device is still present during close. usb_unlink_urb()
never set reject, so this message couldn't appear here before.

The kill itself still completes, so the only effect is log noise. Would it
make sense to treat -EPERM as an expected result, alongside -ENODEV?

>  
>  	usb_kill_anchored_urbs(&dev->rx_submitted);
>  

[Severity: Critical]
This isn't a bug introduced by this patch, but it is in the same helper
and the same disconnect teardown. Can unlink_all_urbs() free the RX
coherent buffers twice?

The loop just below this hunk frees every dev->rxbuf[i] but never clears
the pointers:

	for (i = 0; i < MAX_RX_URBS; ++i)
		usb_free_coherent(dev->udev, RX_BUFFER_SIZE,
				  dev->rxbuf[i], dev->rxbuf_dma[i]);

unlink_all_urbs() is called from ems_usb_close() and also unconditionally
from ems_usb_disconnect():

ems_usb_disconnect() {
	...
		unregister_netdev(dev->netdev);

		unlink_all_urbs(dev);
	...
}

There are three ways to reach a second free:

If the interface is up at unplug, unregister_netdev() closes it and
ems_usb_close()->unlink_all_urbs() frees the buffers. ems_usb_disconnect()
then calls unlink_all_urbs() again on the same pointers.

If the interface was opened and closed earlier and is down at unplug,
ems_usb_disconnect() frees the stale pointers from the last session a
second time.

If a later ems_usb_start() breaks out of the RX loop early because an
alloc or submit fails, the slots it didn't refill still hold pointers that
the previous close already freed. The next close or disconnect frees them
again.

usb_free_coherent() only skips a NULL addr, so each stale pointer goes to
hcd_buffer_free(). Depending on the HCD, that means dma_pool_free(),
kfree() or gen_pool_free() on memory that was already freed.

This loop appears to date from commit 9969e3c5f40c ("can: ems_usb: fix
memory leak").

Could dev->rxbuf[i] be set to NULL after each free, or could the extra
unlink_all_urbs() call in ems_usb_disconnect() be dropped?

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Does
ems_usb_start() leave URBs in flight when it fails part way through?

If usb_submit_urb(dev->intr_urb) fails, ems_usb_start() returns while the
RX URBs are still anchored on rx_submitted and in flight.

If a later ems_usb_control_cmd() or ems_usb_write_mode() call fails, it
returns with both the RX URBs and the self-resubmitting intr URB still
active:

failed:
	netdev_warn(netdev, "couldn't submit control: %d\n", err);

	return err;

On this path ems_usb_open() only calls close_candev(). ndo_stop isn't
called after a failed open, so unlink_all_urbs() never runs.

On the next open, ems_usb_start() overwrites dev->rxbuf[], so the earlier
buffers and URBs leak. usb_fill_int_urb() also rewrites the intr URB while
it is still active. usb_submit_urb() then hits:

	if (urb->hcpriv) {
		WARN_ONCE(1, "URB %p submitted while active\n", urb);
		return -EBUSY;
	}

>From then on every open fails until the device is replugged.

This doesn't reopen the disconnect use-after-free fixed here, because
ems_usb_disconnect()->unlink_all_urbs() now kills the leftover intr URB
before the frees.

Should ems_usb_start() unwind the submitted URBs on these error paths?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261015015336.816543-2-mkl%40pengutronix.de