Re: [PATCH v5 06/10] can: peak_usb: harden channel handling in PCAN-USB FD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] can: peak_usb: race and livelock between peak_usb_ndo_open() and peak_usb_stop_rx()
--

commit a785087e84d7ed9d274560d9a3d814cad1b1ffde
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>

can: peak_usb: harden channel handling in PCAN-USB FD

This patch validates the CAN channel number reported in PCAN-USB FD
firmware messages against the adapter's controller count and checks for
NULL device slots. It also stops RX URBs on sibling channels when probe
fails or during disconnect before freeing or unregistering devices.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb_core.c b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> index a1b17e1e6fea6..4c08656e77681 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> @@ -574,6 +574,9 @@ static int peak_usb_ndo_open(struct net_device *netdev)
>  	struct peak_usb_device *dev = netdev_priv(netdev);
>  	int err;
>  
> +	if (!test_bit(PCAN_USB_STATE_CONNECTED, &dev->state))
> +		return -ENODEV;
> +
>  	/* common open */
>  	err = open_candev(netdev);
>  	if (err)
> @@ -904,6 +907,22 @@ int pcan_get_ts_info(struct net_device *dev, struct kernel_ethtool_ts_info *info
>  	return 0;
>  }
>  
> +/*
> + * stop RX on dev and on all its previous siblings: clearing CONNECTED makes
> + * peak_usb_ndo_open() fail, then poisoning the RX anchors waits for running
> + * completion handlers and makes any later submission on them fail.
> + */
> +static void peak_usb_stop_rx(struct peak_usb_device *dev)
> +{
> +	struct peak_usb_device *d;
> +
> +	for (d = dev; d; d = d->prev_siblings)
> +		clear_bit(PCAN_USB_STATE_CONNECTED, &d->state);
> +
> +	for (d = dev; d; d = d->prev_siblings)
> +		usb_poison_anchored_urbs(&d->rx_submitted);
> +}

[Severity: High]
Can peak_usb_stop_rx() race with a concurrent peak_usb_ndo_open() and
trigger a livelock in usb_poison_anchored_urbs() or leave the device open
with all RX URBs killed?

peak_usb_ndo_open() runs under rtnl_lock, whereas peak_usb_stop_rx() is
called from peak_usb_disconnect() and the peak_usb_create_dev() error path
without holding rtnl_lock. If peak_usb_ndo_open() passes the
PCAN_USB_STATE_CONNECTED check before peak_usb_stop_rx() clears the bit,
peak_usb_ndo_open() proceeds into peak_usb_start():

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_start() {
    ...
		usb_anchor_urb(urb, &dev->rx_submitted);

		err = usb_submit_urb(urb, GFP_KERNEL);
		if (err) {
			if (err == -ENODEV)
				netif_device_detach(dev->netdev);

			usb_unanchor_urb(urb);
			usb_free_urb(urb);
			break;
		}
    ...
}

In peak_usb_start(), each newly allocated RX URB is anchored to
dev->rx_submitted before usb_submit_urb() is called (and remains anchored
with use_count == 0 until usb_unanchor_urb() if submission fails). If
peak_usb_stop_rx() calls usb_poison_anchored_urbs(&d->rx_submitted) during
that window:

drivers/usb/core/urb.c:usb_poison_anchored_urbs() {
	do {
		spin_lock_irq(&anchor->lock);
		anchor->poisoned = 1;
		while (!list_empty(&anchor->urb_list)) {
			victim = list_entry(anchor->urb_list.prev,
					    struct urb, anchor_list);
			/* make sure the URB isn't freed before we kill it */
			usb_get_urb(victim);
			spin_unlock_irq(&anchor->lock);
			/* this will unanchor the URB */
			usb_poison_urb(victim);
			usb_put_urb(victim);
			spin_lock_irq(&anchor->lock);
		}
		surely_empty = usb_anchor_check_wakeup(anchor);

		spin_unlock_irq(&anchor->lock);
		cpu_relax();
	} while (!surely_empty);
}

Because the unsubmitted URB has urb->ep == NULL and urb->use_count == 0,
usb_poison_urb() returns immediately without sleeping or unanchoring it, so
usb_poison_anchored_urbs() spins in the inner while loop without yielding.
If peak_usb_stop_rx() preempted peak_usb_start() on the same CPU, could
this cause a soft lockup?

Also, if peak_usb_start() submits at least one RX URB (i >= 1) before
peak_usb_stop_rx() poisons dev->rx_submitted, usb_poison_anchored_urbs()
kills the already-submitted URB(s) and causes the next usb_submit_urb() to
fail with -EPERM. Since peak_usb_start() only treats RX submission failure
as fatal when i == 0:

drivers/net/can/usb/peak_usb/pcan_usb_core.c:peak_usb_start() {
    ...
	if (i < PCAN_USB_MAX_RX_URBS) {
		if (i == 0) {
			netdev_err(netdev, "couldn't setup any rx URB\n");
			return err;
		}

		netdev_warn(netdev, "rx performance may be slow\n");
	}
    ...
}

and pcan_usb_fd_send_cmd() returns 0 once PCAN_USB_STATE_CONNECTED is
cleared, wouldn't peak_usb_ndo_open() succeed with all RX URBs killed after
teardown has begun?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261020-canfd_check_channel_idx-v5-0-a62b35b867f0@peak-system.fr?part=6