Re: [PATCH 3/4] can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: gs_usb: wrong rx_errors increment in gs_usb_receive_bulk_callback()
--

commit de6c839020ea7849981f0dbb5e199313eeae8ce2
Author: Marc Kleine-Budde <mkl@pengutronix.de>

can: gs_usb: gs_usb_receive_bulk_callback(): check for overflow flag if SKB allocation fails

When SKB allocation fails in gs_usb_receive_bulk_callback(), this commit
updates the RX statistics and jumps to check for the GS_CAN_FLAG_OVERFLOW
flag in the host frame before resubmitting the URB.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 985d6a9113d96..5687544af99c8 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -673,8 +673,12 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  	if (hf->echo_id == GS_HOST_FRAME_ECHO_ID_RX) { /* normal rx */
>  		if (hf->flags & GS_CAN_FLAG_FD) {
>  			skb = alloc_canfd_skb(netdev, &cfd);
> -			if (!skb)
> -				goto resubmit_urb;
> +			if (!skb) {
> +				netdev->stats.rx_dropped++;
> +				netdev->stats.rx_errors++;
> +
> +				goto check_overflow;
> +			}
>  
>  			cfd->can_id = le32_to_cpu(hf->can_id);
>  			cfd->len = data_length;
> @@ -686,8 +690,12 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  			memcpy(cfd->data, hf->canfd->data, data_length);
>  		} else {
>  			skb = alloc_can_skb(netdev, &cf);
> -			if (!skb)
> -				goto resubmit_urb;
> +			if (!skb) {
> +				netdev->stats.rx_dropped++;
> +				netdev->stats.rx_errors++;
> +
> +				goto check_overflow;
> +			}
>  
>  			cf->can_id = le32_to_cpu(hf->can_id);
>  			can_frame_set_cc_len(cf, hf->can_dlc, dev->can.ctrlmode);
> @@ -728,6 +736,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  		netif_wake_queue(netdev);
>  	}
>  
> +check_overflow:
>  	if (hf->flags & GS_CAN_FLAG_OVERFLOW) {
>  		stats->rx_over_errors++;
>  		stats->rx_errors++;

[Severity: Medium]
Should gs_usb_receive_bulk_callback() only increment stats->rx_dropped
(and not netdev->stats.rx_errors) when alloc_canfd_skb() or
alloc_can_skb() returns NULL?

In standard net_device_stats accounting (and in other CAN drivers under
drivers/net/can/), a host SKB allocation failure is a drop due to lack of
host memory rather than a link or hardware receive error, so only
rx_dropped is incremented.

Furthermore, if GS_CAN_FLAG_OVERFLOW is also set on the host frame when
allocation fails, jumping to check_overflow increments stats->rx_errors a
second time for the same frame:

    check_overflow:
        if (hf->flags & GS_CAN_FLAG_OVERFLOW) {
            stats->rx_over_errors++;
            stats->rx_errors++;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025-gs_usb-fixes-v1-0-f74ebbd4da36@pengutronix.de?part=3