| Message ID | 20220610213335.3077375-3-rhett.aultman@samsara.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | URB_FREE_COHERENT gs_usb memory leak fix | expand |
On 10.06.2022 17:33:37, Rhett Aultman 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 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 Better reference the commit or use a link to lore.kernel.org: 928150fad41b ("can: esd_usb2: fix memory leak") https://lore.kernel.org/all/b31b096926dcb35998ad0271aac4b51770ca7cc8.1627404470.git.paskripkin@gmail.com/ Marc
On Sun. 12 juin 2022 at 00:35, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > On 10.06.2022 17:33:37, Rhett Aultman 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 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 Hi Rhett, It seems that you missed one of my previous comments. Please check this: https://lore.kernel.org/linux-can/CAMZ6RqJ6fq=Rv-BuL6bA89E_DQdJ949quSWjyphy+2tOJJ=kGw@mail.gmail.com/ and add the Fixes tag. Thank you. > Better reference the commit or use a link to lore.kernel.org: > > 928150fad41b ("can: esd_usb2: fix memory leak") > https://lore.kernel.org/all/b31b096926dcb35998ad0271aac4b51770ca7cc8.1627404470.git.paskripkin@gmail.com/ > > 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 |
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@samsara.com> > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> > --- > 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? David > > usb_anchor_urb(urb, &parent->rx_submitted); > > -- > 2.30.2 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
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; usb_anchor_urb(urb, &parent->rx_submitted);