Re: [PATCH 2/4] can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] can: gs_usb: leak of unregistered candevs in gs_usb_probe() error path
--

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

can: gs_usb: gs_usb_probe(): init netdev completely before register_candev()

This commit moves register_candev() out of gs_make_candev() into a second
loop in gs_usb_probe() so that all CAN channels are fully initialized
before any netdev is registered.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 15f28833f2deb..985d6a9113d96 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -1490,18 +1490,8 @@ static struct gs_can *gs_make_candev(unsigned int channel,
>  	can_rx_offload_add_manual(netdev, &dev->offload, GS_NAPI_WEIGHT);
>  	SET_NETDEV_DEV(netdev, &intf->dev);
>  
> -	rc = register_candev(dev->netdev);
> -	if (rc) {
> -		dev_err(&intf->dev,
> -			"Couldn't register candev for channel %d (%pe)\n",
> -			channel, ERR_PTR(rc));
> -		goto out_can_rx_offload_del;
> -	}
> -
>  	return dev;
>  
> -out_can_rx_offload_del:
> -	can_rx_offload_del(&dev->offload);
>  out_free_candev:
>  	free_candev(dev->netdev);
>  	return ERR_PTR(rc);
> @@ -1593,14 +1583,14 @@ static int gs_usb_probe(struct usb_interface *intf,
>  			/* save error code to return later */
>  			rc = PTR_ERR(parent->canch[i]);
>  
> -			/* on failure destroy previously created candevs */
> +			/* on failure clean up previously created candevs */
>  			icount = i;
> -			for (i = 0; i < icount; i++)
> -				gs_destroy_candev(parent->canch[i]);
> +			for (i = 0; i < icount; i++) {
> +				can_rx_offload_del(&parent->canch[i]->offload);
> +				free_candev(parent->canch[i]->netdev);
> +			}
>  
> -			usb_kill_anchored_urbs(&parent->rx_submitted);
> -			kfree(parent);
> -			return rc;
> +			goto out_usb_kill_anchored_urbs;
>  		}
>  		parent->canch[i]->parent = parent;
>  
> @@ -1621,7 +1611,30 @@ static int gs_usb_probe(struct usb_interface *intf,
>  		parent->hf_size_rx = max(parent->hf_size_rx, hf_size_rx);
>  	}
>  
> +	for (i = 0; i < icount; i++) {
> +		struct gs_can *dev = parent->canch[i];
> +
> +		rc = register_candev(dev->netdev);
> +		if (rc) {
> +			dev_err(&intf->dev,
> +				"Couldn't register candev for channel %d (%pe)\n",
> +				i, ERR_PTR(rc));
> +
> +			icount = i;
> +			for (i = 0; i < icount; i++)
> +				gs_destroy_candev(parent->canch[i]);
> +
> +			goto out_usb_kill_anchored_urbs;
> +		}
> +	}

[Severity: Medium]
If register_candev() fails at channel i in gs_usb_probe(), does this error
path leak dev->offload and dev->netdev for channel i and all remaining
unregistered channels up to parent->channel_cnt - 1?

The first loop in gs_usb_probe() calls gs_make_candev() for all icount
channels, allocating dev->netdev via alloc_candev() and initializing
dev->offload via can_rx_offload_add_manual() for every entry in
parent->canch[].

When register_candev() fails at index i in the second loop, the rollback loop
only calls gs_destroy_candev() on the already-registered channels 0 .. i - 1
before jumping to out_usb_kill_anchored_urbs and freeing parent. The
unregistered channels from i to parent->channel_cnt - 1 never have
can_rx_offload_del() or free_candev() called on them.

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