Re: [PATCH] comedi: usbdux: free allocated memory/urbs on failure

[Ian Abbott]

On 15/07/2021 03:26, Salah Triki wrote:
> Free allocated memory/urbs on failure in order to prevent memory leaks.
>
> Signed-off-by: Salah Triki <salah.triki@xxxxxxxxx>
> ---
>   drivers/comedi/drivers/usbdux.c | 42 ++++++++++++++++++++++++++++-----
>   1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/comedi/drivers/usbdux.c b/drivers/comedi/drivers/usbdux.c
> index 0350f303d557..fe388a9a588a 100644
> --- a/drivers/comedi/drivers/usbdux.c
> +++ b/drivers/comedi/drivers/usbdux.c
> @@ -1456,7 +1456,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
>   		/* one frame: 1ms */
>   		urb = usb_alloc_urb(1, GFP_KERNEL);
>   		if (!urb)
> -			return -ENOMEM;
> +			goto free_ai_urbs1;
>   		devpriv->ai_urbs[i] = urb;
>   
>   		urb->dev = usb;
> @@ -1465,7 +1465,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
>   		urb->transfer_flags = URB_ISO_ASAP;
>   		urb->transfer_buffer = kzalloc(SIZEINBUF, GFP_KERNEL);
>   		if (!urb->transfer_buffer)
> -			return -ENOMEM;
> +			goto free_ai_urbs2;
>   
>   		urb->complete = usbduxsub_ai_isoc_irq;
>   		urb->number_of_packets = 1;
> @@ -1478,7 +1478,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
>   		/* one frame: 1ms */
>   		urb = usb_alloc_urb(1, GFP_KERNEL);
>   		if (!urb)
> -			return -ENOMEM;
> +			goto free_ao_urbs1;
>   		devpriv->ao_urbs[i] = urb;
>   
>   		urb->dev = usb;
> @@ -1487,7 +1487,7 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
>   		urb->transfer_flags = URB_ISO_ASAP;
>   		urb->transfer_buffer = kzalloc(SIZEOUTBUF, GFP_KERNEL);
>   		if (!urb->transfer_buffer)
> -			return -ENOMEM;
> +			goto free_ao_urbs2;
>   
>   		urb->complete = usbduxsub_ao_isoc_irq;
>   		urb->number_of_packets = 1;
> @@ -1504,17 +1504,47 @@ static int usbdux_alloc_usb_buffers(struct comedi_device *dev)
>   	if (devpriv->pwm_buf_sz) {
>   		urb = usb_alloc_urb(0, GFP_KERNEL);
>   		if (!urb)
> -			return -ENOMEM;
> +			goto free_ao_urbs0;
>   		devpriv->pwm_urb = urb;
>   
>   		/* max bulk ep size in high speed */
>   		urb->transfer_buffer = kzalloc(devpriv->pwm_buf_sz,
>   					       GFP_KERNEL);
>   		if (!urb->transfer_buffer)
> -			return -ENOMEM;
> +			goto free_urb;
>   	}
>   
>   	return 0;
> +
> +free_urb:
> +	usb_free_urb(urb);
> +
> +free_ao_urbs0:
> +	i = devpriv->n_ao_urbs;
> +
> +free_ao_urbs1:
> +	i--;
> +
> +free_ao_urbs2:
> +	while (i >= 0) {
> +		kfree(devpriv->ao_urbs[i]->transfer_buffer);
> +		usb_free_urb(devpriv->ao_urbs[i]);
> +		i--;
> +	}
> +	i = devpriv->n_ai_urbs;
> +
> +free_ai_urbs1:
> +	i--;
> +
> +free_ai_urbs2:
> +	while (i >= 0) {
> +		kfree(devpriv->ai_urbs[i]->transfer_buffer);
> +		usb_free_urb(devpriv->ai_urbs[i]);
> +		i--;
> +	}
> +
> +	return -ENOMEM;
> +
>   }
>   
>   static void usbdux_free_usb_buffers(struct comedi_device *dev)

NAK.

Actually, there is no need for this because usbdux_free_usb_buffers() 
will soon be called to clean up when this fails.[*]  Also, it isn't 
cleaning up enough to prevent double frees when 
usbdux_free_usb_buffers() is called later.


[*] The error returned from usbdux_alloc_usb_buffers() is propagated as 
an error returned by usbdux_auto_attach(). The Comedi core calls the 
"detach()" handler (usbdux_detach()) to clean up when the 
"auto_attach()" handler (usbdux_auto_attach()) returns an error. 
usbdux_detach() calls usbdux_free_usb_buffers() to clean up the 
(possibly partial) allocations done by usbdux_alloc_usb_buffers().

--
-=( Ian Abbott <abbotti@xxxxxxxxx> || MEV Ltd. is a company  )=-
-=( registered in England & Wales.  Regd. number: 02862268.  )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-