All of lore.kernel.org
 help / color / mirror / Atom feed
From: Rhett Aultman <rhett.aultman@samsara.com>
To: wg@grandegger.com, mkl@pengutronix.de, linux-can@vger.kernel.org
Subject: [PATCH 001/001] can: gs_usb: gs_usb_open/close( ) fix memory leak
Date: Fri, 3 Jun 2022 15:52:00 -0400 (EDT)	[thread overview]
Message-ID: <alpine.DEB.2.22.394.2206031547001.1630869@thelappy> (raw)

From 25f7bada85d9977daf63f95065f5bc57d3bc37de Mon Sep 17 00:00:00 2001
From: Rhett Aultman <rhett.aultman@samsara.com>
Date: Mon, 2 May 2022 13:04:26 -0400
Subject: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak

The gs_usb driver appears to suffer from a malady common to many USB CAN
adapter drivers in that it performs usb_alloc_coherent( ) to allocate a
number of USB request blocks (URBs) for RX, and then later relies on
usb_kill_anchored_urbs( ) to free them, but this doesn't actually free
them.  As a result, this may be leaking DMA memory that's been used by the
driver.

This commit is an adaptation of the techniques found in the esd_usb2 driver
where a similar design pattern led to a memory leak.  It explicitly frees
the RX URBs and their DMA memory via a call to usb_free_coherent( ).  Since
the RX URBs were allocated in the gs_can_open( ), we remove them in
gs_can_close( ) rather than in the disconnect function as was done in
esd_usb2.

For more information, see the following:
* https://www.spinics.net/lists/linux-can/msg08203.html
* https://github.com/torvalds/linux
    928150fad41 (can: esd_usb2: fix memory leak)

From: Rhett Aultman <rhett.aultman@samsara.com>
Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
---
 drivers/net/can/usb/gs_usb.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index b29ba9138866..d3a658b444b5 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -268,6 +268,8 @@ struct gs_can {

 	struct usb_anchor tx_submitted;
 	atomic_t active_tx_urbs;
+	void *rxbuf[GS_MAX_RX_URBS];
+	dma_addr_t rxbuf_dma[GS_MAX_RX_URBS];
 };

 /* usb interface struct */
@@ -742,6 +744,7 @@ static int gs_can_open(struct net_device *netdev)
 		for (i = 0; i < GS_MAX_RX_URBS; i++) {
 			struct urb *urb;
 			u8 *buf;
+			dma_addr_t buf_dma;

 			/* alloc rx urb */
 			urb = usb_alloc_urb(0, GFP_KERNEL);
@@ -752,7 +755,7 @@ static int gs_can_open(struct net_device *netdev)
 			buf = usb_alloc_coherent(dev->udev,
 						 dev->parent->hf_size_rx,
 						 GFP_KERNEL,
-						 &urb->transfer_dma);
+						 &buf_dma);
 			if (!buf) {
 				netdev_err(netdev,
 					   "No memory left for USB buffer\n");
@@ -760,6 +763,8 @@ static int gs_can_open(struct net_device *netdev)
 				return -ENOMEM;
 			}

+			urb->transfer_dma = buf_dma;
+
 			/* fill, anchor, and submit rx urb */
 			usb_fill_bulk_urb(urb,
 					  dev->udev,
@@ -781,10 +786,17 @@ static int gs_can_open(struct net_device *netdev)
 					   "usb_submit failed (err=%d)\n", rc);

 				usb_unanchor_urb(urb);
+				usb_free_coherent(dev->udev,
+						  sizeof(struct gs_host_frame),
+						  buf,
+						  buf_dma);
 				usb_free_urb(urb);
 				break;
 			}

+			dev->rxbuf[i] = buf;
+			dev->rxbuf_dma[i] = buf_dma;
+
 			/* Drop reference,
 			 * USB core will take care of freeing it
 			 */
@@ -842,13 +854,20 @@ static int gs_can_close(struct net_device *netdev)
 	int rc;
 	struct gs_can *dev = netdev_priv(netdev);
 	struct gs_usb *parent = dev->parent;
+	unsigned int i;

 	netif_stop_queue(netdev);

 	/* Stop polling */
 	parent->active_channels--;
-	if (!parent->active_channels)
+	if (!parent->active_channels) {
 		usb_kill_anchored_urbs(&parent->rx_submitted);
+		for (i = 0; i < GS_MAX_RX_URBS; i++)
+			usb_free_coherent(dev->udev,
+					  sizeof(struct gs_host_frame),
+					  dev->rxbuf[i],
+					  dev->rxbuf_dma[i]);
+	}

 	/* Stop sending URBs */
 	usb_kill_anchored_urbs(&dev->tx_submitted);
-- 
2.30.2


             reply	other threads:[~2022-06-03 20:22 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-03 19:52 Rhett Aultman [this message]
2022-06-04  2:11 ` [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak Vincent Mailhol
2022-06-04  2:26   ` Vincent MAILHOL
2022-06-04 14:08     ` Rhett Aultman
2022-06-04 14:41       ` [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT Vincent Mailhol
2022-06-04 16:40         ` Alan Stern
2022-06-05  2:04           ` Vincent MAILHOL
2022-06-05  6:00           ` Oliver Neukum
2022-06-05 13:45             ` Vincent MAILHOL
2022-06-07  9:49               ` Oliver Neukum
2022-06-07 10:18                 ` Vincent MAILHOL
2022-06-07 11:46                   ` Oliver Neukum
2022-06-07 12:12                     ` Vincent MAILHOL
2022-06-05  2:15         ` [RFC PATCH v2] usb: " Vincent Mailhol
2022-06-04 14:53       ` [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak Vincent MAILHOL
2022-06-09 20:47 ` [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman
2022-06-09 20:47   ` [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman
2022-06-10  0:18     ` Vincent MAILHOL
2022-06-10 10:46       ` Marc Kleine-Budde
2022-06-09 20:47   ` [PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT Rhett Aultman
2022-06-09 23:18     ` Vincent Mailhol
2022-06-09 20:47   ` [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close Rhett Aultman
2022-06-10  0:05     ` Vincent Mailhol
2022-06-10  1:28       ` Vincent Mailhol
2022-06-10 21:33   ` [PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman
2022-06-10 21:33     ` [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman
2022-06-11 15:31       ` Marc Kleine-Budde
2022-06-11 16:06         ` Vincent MAILHOL
2022-06-21 13:51           ` Greg Kroah-Hartman
2022-06-21 14:59             ` Vincent MAILHOL
2022-06-21 15:03               ` Greg Kroah-Hartman
2022-06-21 15:54                 ` Vincent MAILHOL
2022-06-21 15:11               ` Alan Stern
2022-06-21 15:55                 ` Vincent MAILHOL
2022-06-21 16:14                   ` Alan Stern
2022-06-21 16:40                     ` Vincent MAILHOL
2022-06-21 17:00                       ` Greg Kroah-Hartman
2022-06-21 17:14                         ` Vincent MAILHOL
2022-06-21 17:46                           ` Greg Kroah-Hartman
2022-06-22  9:22                   ` David Laight
2022-06-22  9:41                     ` Greg Kroah-Hartman
2022-06-22 10:03                       ` David Laight
2022-06-22 11:11                         ` Oliver Neukum
2022-06-22 10:34                       ` Vincent MAILHOL
2022-06-22 12:23                         ` Greg Kroah-Hartman
2022-06-22 15:59                           ` Vincent MAILHOL
2022-06-22 18:11                             ` Rhett Aultman
2022-06-26  8:21                               ` Vincent MAILHOL
2022-06-27 19:24                                 ` Rhett Aultman
2022-06-28  1:09                                   ` Vincent MAILHOL
2022-07-04 13:02                                     ` Marc Kleine-Budde
2022-07-04 15:35                                       ` Rhett Aultman
2022-07-05  7:50                                         ` Marc Kleine-Budde
2022-06-23 17:30       ` Hongren Zenithal Zheng
2022-06-23 17:45         ` Shuah Khan
2022-06-24 14:43           ` Alan Stern
2022-06-24 16:01             ` Hongren Zenithal Zheng
2022-06-24 16:31             ` Shuah Khan
2022-06-24 18:07               ` Alan Stern
2022-06-27 22:54                 ` Shuah Khan
2022-06-28  1:35                   ` Alan Stern
2022-07-01  2:10                     ` Shuah Khan
2022-08-01 17:42                       ` Shuah Khan
2022-08-01 18:28                         ` Vincent MAILHOL
2022-08-03 23:44                           ` Shuah Khan
2022-06-10 21:33     ` [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close Rhett Aultman
2022-06-11 15:35       ` Marc Kleine-Budde
2022-06-11 16:03         ` Vincent MAILHOL
2022-06-12 21:28       ` David Laight
2022-06-12 21:33         ` Marc Kleine-Budde
2022-06-14 15:26     ` [PATCH v4 0/1] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman
2022-06-14 15:26       ` [PATCH v4 1/1] can: gs_usb: fix DMA memory leak on close Rhett Aultman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.DEB.2.22.394.2206031547001.1630869@thelappy \
    --to=rhett.aultman@samsara.com \
    --cc=linux-can@vger.kernel.org \
    --cc=mkl@pengutronix.de \
    --cc=wg@grandegger.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.