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

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

On Sat. 4 juin 2022 at 23:08, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> 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.

For the background, I also had a lot of trouble understanding the
logic of how to usb_free_coherent() URBs when I wrote my own driver
(etas_es58x). At the end, I just mimicked what other USB CAN drivers
were doing which means that the etas_es58x was most certainly also
contaminated by that malady.

I really appreciate your effort to also want to fix others' drivers.

> 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 hope so. Sometimes, there are some complicated intricacies which are
hard to appreciate (and I am not an expert of the USB subsystem). This
is why I CCed the USB mailing list. If I am wrong, someone should
eventually shout at me.

> 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.

I sent you a patch. You can build on top of it the fixes for the other drivers.
And do not hesitate to resend it as part of your series (and in that
particular case, you would need to explicitly add the From: tag).


Yours sincerely,
Vincent Mailhol