linux-can.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
@ 2021-07-25  7:42 Yasushi SHOJI
  2021-07-25  8:12 ` Pavel Skripkin
  0 siblings, 1 reply; 18+ messages in thread
From: Yasushi SHOJI @ 2021-07-25  7:42 UTC (permalink / raw)
  To: Pavel Skripkin; +Cc: linux-can

Hi Pavel,

Apologize for the late reply.

Since 6bd3d80d1f019cef, my Microchip CAN Analyzer stopped working,
more precisely I can't capture any data with it and repeated messages
from the driver flod the syslog. I usually use the Debian kernel image
and linux 5.10.46-2 migrated to unstable on July 20th.  I noticed my
device stopped working a few days later but didn't have time to
bisect.

Does your device work with the patch?
Does the patch work on the main line?

I've posted some report with my hardware configuration at debian mailing list:
https://bugs.debian.org/990850

Please let me know if you need any more information.

Best,
--
               yashi

^ permalink raw reply	[flat|nested] 18+ messages in thread
* [PATCH] can: mcba_usb: fix memory leak in mcba_usb
@ 2021-06-09 21:58 Pavel Skripkin
  2021-06-15  7:33 ` Marc Kleine-Budde
  0 siblings, 1 reply; 18+ messages in thread
From: Pavel Skripkin @ 2021-06-09 21:58 UTC (permalink / raw)
  To: wg, mkl, davem
  Cc: linux-can, netdev, linux-kernel, Pavel Skripkin,
	syzbot+57281c762a3922e14dfe

Syzbot reported memory leak in SocketCAN driver
for Microchip CAN BUS Analyzer Tool. The problem
was in unfreed usb_coherent.

In mcba_usb_start() 20 coherent buffers are allocated
and there is nothing, that frees them:

	1) In callback function the urb is resubmitted and that's all
	2) In disconnect function urbs are simply killed, but
	   URB_FREE_BUFFER is not set (see mcba_usb_start)
           and this flag cannot be used with coherent buffers.

Fail log:
[ 1354.053291][ T8413] mcba_usb 1-1:0.0 can0: device disconnected
[ 1367.059384][ T8420] kmemleak: 20 new suspected memory leaks (see /sys/kernel/debug/kmem)

So, all allocated buffers should be freed with usb_free_coherent()
explicitly

NOTE:
The same pattern for allocating and freeing coherent buffers
is used in drivers/net/can/usb/kvaser_usb/kvaser_usb_core.c

Fixes: 51f3baad7de9 ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer")
Reported-and-tested-by: syzbot+57281c762a3922e14dfe@syzkaller.appspotmail.com
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/net/can/usb/mcba_usb.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index 029e77dfa773..a45865bd7254 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -82,6 +82,8 @@ struct mcba_priv {
 	bool can_ka_first_pass;
 	bool can_speed_check;
 	atomic_t free_ctx_cnt;
+	void *rxbuf[MCBA_MAX_RX_URBS];
+	dma_addr_t rxbuf_dma[MCBA_MAX_RX_URBS];
 };
 
 /* CAN frame */
@@ -633,6 +635,7 @@ static int mcba_usb_start(struct mcba_priv *priv)
 	for (i = 0; i < MCBA_MAX_RX_URBS; i++) {
 		struct urb *urb = NULL;
 		u8 *buf;
+		dma_addr_t buf_dma;
 
 		/* create a URB, and a buffer for it */
 		urb = usb_alloc_urb(0, GFP_KERNEL);
@@ -642,7 +645,7 @@ static int mcba_usb_start(struct mcba_priv *priv)
 		}
 
 		buf = usb_alloc_coherent(priv->udev, MCBA_USB_RX_BUFF_SIZE,
-					 GFP_KERNEL, &urb->transfer_dma);
+					 GFP_KERNEL, &buf_dma);
 		if (!buf) {
 			netdev_err(netdev, "No memory left for USB buffer\n");
 			usb_free_urb(urb);
@@ -661,11 +664,14 @@ static int mcba_usb_start(struct mcba_priv *priv)
 		if (err) {
 			usb_unanchor_urb(urb);
 			usb_free_coherent(priv->udev, MCBA_USB_RX_BUFF_SIZE,
-					  buf, urb->transfer_dma);
+					  buf, buf_dma);
 			usb_free_urb(urb);
 			break;
 		}
 
+		priv->rxbuf[i] = buf;
+		priv->rxbuf_dma[i] = buf_dma;
+
 		/* Drop reference, USB core will take care of freeing it */
 		usb_free_urb(urb);
 	}
@@ -708,7 +714,14 @@ static int mcba_usb_open(struct net_device *netdev)
 
 static void mcba_urb_unlink(struct mcba_priv *priv)
 {
+	int i;
+
 	usb_kill_anchored_urbs(&priv->rx_submitted);
+
+	for (i = 0; i < MCBA_MAX_RX_URBS; ++i)
+		usb_free_coherent(priv->udev, MCBA_USB_RX_BUFF_SIZE,
+				  priv->rxbuf[i], priv->rxbuf_dma[i]);
+
 	usb_kill_anchored_urbs(&priv->tx_submitted);
 }
 
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-07-27  2:19 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-25  7:42 [PATCH] can: mcba_usb: fix memory leak in mcba_usb Yasushi SHOJI
2021-07-25  8:12 ` Pavel Skripkin
2021-07-25  9:42   ` Marc Kleine-Budde
2021-07-25 10:18     ` Pavel Skripkin
2021-07-25 10:36     ` [PATCH] net: can: add missing urb->transfer_dma initialization Pavel Skripkin
2021-07-25 13:27       ` Yasushi SHOJI
2021-07-25 16:30         ` Marc Kleine-Budde
2021-07-25 10:44   ` [PATCH] can: mcba_usb: fix memory leak in mcba_usb Yasushi SHOJI
2021-07-25 16:27     ` Marc Kleine-Budde
2021-07-25 16:35       ` Yasushi SHOJI
2021-07-26  9:31         ` Marc Kleine-Budde
2021-07-26  9:34           ` Marc Kleine-Budde
2021-07-26 10:43             ` Yasushi SHOJI
2021-07-26 10:42           ` Yasushi SHOJI
2021-07-26 11:17             ` Marc Kleine-Budde
2021-07-27  2:19               ` Yasushi SHOJI
  -- strict thread matches above, loose matches on Subject: below --
2021-06-09 21:58 Pavel Skripkin
2021-06-15  7:33 ` Marc Kleine-Budde

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).