Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Yasushi SHOJI]

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

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Pavel Skripkin]

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

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Marc Kleine-Budde]

[-- 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 --]

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Pavel Skripkin]

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

[PATCH] net: can: add missing urb->transfer_dma initialization

[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

Re: [PATCH] net: can: add missing urb->transfer_dma initialization

[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

Re: [PATCH] net: can: add missing urb->transfer_dma initialization

[Marc Kleine-Budde]

[-- 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 --]

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[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

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Marc Kleine-Budde]

[-- 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 --]

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[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

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Marc Kleine-Budde]

[-- 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 --]

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Marc Kleine-Budde]

[-- 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 --]

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Yasushi SHOJI]

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

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Yasushi SHOJI]

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

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Marc Kleine-Budde]

[-- 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 --]

Re: [PATCH] can: mcba_usb: fix memory leak in mcba_usb

[Yasushi SHOJI]

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