Re: [PATCH v5 07/10] can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR

[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] In pcan_usb_decode_data()
  (drivers/net/can/usb/peak_usb/pcan_usb.c), the patch moves the SRR…

--

can: peak_usb: fix RX cursor desync for self-received RTR frames with SRR

This patch moves the SRR writer-id byte skip in pcan_usb_decode_data()
out of the non-RTR branch. The byte is now consumed for RTR frames too,
which matches the layout produced by pcan_usb_encode_msg(). Before this,
self-received RTR frames with SRR set left the RX cursor one byte short.

> diff --git a/drivers/net/can/usb/peak_usb/pcan_usb.c b/drivers/net/can/usb/peak_usb/pcan_usb.c
> index 93da0fd4f10e7..4c8cfed31634a 100644
> --- a/drivers/net/can/usb/peak_usb/pcan_usb.c
> +++ b/drivers/net/can/usb/peak_usb/pcan_usb.c
> @@ -697,16 +697,19 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
>  		memcpy(cf->data, mc->ptr, cf->len);
>  		mc->ptr += rec_len;
>  
> -		/* Ignore next byte (client private id) if SRR bit is set */
> -		if (can_id_flags & PCAN_USB_TX_SRR) {
> -			if ((mc->ptr + 1) > mc->end)
> -				goto decode_failed;
> -			mc->ptr++;
> -		}
> -
>  		/* update statistics */
>  		mc->netdev->stats.rx_bytes += cf->len;
>  	}
> +
> +	/* Ignore next byte (client private id) if SRR bit is set.
> +	 * Note: this test must be done even when the frame is an RTR frame.
> +	 */
> +	if (can_id_flags & PCAN_USB_TX_SRR) {
> +		if ((mc->ptr + 1) > mc->end)
> +			goto decode_failed;

[Severity: Low]
Can rx_bytes now be over-counted for a frame that gets dropped?

The SRR bounds check now runs after the rx_bytes update in the non-RTR
branch. Take a non-RTR record with PCAN_USB_TX_SRR set in can_id_flags
where the URB ends right after the payload, so the writer-id byte is
missing:

pcan_usb_decode_data()
    if ((mc->ptr + rec_len) > mc->end)    /* passes */
    ...
    mc->netdev->stats.rx_bytes += cf->len;
    ...
    if ((mc->ptr + 1) > mc->end)          /* fails */
        goto decode_failed;

The skb is freed, -EINVAL is returned and rx_packets is not incremented,
but rx_bytes has already gone up by cf->len.

Before this patch the SRR check came before the rx_bytes update, so a
rejected record was not counted. Nothing undoes the increment later,
because peak_usb_read_bulk_callback() only calls pcan_dump_mem() when
dev_decode_buf() returns an error.

Would it work to move the rx_bytes update below the SRR check, for
example next to the rx_packets++?

> +		mc->ptr++;
> +	}
> +
>  	mc->netdev->stats.rx_packets++;
>  
>  	/* convert timestamp into kernel time */

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