[v2,0/3] URB_FREE_COHERENT gs_usb memory leak fix

This patchset is a v2 replacement for an earlier patch to resolve a
memory leak which can occur with successive iterations of calling
gs_can_open() and gs_can_close().  The central cause of this memory
leak, which is an issue common to many of the USB CAN drivers, is that
memory allocated for RX buffers using usb_alloc_coherent() and then kept
in the URB will be properly freed when the URB is killed.  This
assumption is incorrect, as memory allocated with usb_alloc_coherent()
must be freed with usb_free_coherent(), and there is no provision for this
in the existing URB code.

The common solution to this, found in v1 of my patches as well as in
already merged patches for other CAN USB drivers (see the patch for the
esd CAN-USB/2 driver here:
https://www.spinics.net/lists/linux-can/msg08203.html) is for the driver
itself to maintain an array of addresses of the buffers allocated with
usb_alloc_coherent() and to then iteratively call usb_free_coherent() on
them in their close function.  This solution requires a driver developer
to understand this unclear nuance, and it has historically been solved
in a piecemeal way one driver at a time (note: the gs_usb driver has had
this issue since the 3.x.x kernel series).

Rather than continue to place the burden of complexity on the drivers,
this patchset adds a new URB flag which allows the DMA buffer to be
correctly freed with the URB is killed.  This results in a much simpler
solution at the driver level and with minimal additional code in the USB
core.

Rhett Aultman (3):
  drivers: usb/core/urb: Add URB_FREE_COHERENT
  drivers: usb/core/urb: allow URB_FREE_COHERENT
  can: gs_usb: fix DMA memory leak on close

 drivers/net/can/usb/gs_usb.c | 2 +-
 drivers/usb/core/urb.c       | 5 ++++-
 include/linux/usb.h          | 3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

On Fri. 10 juin 2022 at 06:13, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> The URB_FREE_COHERENT flag needs to be added to the allowed flags set in
> order to prevent a "bogus flags" warning when it is used.
>
> Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> ---
>  drivers/usb/core/urb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 1460fdac0b18..36c48fb196e0 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -507,7 +507,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>
>         /* Check against a simple/standard policy */
>         allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
> -                       URB_FREE_BUFFER);
> +                       URB_FREE_BUFFER | URB_FREE_COHERENT);
>         switch (xfertype) {
>         case USB_ENDPOINT_XFER_BULK:
>         case USB_ENDPOINT_XFER_INT:

Thanks for the testing and thanks for fixing this warning.
I do not think this needs to be a separate patch. You can squash it to
my patch and add Co-developed-by/Signed-off-by tags to reflect your
work.

Yours sincerely,
Vincent Mailhol

On Fri. 10 Jun 2022 at 05:53, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> The gs_usb driver allocates DMA memory with usb_alloc_coherent() in
> gs_can_open() and then keeps this memory in an URB, with the expectation
> that the memory will be freed when the URB is killed in gs_can_close().
> Memory allocated with usb_alloc_coherent() cannot be freed in this way
> and much be freed using usb_free_coherent() instead.  This means that
      ^^^^
and must

> repeated cycles of calling gs_can_open() and gs_can_close() will lead to
> a memory leak.
>
> Historically, drivers have handled this by keeping an array of pointers
> to their DMA rx buffers and explicitly freeing them.  For an example of
> this technique used in the esd_usb2 driver, see here:
> https://www.spinics.net/lists/linux-can/msg08203.html
>
> While the above method works, the conditions that cause this leak are
> not apparent to driver writers and the method for solving it at the
> driver level has been piecemeal.  This patch makes use of a new
> URB_FREE_COHERENT flag on the URB, reducing the solution of this memory
> leak down to a single line of code.
>
> Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> ---
>  drivers/net/can/usb/gs_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index b29ba9138866..3ded3e14c830 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
> @@ -768,7 +768,7 @@ static int gs_can_open(struct net_device *netdev)
>                                           buf,
>                                           dev->parent->hf_size_rx,
>                                           gs_usb_receive_bulk_callback, parent);
>
> -                       urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> +                       urb->transfer_flags |= (URB_NO_TRANSFER_DMA_MAP | URB_FREE_COHERENT);

Nitpick but parenthesis are not needed here. Also, there are no
reasons to do a |=. I would prefer to see this:
        urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_FREE_COHERENT;

>                         usb_anchor_urb(urb, &parent->rx_submitted);

I looked at the code of gs_usb, this driver has a lot of dust. What
strikes me the most is that it uses usb_alloc_coherent() each time in
its xmit() function instead of allocating the TX URB once in the
open() function and resubmitting then in the callback function. That
last comment is not a criticism of this patch. But if someone has the
motivation to do some cleaning, go ahead…

Aside from the nitpicks, the patch looks good to me. You can add this
to your v3:
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>


Yours sincerely,
Vincent Mailhol

On Fri. 10 juin 2022 at 09:05, Vincent Mailhol
<vincent.mailhol@gmail.com> wrote:
> On Fri. 10 Jun 2022 at 05:53, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> > The gs_usb driver allocates DMA memory with usb_alloc_coherent() in
> > gs_can_open() and then keeps this memory in an URB, with the expectation
> > that the memory will be freed when the URB is killed in gs_can_close().
> > Memory allocated with usb_alloc_coherent() cannot be freed in this way
> > and much be freed using usb_free_coherent() instead.  This means that
>       ^^^^
> and must
>
> > repeated cycles of calling gs_can_open() and gs_can_close() will lead to
> > a memory leak.
> >
> > Historically, drivers have handled this by keeping an array of pointers
> > to their DMA rx buffers and explicitly freeing them.  For an example of
> > this technique used in the esd_usb2 driver, see here:
> > https://www.spinics.net/lists/linux-can/msg08203.html
> >
> > While the above method works, the conditions that cause this leak are
> > not apparent to driver writers and the method for solving it at the
> > driver level has been piecemeal.  This patch makes use of a new
> > URB_FREE_COHERENT flag on the URB, reducing the solution of this memory
> > leak down to a single line of code.
> >

One more thing, for bug fixes, the best practice is to add a Fixes tag. c.f.:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")

> > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> > ---
> >  drivers/net/can/usb/gs_usb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> > index b29ba9138866..3ded3e14c830 100644
> > --- a/drivers/net/can/usb/gs_usb.c
> > +++ b/drivers/net/can/usb/gs_usb.c
> > @@ -768,7 +768,7 @@ static int gs_can_open(struct net_device *netdev)
> >                                           buf,
> >                                           dev->parent->hf_size_rx,
> >                                           gs_usb_receive_bulk_callback, parent);
> >
> > -                       urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> > +                       urb->transfer_flags |= (URB_NO_TRANSFER_DMA_MAP | URB_FREE_COHERENT);
>
> Nitpick but parenthesis are not needed here. Also, there are no
> reasons to do a |=. I would prefer to see this:
>         urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_FREE_COHERENT;
>
> >                         usb_anchor_urb(urb, &parent->rx_submitted);
>
> I looked at the code of gs_usb, this driver has a lot of dust. What
> strikes me the most is that it uses usb_alloc_coherent() each time in
> its xmit() function instead of allocating the TX URB once in the
> open() function and resubmitting then in the callback function. That
> last comment is not a criticism of this patch. But if someone has the
> motivation to do some cleaning, go ahead…
>
> Aside from the nitpicks, the patch looks good to me. You can add this
> to your v3:
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
>
> Yours sincerely,
> Vincent Mailhol