Re: [PATCH v4 09/10] can: peak_usb: register the netdev only once the channel is set up

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 1

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 does not close the command-buffer race on multi-channel
  PCAN-USB FD family adapters (PCAN-USB Pro FD and PCAN-USB X6,…
- [Low] The patch moves register_candev() (pcan_usb_core.c:1024) after
  dev_init(), dev_set_bus(dev, 0) and dev_get_can_channel_id().

--

can: peak_usb: register the netdev only once the channel is set up

This patch moves register_candev() in peak_usb_create_dev() after
dev_init(), dev_set_bus() and dev_get_can_channel_id(). The netdev then
can't be opened or configured while the channel is still being set up.
The error paths no longer unregister the netdev, and PCAN-USB now uses
dev_err() when reading the channel id fails.

> 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 4c08656e77681..ccfc26d3e5a6b 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb_core.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb_core.c

[ ... ]

> @@ -1027,6 +1018,18 @@ static int peak_usb_create_dev(const struct peak_usb_adapter *peak_usb_adapter,
>  	/* get CAN channel id early */
>  	dev->adapter->dev_get_can_channel_id(dev, &dev->can_channel_id);
>  
> +	/* register the netdev only once the channel is fully set up, so that
> +	 * it can't be opened or configured while being initialized.
> +	 */
> +	err = register_candev(netdev);

[Severity: High]
Does this fully close the command buffer race on the multi-channel
PCAN-USB FD adapters (PCAN-USB Pro FD and PCAN-USB X6)?

The commit message says the patch addresses this case:

  the commands sent by the open or bittiming callbacks may race with
  the ones sent by dev_init() on the same command buffer;

On those adapters, pcan_usb_fd_init() makes every channel after the
first share the previous sibling's command buffer:

drivers/net/can/usb/peak_usb/pcan_usb_fd.c:pcan_usb_fd_init() {
    ...
		pdev->usb_if = ppdev->usb_if;
		pdev->cmd_buffer_addr = ppdev->cmd_buffer_addr;
    ...
}

So one buffer, and one fw_info, serves the whole interface. Nothing in
peak_usb appears to lock either of them.

peak_usb_probe() calls peak_usb_create_dev() for each channel in turn.
With this patch, can0 is registered at the end of channel 0's
peak_usb_create_dev(). Channel 1's peak_usb_create_dev() then runs
without RTNL:

peak_usb_create_dev(ch1)
  dev_init() -> pcan_usb_fd_init()
    pcan_usb_fd_set_clock_domain()
    pcan_usb_fd_set_can_led(dev, PCAN_UFD_LED_DEF)
  dev_set_bus(dev, 0) -> pcan_usb_fd_set_bus()
  dev_get_can_channel_id() -> pcan_usb_fd_get_can_channel_id()
    pcan_usb_fd_read_fwinfo(dev, &usb_if->fw_info)
      pcan_usb_pro_send_req()   /* memsets the shared fw_info */

Meanwhile, userspace holding only RTNL can bring up can0:

peak_usb_ndo_open() -> pcan_usb_fd_start()
  pcan_usb_fd_set_filter_std() / pcan_usb_fd_set_options()

Userspace can also set can0's bittiming through
pcan_usb_fd_set_bittiming_slow(). Both paths build commands in the same
buffer and send them with pcan_usb_fd_send_cmd(). That function writes
the EOC marker into the buffer and reads the shared fw_info->cmd_out_ep:

drivers/net/can/usb/peak_usb/pcan_usb_fd.c:pcan_usb_fd_send_cmd() {
    ...
		memset(cmd_tail, 0xff, sizeof(u64));
    ...
		err = usb_bulk_msg(dev->udev,
				   usb_sndbulkpipe(dev->udev,
						   fw_info->cmd_out_ep),
    ...
}

Could channel 1's RESET_MODE and EOC overwrite can0's 64 FILTER_STD
records, or the other way round? Could can0's bulk transfer use a
cmd_out_ep that pcan_usb_pro_send_req() has just zeroed?

The cross-channel form of this race was already there before this
patch. Moving register_candev() within a single channel can't prevent
it, though.

A fix probably needs to serialize command building and sending per
interface. Two options are a mutex in struct pcan_usb_fd_if, or holding
RTNL across the probe-time command sequence.

The last patch in the series, "can: peak_usb: only dump received bytes
on decode error", only touches peak_usb_read_bulk_callback(). So this
race seems to remain at the end of the series.

> +	if (err) {
> +		dev_err(&intf->dev, "couldn't register CAN device: %d\n", err);
> +		goto adap_dev_free;
> +	}

[Severity: Low]
Can this error path now leave the firmware reporting that a driver is
loaded?

register_candev() now runs after dev_init() has already sent commands
to the hardware. For channel 0, pcan_usb_fd_init() sends
pcan_usb_fd_drv_loaded(dev, 1), programs the clock domain and sets the
CAN LEDs to PCAN_UFD_LED_DEF. In the same way, pcan_usb_pro_init() sends
pcan_usb_pro_drv_loaded(dev, 1) and turns the device LED on.

register_candev() can fail, for example with -ENOMEM from
register_netdev(), a netdevice notifier veto, or a can_get_termination()
error. In that case the adap_dev_free path only calls peak_usb_stop_rx()
and dev_free(). pcan_usb_fd_free() and pcan_usb_pro_free() only clear
usb_if->dev[] and free memory.

Only pcan_usb_fd_exit() and pcan_usb_pro_exit() send drv_loaded(0) and
switch the LEDs off. dev_exit() is only called from
peak_usb_do_device_exit() at module unload.

Before this patch, register_candev() ran before any command reached the
device, so a registration failure left the hardware untouched.

The existing dev_set_bus() failure path and peak_usb_disconnect()
already leave the device in this state. The effect is limited to the
firmware status and the LEDs.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020-canfd_check_channel_idx-v4-0-bfbde81f8bab%40peak-system.fr