Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak

[Rhett Aultman <rhett.aultman@samsara.com>]

On Sat, 4 Jun 2022, Vincent MAILHOL wrote:

> > For me, the natural fix would be:
> >
> > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> > index 33d62d7e3929..1460fdac0b18 100644
> > --- a/drivers/usb/core/urb.c
> > +++ b/drivers/usb/core/urb.c
> > @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
> >
> >         if (urb->transfer_flags & URB_FREE_BUFFER)
> >                 kfree(urb->transfer_buffer);
> > +       else if (urb->transfer_flags & URB_FREE_COHERENT)
> > +               usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> > +                                 urb->transfer_buffer, urb->transfer_dma);
> >
> >         kfree(urb);
> >  }
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 200b7b79acb5..dfc348d56fed 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
> >  #define URB_NO_INTERRUPT       0x0080  /* HINT: no non-error interrupt
> >                                          * needed */
> >  #define URB_FREE_BUFFER                0x0100  /* Free transfer buffer with the URB */
> > +#define URB_FREE_COHERENT      0x0200  /* Free DMA memory of transfer buffer */
>
> #define URB_FREE_COHERENT      0x0400  /* Free DMA memory of transfer buffer */
>
> Obviously, the value 0x0200 is already taken by URB_DIR_IN just below.
> So 0x0400 seems more adequate.
> Sorry for the noise.

Now that you've pointed this out, I agree this appears to be the more
natural way to handle things.  I will take a point of making a rewrite
here.  Additionally, I'll do my best to bring all the other USB CAN
drivers in line with this.  My only concern in doing that is that I have
gs_usb devices to confirm the fix with but not devices for the other
drivers.

> > Maybe I missed something obvious, but if so, I would like to
> > understand what is wrong in above approach.

I don't think anything is wrong with the above approach.  I just happened
to see an issue in the gs_usb driver that was already fixed in the other
USB CAN drivers, and adapted the fix used in the other drivers to the
gs_usb one.  Doing so got your attention and you pointed out a more
general fix, so I agree with going this route.  I'll post a new patch this
week.

Best,
Rhett