Messages in this thread | | | Subject | Re: [PATCH] comedi: usbdux: free allocated memory/urbs on failure | From | Ian Abbott <> | Date | Thu, 15 Jul 2021 12:34:25 +0100 |
| |
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@gmail.com> > --- > 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@mev.co.uk> || 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 )=-
| |