Re: [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12.06.2022 21:28:06, David Laight wrote:
> From: Rhett Aultman
> > Sent: 10 June 2022 22:34
> > 
> > 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 must be freed using usb_free_coherent() instead.  This means that
> > 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@xxxxxxxxxxx>
> > Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > ---
> >  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..4e7e6a7b961a 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;
> 
> Should that be clearing any other flags?

This chance is intentional, Vincent suggested that in

| https://lore.kernel.org/all/CAMZ6RqKwvSswxThiKqEB8VhD5MyHvRbSwO_9-ZNwLgmnm-0iBw@xxxxxxxxxxxxxx

I think there are no other flags set, yet.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux