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

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  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:44   ` [PATCH] can: mcba_usb: fix memory leak in mcba_usb Yasushi SHOJI
  0 siblings, 2 replies; 18+ messages in thread
From: Pavel Skripkin @ 2021-07-25  8:12 UTC (permalink / raw)
  To: Yasushi SHOJI; +Cc: linux-can, mkl

On Sun, 25 Jul 2021 16:42:49 +0900
Yasushi SHOJI <yasushi.shoji@gmail.com> wrote:

> Hi Pavel,
> 

Hi, Yasushi!

> 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 don't have this device, I just fixed this syzbot bug report:
https://syzkaller.appspot.com/bug?id=c94c1c23e829d5ac97995d51219f0c5a0cd1fa54.

I think, I found the root case. In this patch I saved dma_buff to local
variable and then to array, but forgot to assign it to
urb->transfer_buf. This log

[   33.862175] DMAR: [DMA Write] Request device [00:14.0] PASID ffffffff fault addr 0 [fault reason 05] PTE Write access is not set

points exactly to this problem.


Can You try the following patch?

diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index a45865bd7254..a1a154c08b7f 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -653,6 +653,8 @@ static int mcba_usb_start(struct mcba_priv *priv)
 			break;
 		}
 
+		urb->transfer_dma = buf_dma;
+
 		usb_fill_bulk_urb(urb, priv->udev,
 				  usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN),
 				  buf, MCBA_USB_RX_BUFF_SIZE,



I've added Marc to this discussion, I believe, he can help us, since he
is CAN maintainer.


I am sorry for breaking your device :(

> 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





With regards,
Pavel Skripkin



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  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 10:44   ` [PATCH] can: mcba_usb: fix memory leak in mcba_usb Yasushi SHOJI
  1 sibling, 2 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2021-07-25  9:42 UTC (permalink / raw)
  To: Pavel Skripkin; +Cc: Yasushi SHOJI, linux-can

[-- Attachment #1: Type: text/plain, Size: 1239 bytes --]

On 25.07.2021 11:12:42, Pavel Skripkin wrote:
> Can You try the following patch?

Can you create a proper patch with you S-o-b?

> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> index a45865bd7254..a1a154c08b7f 100644
> --- a/drivers/net/can/usb/mcba_usb.c
> +++ b/drivers/net/can/usb/mcba_usb.c
> @@ -653,6 +653,8 @@ static int mcba_usb_start(struct mcba_priv *priv)
>  			break;
>  		}
>  
> +		urb->transfer_dma = buf_dma;
> +
>  		usb_fill_bulk_urb(urb, priv->udev,
>  				  usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN),
>  				  buf, MCBA_USB_RX_BUFF_SIZE,
> 
> 
> 
> I've added Marc to this discussion, I believe, he can help us, since he
> is CAN maintainer.

Yasushi, please test and post your Tested-by here. After Pavel posts a
proper patch and you tested it, I'll forward it to net/master, then it
will be applied to the stable kernels. Debian can even pick up the patch
earlier.

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  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
  1 sibling, 0 replies; 18+ messages in thread
From: Pavel Skripkin @ 2021-07-25 10:18 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Yasushi SHOJI, linux-can

On Sun, 25 Jul 2021 11:42:46 +0200
Marc Kleine-Budde <mkl@pengutronix.de> wrote:

> On 25.07.2021 11:12:42, Pavel Skripkin wrote:
> > Can You try the following patch?
> 
> Can you create a proper patch with you S-o-b?
> 
Hi, Marc.

Sure! I thought about a bit diffrent fix path like:
diff -> Yasushi's test -> patch with Tested-by tag. I guess, your
approach is more standard, so, I will send pacth as reply to your email.

Thanks!

> > diff --git a/drivers/net/can/usb/mcba_usb.c
> > b/drivers/net/can/usb/mcba_usb.c index a45865bd7254..a1a154c08b7f
> > 100644 --- a/drivers/net/can/usb/mcba_usb.c
> > +++ b/drivers/net/can/usb/mcba_usb.c
> > @@ -653,6 +653,8 @@ static int mcba_usb_start(struct mcba_priv
> > *priv) break;
> >  		}
> >  
> > +		urb->transfer_dma = buf_dma;
> > +
> >  		usb_fill_bulk_urb(urb, priv->udev,
> >  				  usb_rcvbulkpipe(priv->udev,
> > MCBA_USB_EP_IN), buf, MCBA_USB_RX_BUFF_SIZE,
> > 
> > 
> > 
> > I've added Marc to this discussion, I believe, he can help us,
> > since he is CAN maintainer.
> 
> Yasushi, please test and post your Tested-by here. After Pavel posts a
> proper patch and you tested it, I'll forward it to net/master, then it
> will be applied to the stable kernels. Debian can even pick up the
> patch earlier.
> 
> regards,
> Marc
> 



With regards,
Pavel Skripkin

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH] net: can: add missing urb->transfer_dma initialization
  2021-07-25  9:42   ` Marc Kleine-Budde
  2021-07-25 10:18     ` Pavel Skripkin
@ 2021-07-25 10:36     ` Pavel Skripkin
  2021-07-25 13:27       ` Yasushi SHOJI
  1 sibling, 1 reply; 18+ messages in thread
From: Pavel Skripkin @ 2021-07-25 10:36 UTC (permalink / raw)
  To: mkl, yasushi.shoji, wg; +Cc: linux-can, linux-kernel, netdev, Pavel Skripkin

Yasushi reported, that his Microchip CAN Analyzer stopped working since
commit 91c02557174b ("can: mcba_usb: fix memory leak in mcba_usb").
The problem was in missing urb->transfer_dma initialization.

In my previous patch to this driver I refactored mcba_usb_start() code to
avoid leaking usb coherent buffers. To achive it, I passed local stack
variable to usb_alloc_coherent() and then saved it to private array to
correctly free all coherent buffers on ->close() call. But I forgot to
inialize urb->transfer_dma with variable passed to usb_alloc_coherent().

All of this was causing device to not work, since dma addr 0 is not valid
and following log can be found on bug report page, which points exactly to
problem described above.

[   33.862175] DMAR: [DMA Write] Request device [00:14.0] PASID ffffffff fault addr 0 [fault reason 05] PTE Write access is not set

Bug report: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990850

Reported-by: Yasushi SHOJI <yasushi.shoji@gmail.com>
Fixes: 91c02557174b ("can: mcba_usb: fix memory leak in mcba_usb")
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/net/can/usb/mcba_usb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
index a45865bd7254..a1a154c08b7f 100644
--- a/drivers/net/can/usb/mcba_usb.c
+++ b/drivers/net/can/usb/mcba_usb.c
@@ -653,6 +653,8 @@ static int mcba_usb_start(struct mcba_priv *priv)
 			break;
 		}
 
+		urb->transfer_dma = buf_dma;
+
 		usb_fill_bulk_urb(urb, priv->udev,
 				  usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN),
 				  buf, MCBA_USB_RX_BUFF_SIZE,
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  2021-07-25  8:12 ` Pavel Skripkin
  2021-07-25  9:42   ` Marc Kleine-Budde
@ 2021-07-25 10:44   ` Yasushi SHOJI
  2021-07-25 16:27     ` Marc Kleine-Budde
  1 sibling, 1 reply; 18+ messages in thread
From: Yasushi SHOJI @ 2021-07-25 10:44 UTC (permalink / raw)
  To: Pavel Skripkin; +Cc: linux-can, mkl, Yasushi SHOJI

Hi Pavel,

On Sun, Jul 25, 2021 at 5:12 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
> Can You try the following patch?
>
> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> index a45865bd7254..a1a154c08b7f 100644
> --- a/drivers/net/can/usb/mcba_usb.c
> +++ b/drivers/net/can/usb/mcba_usb.c
> @@ -653,6 +653,8 @@ static int mcba_usb_start(struct mcba_priv *priv)
>                         break;
>                 }
>
> +               urb->transfer_dma = buf_dma;
> +
>                 usb_fill_bulk_urb(urb, priv->udev,
>                                   usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN),
>                                   buf, MCBA_USB_RX_BUFF_SIZE,

Yup, this patch fixed it.  I've tested on top of v5.10.52.

Tested-by: Yasushi SHOJI <yashi@spacecubics.com>

> I am sorry for breaking your device :(

No problem.  It'd be nice if we'd test with real hardware before
merging into the stable tree, though.
Anyway, thank you for your quick fix!

Best regards,
--
               yashi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: can: add missing urb->transfer_dma initialization
  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
  0 siblings, 1 reply; 18+ messages in thread
From: Yasushi SHOJI @ 2021-07-25 13:27 UTC (permalink / raw)
  To: Pavel Skripkin; +Cc: mkl, wg, linux-can, linux-kernel, netdev, Yasushi SHOJI

Hi Pavel,

I've tested this patch on top of v5.14-rc2.  All good.

Tested-by: Yasushi SHOJI <yashi@spacecubics.com>

Some nitpicks.

On Sun, Jul 25, 2021 at 7:36 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
>
> Yasushi reported, that his Microchip CAN Analyzer stopped working since
> commit 91c02557174b ("can: mcba_usb: fix memory leak in mcba_usb").
> The problem was in missing urb->transfer_dma initialization.
>
> In my previous patch to this driver I refactored mcba_usb_start() code to
> avoid leaking usb coherent buffers. To achive it, I passed local stack

achieve

> variable to usb_alloc_coherent() and then saved it to private array to
> correctly free all coherent buffers on ->close() call. But I forgot to
> inialize urb->transfer_dma with variable passed to usb_alloc_coherent().

initialize

> All of this was causing device to not work, since dma addr 0 is not valid
> and following log can be found on bug report page, which points exactly to
> problem described above.
>
> [   33.862175] DMAR: [DMA Write] Request device [00:14.0] PASID ffffffff fault addr 0 [fault reason 05] PTE Write access is not set
>
> Bug report: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990850
>
> Reported-by: Yasushi SHOJI <yasushi.shoji@gmail.com>
> Fixes: 91c02557174b ("can: mcba_usb: fix memory leak in mcba_usb")
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  drivers/net/can/usb/mcba_usb.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> index a45865bd7254..a1a154c08b7f 100644
> --- a/drivers/net/can/usb/mcba_usb.c
> +++ b/drivers/net/can/usb/mcba_usb.c
> @@ -653,6 +653,8 @@ static int mcba_usb_start(struct mcba_priv *priv)
>                         break;
>                 }
>
> +               urb->transfer_dma = buf_dma;
> +
>                 usb_fill_bulk_urb(urb, priv->udev,
>                                   usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN),
>                                   buf, MCBA_USB_RX_BUFF_SIZE,
> --
> 2.32.0

Pavel, thanks again for your quick fix. :-)

Best,
--
               yashi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  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
  0 siblings, 1 reply; 18+ messages in thread
From: Marc Kleine-Budde @ 2021-07-25 16:27 UTC (permalink / raw)
  To: Yasushi SHOJI; +Cc: Pavel Skripkin, linux-can, Yasushi SHOJI

[-- Attachment #1: Type: text/plain, Size: 1669 bytes --]

On 25.07.2021 19:44:43, Yasushi SHOJI wrote:
> Hi Pavel,
> 
> On Sun, Jul 25, 2021 at 5:12 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
> > Can You try the following patch?
> >
> > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c
> > index a45865bd7254..a1a154c08b7f 100644
> > --- a/drivers/net/can/usb/mcba_usb.c
> > +++ b/drivers/net/can/usb/mcba_usb.c
> > @@ -653,6 +653,8 @@ static int mcba_usb_start(struct mcba_priv *priv)
> >                         break;
> >                 }
> >
> > +               urb->transfer_dma = buf_dma;
> > +
> >                 usb_fill_bulk_urb(urb, priv->udev,
> >                                   usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN),
> >                                   buf, MCBA_USB_RX_BUFF_SIZE,
> 
> Yup, this patch fixed it.  I've tested on top of v5.10.52.
> 
> Tested-by: Yasushi SHOJI <yashi@spacecubics.com>
> 
> > I am sorry for breaking your device :(
> 
> No problem.  It'd be nice if we'd test with real hardware before
> merging into the stable tree, though.

Yes absolutely - but I don't have access to that hardware. Since you're
interested in that hardware, what about adding you as a reviewer to the
kernel. Then for every change on this driver, you'll be added on Cc and
can test it. What do you think?

> Anyway, thank you for your quick fix!

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] net: can: add missing urb->transfer_dma initialization
  2021-07-25 13:27       ` Yasushi SHOJI
@ 2021-07-25 16:30         ` Marc Kleine-Budde
  0 siblings, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2021-07-25 16:30 UTC (permalink / raw)
  To: Yasushi SHOJI
  Cc: Pavel Skripkin, wg, linux-can, linux-kernel, netdev, Yasushi SHOJI

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

On 25.07.2021 22:27:37, Yasushi SHOJI wrote:
> Hi Pavel,
> 
> I've tested this patch on top of v5.14-rc2.  All good.
> 
> Tested-by: Yasushi SHOJI <yashi@spacecubics.com>
> 
> Some nitpicks.
> 
> On Sun, Jul 25, 2021 at 7:36 PM Pavel Skripkin <paskripkin@gmail.com> wrote:
> >
> > Yasushi reported, that his Microchip CAN Analyzer stopped working since
> > commit 91c02557174b ("can: mcba_usb: fix memory leak in mcba_usb").
> > The problem was in missing urb->transfer_dma initialization.
> >
> > In my previous patch to this driver I refactored mcba_usb_start() code to
> > avoid leaking usb coherent buffers. To achive it, I passed local stack
> 
> achieve
> 
> > variable to usb_alloc_coherent() and then saved it to private array to
> > correctly free all coherent buffers on ->close() call. But I forgot to
> > inialize urb->transfer_dma with variable passed to usb_alloc_coherent().
> 
> initialize

Fixed while applying.

Thanks,
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  2021-07-25 16:27     ` Marc Kleine-Budde
@ 2021-07-25 16:35       ` Yasushi SHOJI
  2021-07-26  9:31         ` Marc Kleine-Budde
  0 siblings, 1 reply; 18+ messages in thread
From: Yasushi SHOJI @ 2021-07-25 16:35 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Pavel Skripkin, linux-can, Yasushi SHOJI

Hi,

On Mon, Jul 26, 2021 at 1:27 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Yes absolutely - but I don't have access to that hardware. Since you're
> interested in that hardware, what about adding you as a reviewer to the
> kernel. Then for every change on this driver, you'll be added on Cc and
> can test it. What do you think?

Sure, I'm happy to help.
-- 
               yashi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  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:42           ` Yasushi SHOJI
  0 siblings, 2 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2021-07-26  9:31 UTC (permalink / raw)
  To: Yasushi SHOJI; +Cc: Pavel Skripkin, linux-can, Yasushi SHOJI

[-- Attachment #1: Type: text/plain, Size: 799 bytes --]

On 26.07.2021 01:35:53, Yasushi SHOJI wrote:
> On Mon, Jul 26, 2021 at 1:27 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > Yes absolutely - but I don't have access to that hardware. Since you're
> > interested in that hardware, what about adding you as a reviewer to the
> > kernel. Then for every change on this driver, you'll be added on Cc and
> > can test it. What do you think?
> 
> Sure, I'm happy to help.

Great! I've send an update to the MAINTAINERs file, Can you reply with
your "Acked-by:"?

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  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
  1 sibling, 1 reply; 18+ messages in thread
From: Marc Kleine-Budde @ 2021-07-26  9:34 UTC (permalink / raw)
  To: Yasushi SHOJI; +Cc: Pavel Skripkin, linux-can, Yasushi SHOJI

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

On 26.07.2021 11:31:08, Marc Kleine-Budde wrote:
> On 26.07.2021 01:35:53, Yasushi SHOJI wrote:
> > On Mon, Jul 26, 2021 at 1:27 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > Yes absolutely - but I don't have access to that hardware. Since you're
> > > interested in that hardware, what about adding you as a reviewer to the
> > > kernel. Then for every change on this driver, you'll be added on Cc and
> > > can test it. What do you think?
> > 
> > Sure, I'm happy to help.
> 
> Great! I've send an update to the MAINTAINERs file, Can you reply with
> your "Acked-by:"?

BTW: is this the device we're talking about?

https://www.microchip.com/Developmenttools/ProductDetails/APGDT002

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  2021-07-26  9:31         ` Marc Kleine-Budde
  2021-07-26  9:34           ` Marc Kleine-Budde
@ 2021-07-26 10:42           ` Yasushi SHOJI
  2021-07-26 11:17             ` Marc Kleine-Budde
  1 sibling, 1 reply; 18+ messages in thread
From: Yasushi SHOJI @ 2021-07-26 10:42 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Yasushi SHOJI, Pavel Skripkin, linux-can

Hi Marc,

On Mon, Jul 26, 2021 at 6:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Great! I've send an update to the MAINTAINERs file, Can you reply with
> your "Acked-by:"?

Acked-by: Yasushi SHOJI <yashi@spacecubics.com>
-- 
           yashi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  2021-07-26  9:34           ` Marc Kleine-Budde
@ 2021-07-26 10:43             ` Yasushi SHOJI
  0 siblings, 0 replies; 18+ messages in thread
From: Yasushi SHOJI @ 2021-07-26 10:43 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Yasushi SHOJI, Pavel Skripkin, linux-can

Hi Marc,

On Mon, Jul 26, 2021 at 6:34 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> BTW: is this the device we're talking about?
>
> https://www.microchip.com/Developmenttools/ProductDetails/APGDT002

Yes, that's what I have.  It's about $200 USD.
-- 
              yashi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  2021-07-26 10:42           ` Yasushi SHOJI
@ 2021-07-26 11:17             ` Marc Kleine-Budde
  2021-07-27  2:19               ` Yasushi SHOJI
  0 siblings, 1 reply; 18+ messages in thread
From: Marc Kleine-Budde @ 2021-07-26 11:17 UTC (permalink / raw)
  To: Yasushi SHOJI; +Cc: Yasushi SHOJI, Pavel Skripkin, linux-can

[-- Attachment #1: Type: text/plain, Size: 723 bytes --]

On 26.07.2021 19:42:28, Yasushi SHOJI wrote:
> Hi Marc,
> 
> On Mon, Jul 26, 2021 at 6:31 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > Great! I've send an update to the MAINTAINERs file, Can you reply with
> > your "Acked-by:"?
> 
> Acked-by: Yasushi SHOJI <yashi@spacecubics.com>

Sorry for not being clear - I mean reply to the patch I've send.

However, I've updated your email address, add your Acked-by and sent a
v2.

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb
  2021-07-26 11:17             ` Marc Kleine-Budde
@ 2021-07-27  2:19               ` Yasushi SHOJI
  0 siblings, 0 replies; 18+ messages in thread
From: Yasushi SHOJI @ 2021-07-27  2:19 UTC (permalink / raw)
  To: Marc Kleine-Budde; +Cc: Yasushi SHOJI, Pavel Skripkin, linux-can

On Mon, Jul 26, 2021 at 8:17 PM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> Sorry for not being clear - I mean reply to the patch I've send.
>
> However, I've updated your email address, add your Acked-by and sent a
> v2.

Thanks!
-- 
             yashi

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [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, 0 replies; 18+ messages in thread
From: Marc Kleine-Budde @ 2021-06-15  7:33 UTC (permalink / raw)
  To: Pavel Skripkin
  Cc: wg, davem, linux-can, netdev, linux-kernel, syzbot+57281c762a3922e14dfe

[-- Attachment #1: Type: text/plain, Size: 1487 bytes --]

On 10.06.2021 00:58:33, Pavel Skripkin wrote:
> 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>

Applied to linux-can/testing.

Tnx,
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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ 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).