Re: [PATCH net] can: esd_usb: free_candev() after unlink_all_urbs() in disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Moving `unlink_all_urbs()` before `unregister_netdev()` introduces a memory leak race with `esd_usb_open()` on the RX path.
--

commit f21a9588c02c39e2094757bcdada4fb7aca593c8
Author: Xiang Mei <xmei5@asu.edu>

can: esd_usb: free_candev() after unlink_all_urbs() in disconnect

This commit fixes a slab-use-after-free issue by killing URBs before freeing
the CAN devices.

> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index d257440fa01ff..34c0ca1872839 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -1385,6 +1385,7 @@ static void esd_usb_disconnect(struct usb_interface *intf)
>  
>  	if (dev) {
>  		dev->in_usb_disconnect = 1;
> +		unlink_all_urbs(dev);

[Severity: Medium]
Does moving unlink_all_urbs() before unregister_netdev() introduce a memory
leak regression by racing with esd_usb_open() on the RX path?

Previously, unregister_netdev() ran first, providing rtnl_lock 
synchronization. Now that unlink_all_urbs() runs before unregister_netdev(),
esd_usb_open() could run concurrently if the interface is brought up at the
exact same time the device is disconnected.

If this race occurs, unlink_all_urbs() could scan the buffers while they are
still NULL:

drivers/net/can/usb/esd_usb.c:unlink_all_urbs() {
        ...
        for (i = 0; i < ESD_USB_MAX_RX_URBS; ++i)
                usb_free_coherent(dev->udev, ESD_USB_RX_BUFFER_SIZE,
                                  dev->rxbuf[i], dev->rxbuf_dma[i]);
        ...
}

And concurrently, esd_usb_setup_rx_urbs() could allocate them immediately
after the check:

drivers/net/can/usb/esd_usb.c:esd_usb_setup_rx_urbs() {
        ...
        dev->rxbuf[i] = buf;
        dev->rxbuf_dma[i] = buf_dma;
        ...
}

Could these newly allocated DMA buffers be permanently leaked since
unlink_all_urbs() has already completed its cleanup pass?

>  		for (i = 0; i < dev->net_count; i++) {
>  			if (dev->nets[i]) {
>  				netdev = dev->nets[i]->netdev;
>  				netdev_info(netdev, "unregister\n");
>  				unregister_netdev(netdev);
>  				free_candev(netdev);
>  			}
>  		}
> -		unlink_all_urbs(dev);
>  		kfree(dev);
>  	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705010005.1169943-1-xmei5@asu.edu?part=1