[PATCH 001/001] can: gs_usb: gs_usb_open/close( ) fix memory leak

[PATCH 001/001] can: gs_usb: gs_usb_open/close( ) fix memory leak

[Rhett Aultman]

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

Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak

[Vincent Mailhol]

+CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
+CC: linux-usb@vger.kernel.org

Rhett Aultman <rhett.aultman@samsara.com> writes:
> 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.

Right. To be frank, I think that there is a gap in the current URB
interface. If you do a simple kmalloc(), you can just set
URB_FREE_BUFFER in urb::transfer_flags. usb_kill_anchored_urbs() would
then eventually call urb_destroy() and automatically free it for you.

As far as I understand, I am not aware of equivalent mechanism to
automatically call usb_free_coherent() in the case that the driver
uses DMA memory. And I think that this is the root cause of this
"malady".

For me, the natural fix would be:

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 33d62d7e3929..1460fdac0b18 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
 
        if (urb->transfer_flags & URB_FREE_BUFFER)
                kfree(urb->transfer_buffer);
+       else if (urb->transfer_flags & URB_FREE_COHERENT)
+               usb_free_coherent(urb->dev, urb->transfer_buffer_length,
+                                 urb->transfer_buffer, urb->transfer_dma);
 
        kfree(urb);
 }
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 200b7b79acb5..dfc348d56fed 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
 #define URB_NO_INTERRUPT       0x0080  /* HINT: no non-error interrupt
                                         * needed */
 #define URB_FREE_BUFFER                0x0100  /* Free transfer buffer with the URB */
+#define URB_FREE_COHERENT      0x0200  /* Free DMA memory of transfer buffer */
 
 /* The following flags are used internally by usbcore and HCDs */
 #define URB_DIR_IN             0x0200  /* Transfer from device to host */
---

After doing this, drivers only have to add the URB_FREE_COHERENT flag
and voila!

Reusing URB_NO_TRANSFER_DMA_MAP to force a call to usb_free_coherent()
would be a bad idea because it would result in a double free for all
the drivers which correctly free their DMA memory. This is why above
snippet introduces a new URB_FREE_COHERENT flag.

Maybe I missed something obvious, but if so, I would like to
understand what is wrong in above approach.

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

Nitpick: the From tag is redundant. You already have it in the e-mail
header. It is relevant to explicitly add the From tag when picking
someone's else patch, but if the author and the signer are the same,
you are good to go without.

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

I do not like how the driver has to keep in a local array a reference
to all DMA allocated memory. All this information is redundant because
already present in the URB. So I really hope that we can fix it on the
URB API side and remove such complexity on the driver side.

>  };
> 
>  /* 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
> 

Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak

[Vincent MAILHOL]

On Sat. 4 Jun. 2022 at 11:12, Vincent Mailhol
<mailhol.vincent@wanadoo.fr> wrote:
> +CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> +CC: linux-usb@vger.kernel.org
>
> Rhett Aultman <rhett.aultman@samsara.com> writes:
> > 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.
>
> Right. To be frank, I think that there is a gap in the current URB
> interface. If you do a simple kmalloc(), you can just set
> URB_FREE_BUFFER in urb::transfer_flags. usb_kill_anchored_urbs() would
> then eventually call urb_destroy() and automatically free it for you.
>
> As far as I understand, I am not aware of equivalent mechanism to
> automatically call usb_free_coherent() in the case that the driver
> uses DMA memory. And I think that this is the root cause of this
> "malady".
>
> For me, the natural fix would be:
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 33d62d7e3929..1460fdac0b18 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
>
>         if (urb->transfer_flags & URB_FREE_BUFFER)
>                 kfree(urb->transfer_buffer);
> +       else if (urb->transfer_flags & URB_FREE_COHERENT)
> +               usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> +                                 urb->transfer_buffer, urb->transfer_dma);
>
>         kfree(urb);
>  }
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 200b7b79acb5..dfc348d56fed 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
>  #define URB_NO_INTERRUPT       0x0080  /* HINT: no non-error interrupt
>                                          * needed */
>  #define URB_FREE_BUFFER                0x0100  /* Free transfer buffer with the URB */
> +#define URB_FREE_COHERENT      0x0200  /* Free DMA memory of transfer buffer */

#define URB_FREE_COHERENT      0x0400  /* Free DMA memory of transfer buffer */

Obviously, the value 0x0200 is already taken by URB_DIR_IN just below.
So 0x0400 seems more adequate.
Sorry for the noise.

>  /* The following flags are used internally by usbcore and HCDs */
>  #define URB_DIR_IN             0x0200  /* Transfer from device to host */
> ---
>
> After doing this, drivers only have to add the URB_FREE_COHERENT flag
> and voila!
>
> Reusing URB_NO_TRANSFER_DMA_MAP to force a call to usb_free_coherent()
> would be a bad idea because it would result in a double free for all
> the drivers which correctly free their DMA memory. This is why above
> snippet introduces a new URB_FREE_COHERENT flag.
>
> Maybe I missed something obvious, but if so, I would like to
> understand what is wrong in above approach.
>
> >
> > 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>
>
> Nitpick: the From tag is redundant. You already have it in the e-mail
> header. It is relevant to explicitly add the From tag when picking
> someone's else patch, but if the author and the signer are the same,
> you are good to go without.
>
> > 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];
>
> I do not like how the driver has to keep in a local array a reference
> to all DMA allocated memory. All this information is redundant because
> already present in the URB. So I really hope that we can fix it on the
> URB API side and remove such complexity on the driver side.
>
> >  };
> >
> >  /* 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
> >

Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak

[Rhett Aultman]

On Sat, 4 Jun 2022, Vincent MAILHOL wrote:

> > For me, the natural fix would be:
> >
> > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> > index 33d62d7e3929..1460fdac0b18 100644
> > --- a/drivers/usb/core/urb.c
> > +++ b/drivers/usb/core/urb.c
> > @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
> >
> >         if (urb->transfer_flags & URB_FREE_BUFFER)
> >                 kfree(urb->transfer_buffer);
> > +       else if (urb->transfer_flags & URB_FREE_COHERENT)
> > +               usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> > +                                 urb->transfer_buffer, urb->transfer_dma);
> >
> >         kfree(urb);
> >  }
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 200b7b79acb5..dfc348d56fed 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
> >  #define URB_NO_INTERRUPT       0x0080  /* HINT: no non-error interrupt
> >                                          * needed */
> >  #define URB_FREE_BUFFER                0x0100  /* Free transfer buffer with the URB */
> > +#define URB_FREE_COHERENT      0x0200  /* Free DMA memory of transfer buffer */
>
> #define URB_FREE_COHERENT      0x0400  /* Free DMA memory of transfer buffer */
>
> Obviously, the value 0x0200 is already taken by URB_DIR_IN just below.
> So 0x0400 seems more adequate.
> Sorry for the noise.

Now that you've pointed this out, I agree this appears to be the more
natural way to handle things.  I will take a point of making a rewrite
here.  Additionally, I'll do my best to bring all the other USB CAN
drivers in line with this.  My only concern in doing that is that I have
gs_usb devices to confirm the fix with but not devices for the other
drivers.

> > Maybe I missed something obvious, but if so, I would like to
> > understand what is wrong in above approach.

I don't think anything is wrong with the above approach.  I just happened
to see an issue in the gs_usb driver that was already fixed in the other
USB CAN drivers, and adapted the fix used in the other drivers to the
gs_usb one.  Doing so got your attention and you pointed out a more
general fix, so I agree with going this route.  I'll post a new patch this
week.

Best,
Rhett

[RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Vincent Mailhol]

When allocating URB memory with kmalloc(), drivers can simply set the
URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
will be freed in the background when calling killing the URB (for
example with usb_kill_anchored_urbs()).

However, there are no equivalent mechanism when allocating DMA memory
(with usb_alloc_coherent()).

This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
cause the kernel to automatically call usb_free_coherent() when the
URB is killed, similarly to how URB_FREE_BUFFER triggers a call to
kfree().

Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Hi Rhett Aultman,

I put the code snippet I previously sent into a patch. It is not
tested (this is why I post it as RFC). Please feel free to add this to
your series.
---
 drivers/usb/core/urb.c | 3 +++
 include/linux/usb.h    | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 33d62d7e3929..1460fdac0b18 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
 
 	if (urb->transfer_flags & URB_FREE_BUFFER)
 		kfree(urb->transfer_buffer);
+	else if (urb->transfer_flags & URB_FREE_COHERENT)
+		usb_free_coherent(urb->dev, urb->transfer_buffer_length,
+				  urb->transfer_buffer, urb->transfer_dma);
 
 	kfree(urb);
 }
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 60bee864d897..2200b3785fdb 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
 #define URB_NO_INTERRUPT	0x0080	/* HINT: no non-error interrupt
 					 * needed */
 #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
+#define URB_FREE_COHERENT	0x0400  /* Free DMA memory of transfer buffer */
 
 /* The following flags are used internally by usbcore and HCDs */
 #define URB_DIR_IN		0x0200	/* Transfer from device to host */
-- 
2.35.1

Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Alan Stern]

On Sat, Jun 04, 2022 at 11:41:57PM +0900, Vincent Mailhol wrote:
> When allocating URB memory with kmalloc(), drivers can simply set the
> URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> will be freed in the background when calling killing the URB (for
> example with usb_kill_anchored_urbs()).
> 
> However, there are no equivalent mechanism when allocating DMA memory
> (with usb_alloc_coherent()).
> 
> This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> cause the kernel to automatically call usb_free_coherent() when the
> URB is killed, similarly to how URB_FREE_BUFFER triggers a call to
> kfree().
> 
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
> Hi Rhett Aultman,
> 
> I put the code snippet I previously sent into a patch. It is not
> tested (this is why I post it as RFC). Please feel free to add this to
> your series.
> ---
>  drivers/usb/core/urb.c | 3 +++
>  include/linux/usb.h    | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 33d62d7e3929..1460fdac0b18 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
>  
>  	if (urb->transfer_flags & URB_FREE_BUFFER)
>  		kfree(urb->transfer_buffer);
> +	else if (urb->transfer_flags & URB_FREE_COHERENT)
> +		usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> +				  urb->transfer_buffer, urb->transfer_dma);
>  
>  	kfree(urb);
>  }
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 60bee864d897..2200b3785fdb 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
>  #define URB_NO_INTERRUPT	0x0080	/* HINT: no non-error interrupt
>  					 * needed */
>  #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
> +#define URB_FREE_COHERENT	0x0400  /* Free DMA memory of transfer buffer */
>  
>  /* The following flags are used internally by usbcore and HCDs */
>  #define URB_DIR_IN		0x0200	/* Transfer from device to host */

I don't see anything wrong with this, except that it would be nice to keep 
the flag values in numerical order.  In other words, set URB_FREE_COHERENT 
to 0x0200 and change URB_DIR_IN to 0x0400.

Alan Stern

Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Vincent MAILHOL]

On Sun. 5 juin 2022 at 01:40, Alan Stern <stern@rowland.harvard.edu> wrote :
> On Sat, Jun 04, 2022 at 11:41:57PM +0900, Vincent Mailhol wrote:
> > When allocating URB memory with kmalloc(), drivers can simply set the
> > URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> > will be freed in the background when calling killing the URB (for
> > example with usb_kill_anchored_urbs()).
> >
> > However, there are no equivalent mechanism when allocating DMA memory
> > (with usb_alloc_coherent()).
> >
> > This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> > cause the kernel to automatically call usb_free_coherent() when the
> > URB is killed, similarly to how URB_FREE_BUFFER triggers a call to
> > kfree().
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > Hi Rhett Aultman,
> >
> > I put the code snippet I previously sent into a patch. It is not
> > tested (this is why I post it as RFC). Please feel free to add this to
> > your series.
> > ---
> >  drivers/usb/core/urb.c | 3 +++
> >  include/linux/usb.h    | 1 +
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> > index 33d62d7e3929..1460fdac0b18 100644
> > --- a/drivers/usb/core/urb.c
> > +++ b/drivers/usb/core/urb.c
> > @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
> >
> >       if (urb->transfer_flags & URB_FREE_BUFFER)
> >               kfree(urb->transfer_buffer);
> > +     else if (urb->transfer_flags & URB_FREE_COHERENT)
> > +             usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> > +                               urb->transfer_buffer, urb->transfer_dma);
> >
> >       kfree(urb);
> >  }
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 60bee864d897..2200b3785fdb 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
> >  #define URB_NO_INTERRUPT     0x0080  /* HINT: no non-error interrupt
> >                                        * needed */
> >  #define URB_FREE_BUFFER              0x0100  /* Free transfer buffer with the URB */
> > +#define URB_FREE_COHERENT    0x0400  /* Free DMA memory of transfer buffer */
> >
> >  /* The following flags are used internally by usbcore and HCDs */
> >  #define URB_DIR_IN           0x0200  /* Transfer from device to host */
>
> I don't see anything wrong with this, except that it would be nice to keep
> the flag values in numerical order.  In other words, set URB_FREE_COHERENT
> to 0x0200 and change URB_DIR_IN to 0x0400.

Indeed, the URB_DIR_IN macro is not part of the UAPI so it should be
safe to renumber it. Maybe I was confused by the USB_DIR_IN which is
part of UAPI so some part of my subcocient was telling me not to
change it...

Thanks for pointing this out, I will send a v2 right away (and I will
keep the RFC tag because this is not yet tested).


Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Oliver Neukum]

On 04.06.22 18:40, Alan Stern wrote:
>
> I don't see anything wrong with this, except that it would be nice to keep 
> the flag values in numerical order.  In other words, set URB_FREE_COHERENT 
> to 0x0200 and change URB_DIR_IN to 0x0400.
Hi,

but what sense does this patch make? You use coherent allocations
to avoid repeated DMA synchronizations. That is sort of incompatible
with the notion of discarding the buffer automatically.

    Regards
        Oliver

Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Vincent MAILHOL]

On Sun. 5 Jun. 2022 at 15:00, Oliver Neukum <oneukum@suse.com> wrote:
> On 04.06.22 18:40, Alan Stern wrote:
> >
> > I don't see anything wrong with this, except that it would be nice to keep
> > the flag values in numerical order.  In other words, set URB_FREE_COHERENT
> > to 0x0200 and change URB_DIR_IN to 0x0400.
> Hi,
>
> but what sense does this patch make? You use coherent allocations
> to avoid repeated DMA synchronizations. That is sort of incompatible
> with the notion of discarding the buffer automatically.

This is how I see things:
  * In the open() function, the driver will do the coherent allocation
for its transfer_buffers, fill those into URBs and add all the URBs in
an anchor.
  * During runtime, the driver will keep recycling the same URBs (no
need to kill URB nor to usb_free_coherent() the transfer_buffer).
  * Finally, in the close() function, the driver has to kill the URBs
and usb_free_coherent() the transfer_buffers. As far as I understand,
no helper functions allow us to do all that, thus requiring the driver
to iterate through the anchor to manually usb_free_coherent() the
transfer buffer.

So, the intent of this patch is to provide a method to both kill the
URBs and usb_free_coherent() the transfer buffer at once. The
rationale is that those two actions are done sequentially, at least in
the use case exposed above, and so the driver has enough control to
assert that usb_free_coherent() will occur at a good timing. For other
use cases where killing the URBs and freeing the transfer_buffer
should be done separately, the driver should not set URB_FREE_COHERENT
and continue to manually usb_free_coherent() the transfer_buffer.

Which part of this logic is incompatible with the coherent allocation?
Also, if the above is incorrect, what is the canonical way to kill the
URBs and free the transfer_buffers?

Thank you!


Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Oliver Neukum]

On 05.06.22 15:45, Vincent MAILHOL wrote:
>
> This is how I see things:
>   * In the open() function, the driver will do the coherent allocation
> for its transfer_buffers, fill those into URBs and add all the URBs in
> an anchor.
>   * During runtime, the driver will keep recycling the same URBs (no
> need to kill URB nor to usb_free_coherent() the transfer_buffer).
Yes.
>   * Finally, in the close() function, the driver has to kill the URBs
> and usb_free_coherent() the transfer_buffers. As far as I understand,
> no helper functions allow us to do all that, thus requiring the driver
> to iterate through the anchor to manually usb_free_coherent() the
> transfer buffer.
Yes. But you cannot nicely solve that with a flag as you proposed. You
would need to use a helper function.
> So, the intent of this patch is to provide a method to both kill the
> URBs and usb_free_coherent() the transfer buffer at once. The
Well, you don't directly. Your patch frees the buffer together with the URB.
That has some uses, but you still would need to iterate over the URBs
Yes, there is a helper for that, but then you cover one and only one
use case, that is, you leave no way to free the buffers without
at the same time discrading the URBs.

You can do that, but it strikes me as unelegant.

    Regards
        Oliver

Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Vincent MAILHOL]

On Tue. 7 Jun 2022 at 18:49, Oliver Neukum <oneukum@suse.com> wrote:
> On 05.06.22 15:45, Vincent MAILHOL wrote:
> >
> > This is how I see things:
> >   * In the open() function, the driver will do the coherent allocation
> > for its transfer_buffers, fill those into URBs and add all the URBs in
> > an anchor.
> >   * During runtime, the driver will keep recycling the same URBs (no
> > need to kill URB nor to usb_free_coherent() the transfer_buffer).
> Yes.
> >   * Finally, in the close() function, the driver has to kill the URBs
> > and usb_free_coherent() the transfer_buffers. As far as I understand,
> > no helper functions allow us to do all that, thus requiring the driver
> > to iterate through the anchor to manually usb_free_coherent() the
> > transfer buffer.
> Yes. But you cannot nicely solve that with a flag as you proposed. You
> would need to use a helper function.
> > So, the intent of this patch is to provide a method to both kill the
> > URBs and usb_free_coherent() the transfer buffer at once. The
> Well, you don't directly. Your patch frees the buffer together with the URB.
> That has some uses, but you still would need to iterate over the URBs
> Yes, there is a helper for that, but then you cover one and only one
> use case, that is, you leave no way to free the buffers without
> at the same time discrading the URBs.
>
> You can do that, but it strikes me as unelegant.

Elegancy is also my concern.

My RFC originated from this patch:
https://lore.kernel.org/linux-can/alpine.DEB.2.22.394.2206031547001.1630869@thelappy/

Here the proposed solution was to keep a pointer of all the
transfer_buffer in a local array to be able to free them when closing.
I really found that original patch to be unelegant which led me to
propose this RFC.
Comparatively, I still think my patch to be a more elegant solution,
and the original author also seems to share my thoughts.

If my patch is unelegant, then what would be the elegant/state of the
art way to free all this DMA allocated memory?
(pointing to any reference driver implementation should be enough for
me to understand).


Yours sincerely,
Vincent Mailhol

Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Oliver Neukum]

On 07.06.22 12:18, Vincent MAILHOL wrote:
> Here the proposed solution was to keep a pointer of all the
> transfer_buffer in a local array to be able to free them when closing.
> I really found that original patch to be unelegant which led me to
> propose this RFC.
It was.
> Comparatively, I still think my patch to be a more elegant solution,
> and the original author also seems to share my thoughts.
>
> If my patch is unelegant, then what would be the elegant/state of the
> art way to free all this DMA allocated memory?
> (pointing to any reference driver implementation should be enough for
> me to understand).

I have two options to offer.

1. Use the existing URB_NO_TRANSFER_D;MA_MAP to decide
how to free memory.

2. Provide an explicit API in usbcore among the anchor API

    Regards
        Oliver

Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Vincent MAILHOL]

On Tue. 7 Jun 2022 at 20:46, Oliver Neukum <oneukum@suse.com> wrote:
> On 07.06.22 12:18, Vincent MAILHOL wrote:
> > Here the proposed solution was to keep a pointer of all the
> > transfer_buffer in a local array to be able to free them when closing.
> > I really found that original patch to be unelegant which led me to
> > propose this RFC.
> It was.
> > Comparatively, I still think my patch to be a more elegant solution,
> > and the original author also seems to share my thoughts.
> >
> > If my patch is unelegant, then what would be the elegant/state of the
> > art way to free all this DMA allocated memory?
> > (pointing to any reference driver implementation should be enough for
> > me to understand).
>
> I have two options to offer.
>
> 1. Use the existing URB_NO_TRANSFER_D;MA_MAP to decide
> how to free memory.

If doing so, all drivers using DMA would need to be adjusted (or else
that would be a double free). Similar to URB_FREE_BUFFER, I think it
must be a separate flag so that the drivers still have the option to
release the memory by hand if needed for specific use cases.

> 2. Provide an explicit API in usbcore among the anchor API

Like a usb_kill_anchored_urbs_and_free_coherent()?

Wouldn't it be inconsistent with the URB_FREE_BUFFER flag? It is weird
to me if a normal kmalloc()ed buffer would have to use a flag and
DMAed buffers a special API call.
I mean, yes, it would work but I am not convinced that this approach
is more elegant.

The other advantage I see in my approach is that by setting the
URB_FREE_COHERENT flag, all the kill/free functions from the URB API
would benefit. For example, usb_free_urb() would also benefit. Right
now, I only see the use case for usb_kill_anchored_urbs() but I am
worried that we will have to duplicate many of the URB API.


Yours sincerely,
Vincent Mailhol

[RFC PATCH v2] usb: urb: add new transfer flag URB_FREE_COHERENT

[Vincent Mailhol]

When allocating URB memory with kmalloc(), drivers can simply set the
URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
will be freed in the background when killing the URB (for example with
usb_kill_anchored_urbs()).

However, there are no equivalent mechanism when allocating DMA memory
(with usb_alloc_coherent()).

This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
cause the kernel to automatically call usb_free_coherent() on the
transfer buffer when the URB is killed, similarly to how
URB_FREE_BUFFER triggers a call to kfree().

In order to have all the flags in numerical order, URB_DIR_IN is
renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
value 0x0200.

CC: Alan Stern <stern@rowland.harvard.edu>
CC: Rhett Aultman <rhett.aultman@samsara.com>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
Hi Rhett Aultman,

I put the code snippet I previously sent into a patch. It is not
tested (this is why I post it as RFC). Please feel free to add this to
your series.

** Changelog **

v1 -> v2:

  * Renumber URB_DIR_IN for 0x0200 to 0x0400 in order to keep all the
    flags in numerical order.
---
 drivers/usb/core/urb.c | 3 +++
 include/linux/usb.h    | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 33d62d7e3929..1460fdac0b18 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
 
 	if (urb->transfer_flags & URB_FREE_BUFFER)
 		kfree(urb->transfer_buffer);
+	else if (urb->transfer_flags & URB_FREE_COHERENT)
+		usb_free_coherent(urb->dev, urb->transfer_buffer_length,
+				  urb->transfer_buffer, urb->transfer_dma);
 
 	kfree(urb);
 }
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 60bee864d897..945d68ea1d76 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1328,9 +1328,10 @@ extern int usb_disabled(void);
 #define URB_NO_INTERRUPT	0x0080	/* HINT: no non-error interrupt
 					 * needed */
 #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
+#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
 
 /* The following flags are used internally by usbcore and HCDs */
-#define URB_DIR_IN		0x0200	/* Transfer from device to host */
+#define URB_DIR_IN		0x0400	/* Transfer from device to host */
 #define URB_DIR_OUT		0
 #define URB_DIR_MASK		URB_DIR_IN
 
-- 
2.35.1

Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak

[Vincent MAILHOL]

On Sat. 4 juin 2022 at 23:08, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> On Sat, 4 Jun 2022, Vincent MAILHOL wrote:
>
> > > For me, the natural fix would be:
> > >
> > > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> > > index 33d62d7e3929..1460fdac0b18 100644
> > > --- a/drivers/usb/core/urb.c
> > > +++ b/drivers/usb/core/urb.c
> > > @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
> > >
> > >         if (urb->transfer_flags & URB_FREE_BUFFER)
> > >                 kfree(urb->transfer_buffer);
> > > +       else if (urb->transfer_flags & URB_FREE_COHERENT)
> > > +               usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> > > +                                 urb->transfer_buffer, urb->transfer_dma);
> > >
> > >         kfree(urb);
> > >  }
> > > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > > index 200b7b79acb5..dfc348d56fed 100644
> > > --- a/include/linux/usb.h
> > > +++ b/include/linux/usb.h
> > > @@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
> > >  #define URB_NO_INTERRUPT       0x0080  /* HINT: no non-error interrupt
> > >                                          * needed */
> > >  #define URB_FREE_BUFFER                0x0100  /* Free transfer buffer with the URB */
> > > +#define URB_FREE_COHERENT      0x0200  /* Free DMA memory of transfer buffer */
> >
> > #define URB_FREE_COHERENT      0x0400  /* Free DMA memory of transfer buffer */
> >
> > Obviously, the value 0x0200 is already taken by URB_DIR_IN just below.
> > So 0x0400 seems more adequate.
> > Sorry for the noise.
>
> Now that you've pointed this out, I agree this appears to be the more
> natural way to handle things.  I will take a point of making a rewrite
> here.  Additionally, I'll do my best to bring all the other USB CAN
> drivers in line with this.

For the background, I also had a lot of trouble understanding the
logic of how to usb_free_coherent() URBs when I wrote my own driver
(etas_es58x). At the end, I just mimicked what other USB CAN drivers
were doing which means that the etas_es58x was most certainly also
contaminated by that malady.

I really appreciate your effort to also want to fix others' drivers.

> My only concern in doing that is that I have
> gs_usb devices to confirm the fix with but not devices for the other
> drivers.
>
> > > Maybe I missed something obvious, but if so, I would like to
> > > understand what is wrong in above approach.
>
> I don't think anything is wrong with the above approach.

I hope so. Sometimes, there are some complicated intricacies which are
hard to appreciate (and I am not an expert of the USB subsystem). This
is why I CCed the USB mailing list. If I am wrong, someone should
eventually shout at me.

> I just happened
> to see an issue in the gs_usb driver that was already fixed in the other
> USB CAN drivers, and adapted the fix used in the other drivers to the
> gs_usb one.  Doing so got your attention and you pointed out a more
> general fix, so I agree with going this route.  I'll post a new patch this
> week.

I sent you a patch. You can build on top of it the fixes for the other drivers.
And do not hesitate to resend it as part of your series (and in that
particular case, you would need to explicitly add the From: tag).


Yours sincerely,
Vincent Mailhol

[PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix

[Rhett Aultman]

This patchset is a v2 replacement for an earlier patch to resolve a
memory leak which can occur with successive iterations of calling
gs_can_open() and gs_can_close().  The central cause of this memory
leak, which is an issue common to many of the USB CAN drivers, is that
memory allocated for RX buffers using usb_alloc_coherent() and then kept
in the URB will be properly freed when the URB is killed.  This
assumption is incorrect, as memory allocated with usb_alloc_coherent()
must be freed with usb_free_coherent(), and there is no provision for this
in the existing URB code.

The common solution to this, found in v1 of my patches as well as in
already merged patches for other CAN USB drivers (see the patch for the
esd CAN-USB/2 driver here:
https://www.spinics.net/lists/linux-can/msg08203.html) is for the driver
itself to maintain an array of addresses of the buffers allocated with
usb_alloc_coherent() and to then iteratively call usb_free_coherent() on
them in their close function.  This solution requires a driver developer
to understand this unclear nuance, and it has historically been solved
in a piecemeal way one driver at a time (note: the gs_usb driver has had
this issue since the 3.x.x kernel series).

Rather than continue to place the burden of complexity on the drivers,
this patchset adds a new URB flag which allows the DMA buffer to be
correctly freed with the URB is killed.  This results in a much simpler
solution at the driver level and with minimal additional code in the USB
core.

Rhett Aultman (3):
  drivers: usb/core/urb: Add URB_FREE_COHERENT
  drivers: usb/core/urb: allow URB_FREE_COHERENT
  can: gs_usb: fix DMA memory leak on close

 drivers/net/can/usb/gs_usb.c | 2 +-
 drivers/usb/core/urb.c       | 5 ++++-
 include/linux/usb.h          | 3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.30.2

[PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Rhett Aultman]

When allocating URB memory with kmalloc(), drivers can simply set the
URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
will be freed in the background when killing the URB (for example with
usb_kill_anchored_urbs()).

However, there are no equivalent mechanism when allocating DMA memory
(with usb_alloc_coherent()).

This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
cause the kernel to automatically call usb_free_coherent() on the
transfer buffer when the URB is killed, similarly to how
URB_FREE_BUFFER triggers a call to kfree().

In order to have all the flags in numerical order, URB_DIR_IN is
renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
value 0x0200.

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/usb/core/urb.c | 3 +++
 include/linux/usb.h    | 3 ++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 33d62d7e3929..1460fdac0b18 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
 
 	if (urb->transfer_flags & URB_FREE_BUFFER)
 		kfree(urb->transfer_buffer);
+	else if (urb->transfer_flags & URB_FREE_COHERENT)
+		usb_free_coherent(urb->dev, urb->transfer_buffer_length,
+				  urb->transfer_buffer, urb->transfer_dma);
 
 	kfree(urb);
 }
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 60bee864d897..945d68ea1d76 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1328,9 +1328,10 @@ extern int usb_disabled(void);
 #define URB_NO_INTERRUPT	0x0080	/* HINT: no non-error interrupt
 					 * needed */
 #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
+#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
 
 /* The following flags are used internally by usbcore and HCDs */
-#define URB_DIR_IN		0x0200	/* Transfer from device to host */
+#define URB_DIR_IN		0x0400	/* Transfer from device to host */
 #define URB_DIR_OUT		0
 #define URB_DIR_MASK		URB_DIR_IN
 
-- 
2.30.2

Re: [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Fri. 10 juin 2022 à 06:11, Rhett Aultman <rhett.aultman@samsara.com> wrote:

The From tag goes at the beginning of the patch.

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

> When allocating URB memory with kmalloc(), drivers can simply set the
> URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> will be freed in the background when killing the URB (for example with
> usb_kill_anchored_urbs()).
>
> However, there are no equivalent mechanism when allocating DMA memory
> (with usb_alloc_coherent()).
>
> This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> cause the kernel to automatically call usb_free_coherent() on the
> transfer buffer when the URB is killed, similarly to how
> URB_FREE_BUFFER triggers a call to kfree().
>
> In order to have all the flags in numerical order, URB_DIR_IN is
> renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> value 0x0200.
>
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Move the From tag from here to the first line.

> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> ---
>  drivers/usb/core/urb.c | 3 +++
>  include/linux/usb.h    | 3 ++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 33d62d7e3929..1460fdac0b18 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
>
>         if (urb->transfer_flags & URB_FREE_BUFFER)
>                 kfree(urb->transfer_buffer);
> +       else if (urb->transfer_flags & URB_FREE_COHERENT)
> +               usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> +                                 urb->transfer_buffer, urb->transfer_dma);
>
>         kfree(urb);
>  }
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index 60bee864d897..945d68ea1d76 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -1328,9 +1328,10 @@ extern int usb_disabled(void);
>  #define URB_NO_INTERRUPT       0x0080  /* HINT: no non-error interrupt
>                                          * needed */
>  #define URB_FREE_BUFFER                0x0100  /* Free transfer buffer with the URB */
> +#define URB_FREE_COHERENT      0x0200  /* Free DMA memory of transfer buffer */
>
>  /* The following flags are used internally by usbcore and HCDs */
> -#define URB_DIR_IN             0x0200  /* Transfer from device to host */
> +#define URB_DIR_IN             0x0400  /* Transfer from device to host */
>  #define URB_DIR_OUT            0
>  #define URB_DIR_MASK           URB_DIR_IN

Re: [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Marc Kleine-Budde]

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

On 10.06.2022 09:18:51, Vincent MAILHOL wrote:
> On Fri. 10 juin 2022 à 06:11, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> 
> The From tag goes at the beginning of the patch.
> 
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> > When allocating URB memory with kmalloc(), drivers can simply set the
> > URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> > will be freed in the background when killing the URB (for example with
> > usb_kill_anchored_urbs()).
> >
> > However, there are no equivalent mechanism when allocating DMA memory
> > (with usb_alloc_coherent()).
> >
> > This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> > cause the kernel to automatically call usb_free_coherent() on the
> > transfer buffer when the URB is killed, similarly to how
> > URB_FREE_BUFFER triggers a call to kfree().
> >
> > In order to have all the flags in numerical order, URB_DIR_IN is
> > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> > value 0x0200.
> >
> > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> Move the From tag from here to the first line.

Usually git send-email places the "From:" automatically at the beginning.

> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Rhett, please add your S-o-b here, too, as the patch went though your
hands.

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

[PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT

[Rhett Aultman]

The URB_FREE_COHERENT flag needs to be added to the allowed flags set in
order to prevent a "bogus flags" warning when it is used.

Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
---
 drivers/usb/core/urb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 1460fdac0b18..36c48fb196e0 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -507,7 +507,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 
 	/* Check against a simple/standard policy */
 	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
-			URB_FREE_BUFFER);
+			URB_FREE_BUFFER | URB_FREE_COHERENT);
 	switch (xfertype) {
 	case USB_ENDPOINT_XFER_BULK:
 	case USB_ENDPOINT_XFER_INT:
-- 
2.30.2

Re: [PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT

[Vincent Mailhol]

On Fri. 10 juin 2022 at 06:13, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> The URB_FREE_COHERENT flag needs to be added to the allowed flags set in
> order to prevent a "bogus flags" warning when it is used.
>
> Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> ---
>  drivers/usb/core/urb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index 1460fdac0b18..36c48fb196e0 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -507,7 +507,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>
>         /* Check against a simple/standard policy */
>         allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
> -                       URB_FREE_BUFFER);
> +                       URB_FREE_BUFFER | URB_FREE_COHERENT);
>         switch (xfertype) {
>         case USB_ENDPOINT_XFER_BULK:
>         case USB_ENDPOINT_XFER_INT:

Thanks for the testing and thanks for fixing this warning.
I do not think this needs to be a separate patch. You can squash it to
my patch and add Co-developed-by/Signed-off-by tags to reflect your
work.

Yours sincerely,
Vincent Mailhol

[PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close

[Rhett Aultman]

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 much 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>
---
 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..3ded3e14c830 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);
 
-- 
2.30.2

Re: [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close

[Vincent Mailhol]

On Fri. 10 Jun 2022 at 05:53, Rhett Aultman <rhett.aultman@samsara.com> 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 much be freed using usb_free_coherent() instead.  This means that
      ^^^^
and must

> 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>
> ---
>  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..3ded3e14c830 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);

Nitpick but parenthesis are not needed here. Also, there are no
reasons to do a |=. I would prefer to see this:
        urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_FREE_COHERENT;

>                         usb_anchor_urb(urb, &parent->rx_submitted);

I looked at the code of gs_usb, this driver has a lot of dust. What
strikes me the most is that it uses usb_alloc_coherent() each time in
its xmit() function instead of allocating the TX URB once in the
open() function and resubmitting then in the callback function. That
last comment is not a criticism of this patch. But if someone has the
motivation to do some cleaning, go ahead…

Aside from the nitpicks, the patch looks good to me. You can add this
to your v3:
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>


Yours sincerely,
Vincent Mailhol

Re: [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close

[Vincent Mailhol]

On Fri. 10 juin 2022 at 09:05, Vincent Mailhol
<vincent.mailhol@gmail.com> wrote:
> On Fri. 10 Jun 2022 at 05:53, Rhett Aultman <rhett.aultman@samsara.com> 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 much be freed using usb_free_coherent() instead.  This means that
>       ^^^^
> and must
>
> > 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.
> >

One more thing, for bug fixes, the best practice is to add a Fixes tag. c.f.:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes

Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")

> > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> > ---
> >  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..3ded3e14c830 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);
>
> Nitpick but parenthesis are not needed here. Also, there are no
> reasons to do a |=. I would prefer to see this:
>         urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP | URB_FREE_COHERENT;
>
> >                         usb_anchor_urb(urb, &parent->rx_submitted);
>
> I looked at the code of gs_usb, this driver has a lot of dust. What
> strikes me the most is that it uses usb_alloc_coherent() each time in
> its xmit() function instead of allocating the TX URB once in the
> open() function and resubmitting then in the callback function. That
> last comment is not a criticism of this patch. But if someone has the
> motivation to do some cleaning, go ahead…
>
> Aside from the nitpicks, the patch looks good to me. You can add this
> to your v3:
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
>
> Yours sincerely,
> Vincent Mailhol

[PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix

[Rhett Aultman]

This patchset resolves a to memory leak which can occur with successive
iterations of calling gs_can_open() and gs_can_close().  The central
cause of this memory leak, which is an issue common to many of the USB
CAN drivers, is that memory allocated for RX buffers using
usb_alloc_coherent() and then kept in the URB will be properly freed
when the URB is killed.  This assumption is incorrect, as memory
allocated with usb_alloc_coherent() must be freed with
usb_free_coherent(), and there is no provision for this in the existing
URB code.

The common solution to this, found in v1 of my patches as well as in
already merged patches for other CAN USB drivers (see the patch for the
esd CAN-USB/2 driver here:
https://www.spinics.net/lists/linux-can/msg08203.html) is for the driver
itself to maintain an array of addresses of the buffers allocated with
usb_alloc_coherent() and to then iteratively call usb_free_coherent() on
them in their close function.  This solution requires a driver developer
to understand this unclear nuance, and it has historically been solved
in a piecemeal way one driver at a time (note: the gs_usb driver has had
this issue since the 3.x.x kernel series).

Rather than continue to place the burden of complexity on the drivers,
this patchset adds a new URB flag which allows the DMA buffer to be
correctly freed with the URB is killed.  This results in a much simpler
solution at the driver level and with minimal additional code in the USB
core.

** Changelog **

v2 -> v3:

  * Resolve style nits in gs_usb.c
  * Squash commits in urb.c and correct authorship

v1 -> v2:

  * Add URB_FREE_COHERENT flag to urb.c
  * Use URB_FREE_COHERENT flag rather than arrays of buffers in gs_usb.c

Rhett Aultman (1):
  can: gs_usb: fix DMA memory leak on close

Vincent Mailhol (1):
  drivers: usb/core/urb: Add URB_FREE_COHERENT

 drivers/net/can/usb/gs_usb.c | 2 +-
 drivers/usb/core/urb.c       | 5 ++++-
 include/linux/usb.h          | 3 ++-
 3 files changed, 7 insertions(+), 3 deletions(-)

-- 
2.30.2

[PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Rhett Aultman]

From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

When allocating URB memory with kmalloc(), drivers can simply set the
URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
will be freed in the background when killing the URB (for example with
usb_kill_anchored_urbs()).

However, there are no equivalent mechanism when allocating DMA memory
(with usb_alloc_coherent()).

This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
cause the kernel to automatically call usb_free_coherent() on the
transfer buffer when the URB is killed, similarly to how
URB_FREE_BUFFER triggers a call to kfree().

In order to have all the flags in numerical order, URB_DIR_IN is
renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
value 0x0200.

Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com>
Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 drivers/usb/core/urb.c | 5 ++++-
 include/linux/usb.h    | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 33d62d7e3929..36c48fb196e0 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
 
 	if (urb->transfer_flags & URB_FREE_BUFFER)
 		kfree(urb->transfer_buffer);
+	else if (urb->transfer_flags & URB_FREE_COHERENT)
+		usb_free_coherent(urb->dev, urb->transfer_buffer_length,
+				  urb->transfer_buffer, urb->transfer_dma);
 
 	kfree(urb);
 }
@@ -504,7 +507,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 
 	/* Check against a simple/standard policy */
 	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
-			URB_FREE_BUFFER);
+			URB_FREE_BUFFER | URB_FREE_COHERENT);
 	switch (xfertype) {
 	case USB_ENDPOINT_XFER_BULK:
 	case USB_ENDPOINT_XFER_INT:
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 60bee864d897..945d68ea1d76 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1328,9 +1328,10 @@ extern int usb_disabled(void);
 #define URB_NO_INTERRUPT	0x0080	/* HINT: no non-error interrupt
 					 * needed */
 #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
+#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
 
 /* The following flags are used internally by usbcore and HCDs */
-#define URB_DIR_IN		0x0200	/* Transfer from device to host */
+#define URB_DIR_IN		0x0400	/* Transfer from device to host */
 #define URB_DIR_OUT		0
 #define URB_DIR_MASK		URB_DIR_IN
 
-- 
2.30.2

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Marc Kleine-Budde]

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

On 10.06.2022 17:33:35, Rhett Aultman wrote:
> From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> 
> When allocating URB memory with kmalloc(), drivers can simply set the
> URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> will be freed in the background when killing the URB (for example with
> usb_kill_anchored_urbs()).
> 
> However, there are no equivalent mechanism when allocating DMA memory
> (with usb_alloc_coherent()).
> 
> This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> cause the kernel to automatically call usb_free_coherent() on the
> transfer buffer when the URB is killed, similarly to how
> URB_FREE_BUFFER triggers a call to kfree().
> 
> In order to have all the flags in numerical order, URB_DIR_IN is
> renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> value 0x0200.
> 
> Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com>
> Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

FWIW:
Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>

This patch probably goes upstream via USB. Once this is in net I'll take
the 2nd patch.

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 v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Sun. 12 juin 2022 at 00:31, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > When allocating URB memory with kmalloc(), drivers can simply set the
> > URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> > will be freed in the background when killing the URB (for example with
> > usb_kill_anchored_urbs()).
> >
> > However, there are no equivalent mechanism when allocating DMA memory
> > (with usb_alloc_coherent()).
> >
> > This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> > cause the kernel to automatically call usb_free_coherent() on the
> > transfer buffer when the URB is killed, similarly to how
> > URB_FREE_BUFFER triggers a call to kfree().
> >
> > In order to have all the flags in numerical order, URB_DIR_IN is
> > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> > value 0x0200.
> >
> > Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com>
> > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
>
> FWIW:
> Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
>
> This patch probably goes upstream via USB. Once this is in net I'll take
> the 2nd patch.

Question to Greg: can this first patch also be applied to the stable
branches? Technically, this is a new feature but it will be used to
solve several memory leaks on existing drivers (the gs_usb is only one
example).

Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Greg Kroah-Hartman]

On Sun, Jun 12, 2022 at 01:06:37AM +0900, Vincent MAILHOL wrote:
> On Sun. 12 juin 2022 at 00:31, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > >
> > > When allocating URB memory with kmalloc(), drivers can simply set the
> > > URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> > > will be freed in the background when killing the URB (for example with
> > > usb_kill_anchored_urbs()).
> > >
> > > However, there are no equivalent mechanism when allocating DMA memory
> > > (with usb_alloc_coherent()).
> > >
> > > This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> > > cause the kernel to automatically call usb_free_coherent() on the
> > > transfer buffer when the URB is killed, similarly to how
> > > URB_FREE_BUFFER triggers a call to kfree().
> > >
> > > In order to have all the flags in numerical order, URB_DIR_IN is
> > > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> > > value 0x0200.
> > >
> > > Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > FWIW:
> > Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >
> > This patch probably goes upstream via USB. Once this is in net I'll take
> > the 2nd patch.
> 
> Question to Greg: can this first patch also be applied to the stable
> branches? Technically, this is a new feature but it will be used to
> solve several memory leaks on existing drivers (the gs_usb is only one
> example).

We take in dependent patches into the stable trees all the time when
needed, that's not an issue here.

What is an issue here is that this feels odd as other USB developers
said previously.

My big objection here is what validates that the size of the transfer
buffer here is really the size of the buffer to be freed?  Is that
always set properly to be the length that was allocated?  That might
just be the size of the last transfer using this buffer, but there is no
guarantee that I can see of that says this really is the length of the
allocated buffer, which is why usb_free_coherent() requires a size
parameter.

If that guarantee is always right, then we should be able to drop the
size option in usb_free_coherent(), and I don't think that's really
possible.

thanks,

greg k-h

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Tue. 21 Jun 2022 at 22:56, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Jun 12, 2022 at 01:06:37AM +0900, Vincent MAILHOL wrote:
> > On Sun. 12 juin 2022 at 00:31, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > >
> > > > When allocating URB memory with kmalloc(), drivers can simply set the
> > > > URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> > > > will be freed in the background when killing the URB (for example with
> > > > usb_kill_anchored_urbs()).
> > > >
> > > > However, there are no equivalent mechanism when allocating DMA memory
> > > > (with usb_alloc_coherent()).
> > > >
> > > > This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> > > > cause the kernel to automatically call usb_free_coherent() on the
> > > > transfer buffer when the URB is killed, similarly to how
> > > > URB_FREE_BUFFER triggers a call to kfree().
> > > >
> > > > In order to have all the flags in numerical order, URB_DIR_IN is
> > > > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> > > > value 0x0200.
> > > >
> > > > Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > >
> > > FWIW:
> > > Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > >
> > > This patch probably goes upstream via USB. Once this is in net I'll take
> > > the 2nd patch.
> >
> > Question to Greg: can this first patch also be applied to the stable
> > branches? Technically, this is a new feature but it will be used to
> > solve several memory leaks on existing drivers (the gs_usb is only one
> > example).
>
> We take in dependent patches into the stable trees all the time when
> needed, that's not an issue here.
>
> What is an issue here is that this feels odd as other USB developers
> said previously.
>
> My big objection here is what validates that the size of the transfer
> buffer here is really the size of the buffer to be freed?  Is that
> always set properly to be the length that was allocated?  That might
> just be the size of the last transfer using this buffer, but there is no
> guarantee that I can see of that says this really is the length of the
> allocated buffer, which is why usb_free_coherent() requires a size
> parameter.

Thanks for the comment.

I (probably wrongly) assumed that urb::transfer_buffer_length was the
allocated length and urb::actual_length was what was actually being
transferred. Right now, I am just confused. Seems that I need to study
a bit more and understand the real purpose of
urb::transfer_buffer_length because I still fail to understand in
which situation this can be different from the allocated length.

> If that guarantee is always right, then we should be able to drop the
> size option in usb_free_coherent(), and I don't think that's really
> possible.

I do not follow you on this comment. usb_free_coherent() does not have
a reference to the URB, right? How would it be supposed to retrieve
urb::transfer_buffer_length?


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Greg Kroah-Hartman]

On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> On Tue. 21 Jun 2022 at 22:56, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Sun, Jun 12, 2022 at 01:06:37AM +0900, Vincent MAILHOL wrote:
> > > On Sun. 12 juin 2022 at 00:31, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > >
> > > > > When allocating URB memory with kmalloc(), drivers can simply set the
> > > > > URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> > > > > will be freed in the background when killing the URB (for example with
> > > > > usb_kill_anchored_urbs()).
> > > > >
> > > > > However, there are no equivalent mechanism when allocating DMA memory
> > > > > (with usb_alloc_coherent()).
> > > > >
> > > > > This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> > > > > cause the kernel to automatically call usb_free_coherent() on the
> > > > > transfer buffer when the URB is killed, similarly to how
> > > > > URB_FREE_BUFFER triggers a call to kfree().
> > > > >
> > > > > In order to have all the flags in numerical order, URB_DIR_IN is
> > > > > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> > > > > value 0x0200.
> > > > >
> > > > > Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > > > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > >
> > > > FWIW:
> > > > Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > >
> > > > This patch probably goes upstream via USB. Once this is in net I'll take
> > > > the 2nd patch.
> > >
> > > Question to Greg: can this first patch also be applied to the stable
> > > branches? Technically, this is a new feature but it will be used to
> > > solve several memory leaks on existing drivers (the gs_usb is only one
> > > example).
> >
> > We take in dependent patches into the stable trees all the time when
> > needed, that's not an issue here.
> >
> > What is an issue here is that this feels odd as other USB developers
> > said previously.
> >
> > My big objection here is what validates that the size of the transfer
> > buffer here is really the size of the buffer to be freed?  Is that
> > always set properly to be the length that was allocated?  That might
> > just be the size of the last transfer using this buffer, but there is no
> > guarantee that I can see of that says this really is the length of the
> > allocated buffer, which is why usb_free_coherent() requires a size
> > parameter.
> 
> Thanks for the comment.
> 
> I (probably wrongly) assumed that urb::transfer_buffer_length was the
> allocated length and urb::actual_length was what was actually being
> transferred. Right now, I am just confused. Seems that I need to study
> a bit more and understand the real purpose of
> urb::transfer_buffer_length because I still fail to understand in
> which situation this can be different from the allocated length.
> 
> > If that guarantee is always right, then we should be able to drop the
> > size option in usb_free_coherent(), and I don't think that's really
> > possible.
> 
> I do not follow you on this comment. usb_free_coherent() does not have
> a reference to the URB, right? How would it be supposed to retrieve
> urb::transfer_buffer_length?

Ah, good point.  Along those lines, how do you know what the `dma`
parameter should be set to as well?

thanks,

greg k-h

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Wed. 22 Jun 2022 at 00:11, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > On Tue. 21 Jun 2022 at 22:56, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Sun, Jun 12, 2022 at 01:06:37AM +0900, Vincent MAILHOL wrote:
> > > > On Sun. 12 juin 2022 at 00:31, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > > > > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > > > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > > >
> > > > > > When allocating URB memory with kmalloc(), drivers can simply set the
> > > > > > URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> > > > > > will be freed in the background when killing the URB (for example with
> > > > > > usb_kill_anchored_urbs()).
> > > > > >
> > > > > > However, there are no equivalent mechanism when allocating DMA memory
> > > > > > (with usb_alloc_coherent()).
> > > > > >
> > > > > > This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> > > > > > cause the kernel to automatically call usb_free_coherent() on the
> > > > > > transfer buffer when the URB is killed, similarly to how
> > > > > > URB_FREE_BUFFER triggers a call to kfree().
> > > > > >
> > > > > > In order to have all the flags in numerical order, URB_DIR_IN is
> > > > > > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> > > > > > value 0x0200.
> > > > > >
> > > > > > Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > > > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > > > > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > > > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > > >
> > > > > FWIW:
> > > > > Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > > > >
> > > > > This patch probably goes upstream via USB. Once this is in net I'll take
> > > > > the 2nd patch.
> > > >
> > > > Question to Greg: can this first patch also be applied to the stable
> > > > branches? Technically, this is a new feature but it will be used to
> > > > solve several memory leaks on existing drivers (the gs_usb is only one
> > > > example).
> > >
> > > We take in dependent patches into the stable trees all the time when
> > > needed, that's not an issue here.
> > >
> > > What is an issue here is that this feels odd as other USB developers
> > > said previously.
> > >
> > > My big objection here is what validates that the size of the transfer
> > > buffer here is really the size of the buffer to be freed?  Is that
> > > always set properly to be the length that was allocated?  That might
> > > just be the size of the last transfer using this buffer, but there is no
> > > guarantee that I can see of that says this really is the length of the
> > > allocated buffer, which is why usb_free_coherent() requires a size
> > > parameter.
> >
> > Thanks for the comment.
> >
> > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > allocated length and urb::actual_length was what was actually being
> > transferred. Right now, I am just confused. Seems that I need to study
> > a bit more and understand the real purpose of
> > urb::transfer_buffer_length because I still fail to understand in
> > which situation this can be different from the allocated length.
> >
> > > If that guarantee is always right, then we should be able to drop the
> > > size option in usb_free_coherent(), and I don't think that's really
> > > possible.
> >
> > I do not follow you on this comment. usb_free_coherent() does not have
> > a reference to the URB, right? How would it be supposed to retrieve
> > urb::transfer_buffer_length?
>
> Ah, good point.  Along those lines, how do you know what the `dma`
> parameter should be set to as well?

Here, just assuming that usb_alloc_coherent() was given
urb::transfer_dma as an argument. Maybe I missed some edge cases but I
never saw drivers doing it differently.

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Alan Stern]

On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> I (probably wrongly) assumed that urb::transfer_buffer_length was the
> allocated length and urb::actual_length was what was actually being
> transferred. Right now, I am just confused. Seems that I need to study
> a bit more and understand the real purpose of
> urb::transfer_buffer_length because I still fail to understand in
> which situation this can be different from the allocated length.

urb->transfer_buffer_length is the amount of data that the driver wants 
to send or expects to receive.  urb->actual_length is the amount of data 
that was actually sent or actually received.

Neither of these values has to be the same as the size of the buffer -- 
but they better not be bigger!

Alan Stern

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > allocated length and urb::actual_length was what was actually being
> > transferred. Right now, I am just confused. Seems that I need to study
> > a bit more and understand the real purpose of
> > urb::transfer_buffer_length because I still fail to understand in
> > which situation this can be different from the allocated length.
>
> urb->transfer_buffer_length is the amount of data that the driver wants
> to send or expects to receive.  urb->actual_length is the amount of data
> that was actually sent or actually received.
>
> Neither of these values has to be the same as the size of the buffer --
> but they better not be bigger!

Thanks. Now things are a bit clearer.
I guess that for the outcoming URB what I proposed made no sense. For
incoming URB, I guess that most of the drivers want to set
urb::transfer_buffer once for all with the allocated size and never
touch it again.

Maybe the patch only makes sense of the incoming URB. Would it make
sense to keep it but with an additional check to trigger a dmesg
warning if this is used on an outcoming endpoint and with additional
comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
the allocated size?

Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Alan Stern]

On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > allocated length and urb::actual_length was what was actually being
> > > transferred. Right now, I am just confused. Seems that I need to study
> > > a bit more and understand the real purpose of
> > > urb::transfer_buffer_length because I still fail to understand in
> > > which situation this can be different from the allocated length.
> >
> > urb->transfer_buffer_length is the amount of data that the driver wants
> > to send or expects to receive.  urb->actual_length is the amount of data
> > that was actually sent or actually received.
> >
> > Neither of these values has to be the same as the size of the buffer --
> > but they better not be bigger!
> 
> Thanks. Now things are a bit clearer.
> I guess that for the outcoming URB what I proposed made no sense. For
> incoming URB, I guess that most of the drivers want to set
> urb::transfer_buffer once for all with the allocated size and never
> touch it again.

Not necessarily.  Some drivers may behave differently from the way you 
expect.

> Maybe the patch only makes sense of the incoming URB. Would it make
> sense to keep it but with an additional check to trigger a dmesg
> warning if this is used on an outcoming endpoint and with additional
> comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> the allocated size?

Well, what really matters is that the transfer_buffer_length value has 
to be the same as the size of the buffer.  If that's true, the direction 
of the URB doesn't matter.  So yes, that requirement would definitely 
need to be documented.

On the other hand, there wouldn't be any way to tell automatically if 
the requirement was violated.  And since this function could only be 
used with some of the URBs you're interested in, does it make sense to 
add it at all?  The other URBs would still need their buffers to be 
freed manually.

Alan Stern

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Wed 22 Jun 2022 at 01:15, Alan Stern <stern@rowland.harvard.edu> wrote:
> On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > allocated length and urb::actual_length was what was actually being
> > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > a bit more and understand the real purpose of
> > > > urb::transfer_buffer_length because I still fail to understand in
> > > > which situation this can be different from the allocated length.
> > >
> > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > to send or expects to receive.  urb->actual_length is the amount of data
> > > that was actually sent or actually received.
> > >
> > > Neither of these values has to be the same as the size of the buffer --
> > > but they better not be bigger!
> >
> > Thanks. Now things are a bit clearer.
> > I guess that for the outcoming URB what I proposed made no sense. For
> > incoming URB, I guess that most of the drivers want to set
> > urb::transfer_buffer once for all with the allocated size and never
> > touch it again.
>
> Not necessarily.  Some drivers may behave differently from the way you
> expect.

Yes, my point is not to generalise. Agree that there are exceptions.

> > Maybe the patch only makes sense of the incoming URB. Would it make
> > sense to keep it but with an additional check to trigger a dmesg
> > warning if this is used on an outcoming endpoint and with additional
> > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > the allocated size?
>
> Well, what really matters is that the transfer_buffer_length value has
> to be the same as the size of the buffer.  If that's true, the direction
> of the URB doesn't matter.  So yes, that requirement would definitely
> need to be documented.
>
> On the other hand, there wouldn't be any way to tell automatically if
> the requirement was violated.

ACK. That's why I said "add comment" and not "check".

> And since this function could only be
> used with some of the URBs you're interested in, does it make sense to
> add it at all?  The other URBs would still need their buffers to be
> freed manually.

The rationale is that similarly to URB_FREE_BUFFER, this would be
optional. This is why I did not propose to reuse
URB_NO_TRANSFER_DMA_MAP but instead add a new flag. I propose it
because I think that many drivers can benefit from it.

More than that, the real concern is that many developers forget to
free the DMA allocated memory. c.f. original message of this thread:
https://lore.kernel.org/linux-can/alpine.DEB.2.22.394.2206031547001.1630869@thelappy/T/#m2ef343d3ee708178b1e37be898884bafa7f49f2f

And the usual fix requires to create local arrays to store references
to the transfer buffer and DMA addresses.

I would like to find a solution to remove this burden from the drivers
and have an USB API to easily free the URB when, for example, killing
an anchor.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Greg Kroah-Hartman]

On Wed, Jun 22, 2022 at 01:40:10AM +0900, Vincent MAILHOL wrote:
> On Wed 22 Jun 2022 at 01:15, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > allocated length and urb::actual_length was what was actually being
> > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > a bit more and understand the real purpose of
> > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > which situation this can be different from the allocated length.
> > > >
> > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > that was actually sent or actually received.
> > > >
> > > > Neither of these values has to be the same as the size of the buffer --
> > > > but they better not be bigger!
> > >
> > > Thanks. Now things are a bit clearer.
> > > I guess that for the outcoming URB what I proposed made no sense. For
> > > incoming URB, I guess that most of the drivers want to set
> > > urb::transfer_buffer once for all with the allocated size and never
> > > touch it again.
> >
> > Not necessarily.  Some drivers may behave differently from the way you
> > expect.
> 
> Yes, my point is not to generalise. Agree that there are exceptions.
> 
> > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > sense to keep it but with an additional check to trigger a dmesg
> > > warning if this is used on an outcoming endpoint and with additional
> > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > the allocated size?
> >
> > Well, what really matters is that the transfer_buffer_length value has
> > to be the same as the size of the buffer.  If that's true, the direction
> > of the URB doesn't matter.  So yes, that requirement would definitely
> > need to be documented.
> >
> > On the other hand, there wouldn't be any way to tell automatically if
> > the requirement was violated.
> 
> ACK. That's why I said "add comment" and not "check".
> 
> > And since this function could only be
> > used with some of the URBs you're interested in, does it make sense to
> > add it at all?  The other URBs would still need their buffers to be
> > freed manually.
> 
> The rationale is that similarly to URB_FREE_BUFFER, this would be
> optional. This is why I did not propose to reuse
> URB_NO_TRANSFER_DMA_MAP but instead add a new flag. I propose it
> because I think that many drivers can benefit from it.
> 
> More than that, the real concern is that many developers forget to
> free the DMA allocated memory. c.f. original message of this thread:
> https://lore.kernel.org/linux-can/alpine.DEB.2.22.394.2206031547001.1630869@thelappy/T/#m2ef343d3ee708178b1e37be898884bafa7f49f2f
> 
> And the usual fix requires to create local arrays to store references
> to the transfer buffer and DMA addresses.

Why not just free the memory in the urb completion function that is
called when it is finished?

thanks,

greg k-h

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Wed. 22 Jun 2022 at 02:02, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Jun 22, 2022 at 01:40:10AM +0900, Vincent MAILHOL wrote:
> > On Wed 22 Jun 2022 at 01:15, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > > allocated length and urb::actual_length was what was actually being
> > > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > > a bit more and understand the real purpose of
> > > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > > which situation this can be different from the allocated length.
> > > > >
> > > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > > that was actually sent or actually received.
> > > > >
> > > > > Neither of these values has to be the same as the size of the buffer --
> > > > > but they better not be bigger!
> > > >
> > > > Thanks. Now things are a bit clearer.
> > > > I guess that for the outcoming URB what I proposed made no sense. For
> > > > incoming URB, I guess that most of the drivers want to set
> > > > urb::transfer_buffer once for all with the allocated size and never
> > > > touch it again.
> > >
> > > Not necessarily.  Some drivers may behave differently from the way you
> > > expect.
> >
> > Yes, my point is not to generalise. Agree that there are exceptions.
> >
> > > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > > sense to keep it but with an additional check to trigger a dmesg
> > > > warning if this is used on an outcoming endpoint and with additional
> > > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > > the allocated size?
> > >
> > > Well, what really matters is that the transfer_buffer_length value has
> > > to be the same as the size of the buffer.  If that's true, the direction
> > > of the URB doesn't matter.  So yes, that requirement would definitely
> > > need to be documented.
> > >
> > > On the other hand, there wouldn't be any way to tell automatically if
> > > the requirement was violated.
> >
> > ACK. That's why I said "add comment" and not "check".
> >
> > > And since this function could only be
> > > used with some of the URBs you're interested in, does it make sense to
> > > add it at all?  The other URBs would still need their buffers to be
> > > freed manually.
> >
> > The rationale is that similarly to URB_FREE_BUFFER, this would be
> > optional. This is why I did not propose to reuse
> > URB_NO_TRANSFER_DMA_MAP but instead add a new flag. I propose it
> > because I think that many drivers can benefit from it.
> >
> > More than that, the real concern is that many developers forget to
> > free the DMA allocated memory. c.f. original message of this thread:
> > https://lore.kernel.org/linux-can/alpine.DEB.2.22.394.2206031547001.1630869@thelappy/T/#m2ef343d3ee708178b1e37be898884bafa7f49f2f
> >
> > And the usual fix requires to create local arrays to store references
> > to the transfer buffer and DMA addresses.
>
> Why not just free the memory in the urb completion function that is
> called when it is finished?

I thought it was a bad thing to call usb_free_coherent() in the
completion function and that the best practice was to allocate all the
DMA memory when opening the device, reuse them during runtime and free
everything when closing the device.

Especially, correct me if I am wrong, but isn't there a risk to
trigger below warning if calling usb_free_coherent() in an IRQ context
(which is the case in the completion function)?
https://elixir.bootlin.com/linux/v5.18/source/kernel/dma/mapping.c#L526


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Greg Kroah-Hartman]

On Wed, Jun 22, 2022 at 02:14:37AM +0900, Vincent MAILHOL wrote:
> On Wed. 22 Jun 2022 at 02:02, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jun 22, 2022 at 01:40:10AM +0900, Vincent MAILHOL wrote:
> > > On Wed 22 Jun 2022 at 01:15, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > > > allocated length and urb::actual_length was what was actually being
> > > > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > > > a bit more and understand the real purpose of
> > > > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > > > which situation this can be different from the allocated length.
> > > > > >
> > > > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > > > that was actually sent or actually received.
> > > > > >
> > > > > > Neither of these values has to be the same as the size of the buffer --
> > > > > > but they better not be bigger!
> > > > >
> > > > > Thanks. Now things are a bit clearer.
> > > > > I guess that for the outcoming URB what I proposed made no sense. For
> > > > > incoming URB, I guess that most of the drivers want to set
> > > > > urb::transfer_buffer once for all with the allocated size and never
> > > > > touch it again.
> > > >
> > > > Not necessarily.  Some drivers may behave differently from the way you
> > > > expect.
> > >
> > > Yes, my point is not to generalise. Agree that there are exceptions.
> > >
> > > > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > > > sense to keep it but with an additional check to trigger a dmesg
> > > > > warning if this is used on an outcoming endpoint and with additional
> > > > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > > > the allocated size?
> > > >
> > > > Well, what really matters is that the transfer_buffer_length value has
> > > > to be the same as the size of the buffer.  If that's true, the direction
> > > > of the URB doesn't matter.  So yes, that requirement would definitely
> > > > need to be documented.
> > > >
> > > > On the other hand, there wouldn't be any way to tell automatically if
> > > > the requirement was violated.
> > >
> > > ACK. That's why I said "add comment" and not "check".
> > >
> > > > And since this function could only be
> > > > used with some of the URBs you're interested in, does it make sense to
> > > > add it at all?  The other URBs would still need their buffers to be
> > > > freed manually.
> > >
> > > The rationale is that similarly to URB_FREE_BUFFER, this would be
> > > optional. This is why I did not propose to reuse
> > > URB_NO_TRANSFER_DMA_MAP but instead add a new flag. I propose it
> > > because I think that many drivers can benefit from it.
> > >
> > > More than that, the real concern is that many developers forget to
> > > free the DMA allocated memory. c.f. original message of this thread:
> > > https://lore.kernel.org/linux-can/alpine.DEB.2.22.394.2206031547001.1630869@thelappy/T/#m2ef343d3ee708178b1e37be898884bafa7f49f2f
> > >
> > > And the usual fix requires to create local arrays to store references
> > > to the transfer buffer and DMA addresses.
> >
> > Why not just free the memory in the urb completion function that is
> > called when it is finished?
> 
> I thought it was a bad thing to call usb_free_coherent() in the
> completion function and that the best practice was to allocate all the
> DMA memory when opening the device, reuse them during runtime and free
> everything when closing the device.

Yes, that is true, but as you are finding out here, doing
fire-and-forget urbs with coherent memory aren't the easiest to use.

Which makes me ask, why do you have to use this type of memory for this
driver?  Why is it special to require it?

> Especially, correct me if I am wrong, but isn't there a risk to
> trigger below warning if calling usb_free_coherent() in an IRQ context
> (which is the case in the completion function)?
> https://elixir.bootlin.com/linux/v5.18/source/kernel/dma/mapping.c#L526

Ah, so your idea here wouldn't work either as if you did a
fire-and-forget urb with this type of buffer, the memory would be freed
in the urb callback in irq context.

So this might not be the interface you want to use at all here.

thanks,

greg k-h

RE: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[David Laight]

From: Vincent MAILHOL
> Sent: 21 June 2022 16:56
> 
> On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > allocated length and urb::actual_length was what was actually being
> > > transferred. Right now, I am just confused. Seems that I need to study
> > > a bit more and understand the real purpose of
> > > urb::transfer_buffer_length because I still fail to understand in
> > > which situation this can be different from the allocated length.
> >
> > urb->transfer_buffer_length is the amount of data that the driver wants
> > to send or expects to receive.  urb->actual_length is the amount of data
> > that was actually sent or actually received.
> >
> > Neither of these values has to be the same as the size of the buffer --
> > but they better not be bigger!
> 
> Thanks. Now things are a bit clearer.
> I guess that for the outcoming URB what I proposed made no sense. For
> incoming URB, I guess that most of the drivers want to set
> urb::transfer_buffer once for all with the allocated size and never
> touch it again.
> 
> Maybe the patch only makes sense of the incoming URB. Would it make
> sense to keep it but with an additional check to trigger a dmesg
> warning if this is used on an outcoming endpoint and with additional
> comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> the allocated size?

IIRC urb are pretty big.
You'd be unlucky if adding an extra field to hold the allocated
size would ever need more memory.
So it might just be worth saving the allocated size.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Greg Kroah-Hartman]

On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> From: Vincent MAILHOL
> > Sent: 21 June 2022 16:56
> > 
> > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > allocated length and urb::actual_length was what was actually being
> > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > a bit more and understand the real purpose of
> > > > urb::transfer_buffer_length because I still fail to understand in
> > > > which situation this can be different from the allocated length.
> > >
> > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > to send or expects to receive.  urb->actual_length is the amount of data
> > > that was actually sent or actually received.
> > >
> > > Neither of these values has to be the same as the size of the buffer --
> > > but they better not be bigger!
> > 
> > Thanks. Now things are a bit clearer.
> > I guess that for the outcoming URB what I proposed made no sense. For
> > incoming URB, I guess that most of the drivers want to set
> > urb::transfer_buffer once for all with the allocated size and never
> > touch it again.
> > 
> > Maybe the patch only makes sense of the incoming URB. Would it make
> > sense to keep it but with an additional check to trigger a dmesg
> > warning if this is used on an outcoming endpoint and with additional
> > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > the allocated size?
> 
> IIRC urb are pretty big.

What exactly do you mean by "pretty big" here?  And what is wrong with
that, I have never seen any issues with the current size of that
structure in any benchmark or performance results.  All USB bottlenecks
that I know of are either in the hardware layer, or in the protocol
layer itself (i.e. usb-storage protocol).

> You'd be unlucky if adding an extra field to hold the allocated
> size would ever need more memory.
> So it might just be worth saving the allocated size.

Maybe, yes, then we could transition to allocating the urb and buffer at
the same time like we do partially for iso streams in an urb.  But that
still might be overkill for just this one driver.  I'm curious as to why
a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
its buffers in the first place.

thanks,

greg k-h

RE: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[David Laight]

From: Greg Kroah-Hartman
> Sent: 22 June 2022 10:41
...
> > IIRC urb are pretty big.
> 
> What exactly do you mean by "pretty big" here?

Maybe 100-200 bytes?

> And what is wrong with
> that, I have never seen any issues with the current size of that
> structure in any benchmark or performance results.

Nothing really, the cost of allocating a sub-page structure
is pretty much independent of its size.
What I really meant is it isn't (say) 32 bytes where adding
another 4 could be a significant increase.

> All USB bottlenecks
> that I know of are either in the hardware layer, or in the protocol
> layer itself (i.e. usb-storage protocol).

There is a bufferbloat issue for usbnet on XHCI.
It would be better to fill the ring with (probably) 4k buffers
and have something sort out the USB frames from the buffers
and then get the network driver to use (IIRC) build_skb
to generate the skb from the data fragments.

I was only using 100M last time I was testing that and
didn't get performance issues.
Just problems with the USB connections 'bouncing', that
project got canned for other reasons ...

But at higher speeds and high network use it might all
be a problem.

> 
> > You'd be unlucky if adding an extra field to hold the allocated
> > size would ever need more memory.
> > So it might just be worth saving the allocated size.
> 
> Maybe, yes, then we could transition to allocating the urb and buffer at
> the same time like we do partially for iso streams in an urb.  But that
> still might be overkill for just this one driver.  I'm curious as to why
> a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> its buffers in the first place.

Yes, being able to do short transfers from buffer space in the urb
would save all the issues about having to allocate an extra
buffer to avoid DMA from stack.

Indeed for XHCI there is a bit that allows 8 bytes of data to
replace the pointer in the ring structure itself.
I don't remember the driver every doing that.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Oliver Neukum]

On 22.06.22 12:03, David Laight wrote:
> Yes, being able to do short transfers from buffer space in the urb
> would save all the issues about having to allocate an extra
> buffer to avoid DMA from stack.

That is just as hard to do for DMA coherency.

> Indeed for XHCI there is a bit that allows 8 bytes of data to
> replace the pointer in the ring structure itself.
> I don't remember the driver every doing that.

XHCI does use it. There was even a bug with endianness.

	Regards
		Oliver

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > From: Vincent MAILHOL
> > > Sent: 21 June 2022 16:56
> > >
> > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > allocated length and urb::actual_length was what was actually being
> > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > a bit more and understand the real purpose of
> > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > which situation this can be different from the allocated length.
> > > >
> > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > that was actually sent or actually received.
> > > >
> > > > Neither of these values has to be the same as the size of the buffer --
> > > > but they better not be bigger!
> > >
> > > Thanks. Now things are a bit clearer.
> > > I guess that for the outcoming URB what I proposed made no sense. For
> > > incoming URB, I guess that most of the drivers want to set
> > > urb::transfer_buffer once for all with the allocated size and never
> > > touch it again.
> > >
> > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > sense to keep it but with an additional check to trigger a dmesg
> > > warning if this is used on an outcoming endpoint and with additional
> > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > the allocated size?
> >
> > IIRC urb are pretty big.
>
> What exactly do you mean by "pretty big" here?  And what is wrong with
> that, I have never seen any issues with the current size of that
> structure in any benchmark or performance results.  All USB bottlenecks
> that I know of are either in the hardware layer, or in the protocol
> layer itself (i.e. usb-storage protocol).
>
> > You'd be unlucky if adding an extra field to hold the allocated
> > size would ever need more memory.
> > So it might just be worth saving the allocated size.
>
> Maybe, yes, then we could transition to allocating the urb and buffer at
> the same time like we do partially for iso streams in an urb.  But that
> still might be overkill for just this one driver.

Well, I wouldn't have proposed the patch if it only applied to a
single driver. If we add a urb::allocated_transfer_size as suggested
by David, I believe that the majority of the drivers using DMA memory
will be able to rely on that URB_FREE_COHERENT flag for the garbage
collection.

The caveat, as you pointed before, is that the developper still needs
to be aware of the limitations of DMA and that it should not be freed
in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
functions that would lead to urb_destroy().

> I'm curious as to why
> a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> its buffers in the first place.

The CAN protocol, in its latest revision, allows for transfer speed up
to ~5Mbits. For low performance CPUs, this starts to be a significant
load. Also, the CAN PDU being small (0 to 64 bytes), many small
transfers occur.
Unfortunately I did not do any benchmark myself so I won't be able to
back my explanation with numbers.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Greg Kroah-Hartman]

On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > From: Vincent MAILHOL
> > > > Sent: 21 June 2022 16:56
> > > >
> > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > > allocated length and urb::actual_length was what was actually being
> > > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > > a bit more and understand the real purpose of
> > > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > > which situation this can be different from the allocated length.
> > > > >
> > > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > > that was actually sent or actually received.
> > > > >
> > > > > Neither of these values has to be the same as the size of the buffer --
> > > > > but they better not be bigger!
> > > >
> > > > Thanks. Now things are a bit clearer.
> > > > I guess that for the outcoming URB what I proposed made no sense. For
> > > > incoming URB, I guess that most of the drivers want to set
> > > > urb::transfer_buffer once for all with the allocated size and never
> > > > touch it again.
> > > >
> > > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > > sense to keep it but with an additional check to trigger a dmesg
> > > > warning if this is used on an outcoming endpoint and with additional
> > > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > > the allocated size?
> > >
> > > IIRC urb are pretty big.
> >
> > What exactly do you mean by "pretty big" here?  And what is wrong with
> > that, I have never seen any issues with the current size of that
> > structure in any benchmark or performance results.  All USB bottlenecks
> > that I know of are either in the hardware layer, or in the protocol
> > layer itself (i.e. usb-storage protocol).
> >
> > > You'd be unlucky if adding an extra field to hold the allocated
> > > size would ever need more memory.
> > > So it might just be worth saving the allocated size.
> >
> > Maybe, yes, then we could transition to allocating the urb and buffer at
> > the same time like we do partially for iso streams in an urb.  But that
> > still might be overkill for just this one driver.
> 
> Well, I wouldn't have proposed the patch if it only applied to a
> single driver. If we add a urb::allocated_transfer_size as suggested
> by David, I believe that the majority of the drivers using DMA memory
> will be able to rely on that URB_FREE_COHERENT flag for the garbage
> collection.
> 
> The caveat, as you pointed before, is that the developper still needs
> to be aware of the limitations of DMA and that it should not be freed
> in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> functions that would lead to urb_destroy().
> 
> > I'm curious as to why
> > a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> > its buffers in the first place.
> 
> The CAN protocol, in its latest revision, allows for transfer speed up
> to ~5Mbits. For low performance CPUs, this starts to be a significant
> load. Also, the CAN PDU being small (0 to 64 bytes), many small
> transfers occur.

And is the memcpy the actual issue here?  Even tiny cpus can do large
and small memcopy very very very fast.

> Unfortunately I did not do any benchmark myself so I won't be able to
> back my explanation with numbers.

That might be the simplest solution here :)

thanks,

greg k-h

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> > On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > > From: Vincent MAILHOL
> > > > > Sent: 21 June 2022 16:56
> > > > >
> > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > > > allocated length and urb::actual_length was what was actually being
> > > > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > > > a bit more and understand the real purpose of
> > > > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > > > which situation this can be different from the allocated length.
> > > > > >
> > > > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > > > that was actually sent or actually received.
> > > > > >
> > > > > > Neither of these values has to be the same as the size of the buffer --
> > > > > > but they better not be bigger!
> > > > >
> > > > > Thanks. Now things are a bit clearer.
> > > > > I guess that for the outcoming URB what I proposed made no sense. For
> > > > > incoming URB, I guess that most of the drivers want to set
> > > > > urb::transfer_buffer once for all with the allocated size and never
> > > > > touch it again.
> > > > >
> > > > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > > > sense to keep it but with an additional check to trigger a dmesg
> > > > > warning if this is used on an outcoming endpoint and with additional
> > > > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > > > the allocated size?
> > > >
> > > > IIRC urb are pretty big.
> > >
> > > What exactly do you mean by "pretty big" here?  And what is wrong with
> > > that, I have never seen any issues with the current size of that
> > > structure in any benchmark or performance results.  All USB bottlenecks
> > > that I know of are either in the hardware layer, or in the protocol
> > > layer itself (i.e. usb-storage protocol).
> > >
> > > > You'd be unlucky if adding an extra field to hold the allocated
> > > > size would ever need more memory.
> > > > So it might just be worth saving the allocated size.
> > >
> > > Maybe, yes, then we could transition to allocating the urb and buffer at
> > > the same time like we do partially for iso streams in an urb.  But that
> > > still might be overkill for just this one driver.
> >
> > Well, I wouldn't have proposed the patch if it only applied to a
> > single driver. If we add a urb::allocated_transfer_size as suggested
> > by David, I believe that the majority of the drivers using DMA memory
> > will be able to rely on that URB_FREE_COHERENT flag for the garbage
> > collection.
> >
> > The caveat, as you pointed before, is that the developper still needs
> > to be aware of the limitations of DMA and that it should not be freed
> > in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> > functions that would lead to urb_destroy().
> >
> > > I'm curious as to why
> > > a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> > > its buffers in the first place.
> >
> > The CAN protocol, in its latest revision, allows for transfer speed up
> > to ~5Mbits. For low performance CPUs, this starts to be a significant
> > load. Also, the CAN PDU being small (0 to 64 bytes), many small
> > transfers occur.
>
> And is the memcpy the actual issue here?  Even tiny cpus can do large
> and small memcopy very very very fast.
>
> > Unfortunately I did not do any benchmark myself so I won't be able to
> > back my explanation with numbers.
>
> That might be the simplest solution here :)

Yes, this would give a clear answer whether or not DMA was needed in
the first place. But I do not own that gs_usb device to do the
benchmark myself (and to be honest I do not have time to dedicate for
this at the moment, maybe I will do it later on some other devices).

Has anyone from the linux-can mailing list ever done such a benchmark?
Else, is there anyone who would like to volunteer?


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Rhett Aultman]

On Thu, 23 Jun 2022, Vincent MAILHOL wrote:

> On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> > > On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > > > From: Vincent MAILHOL
> > > > > > Sent: 21 June 2022 16:56
> > > > > >
> > > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > > > > allocated length and urb::actual_length was what was actually being
> > > > > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > > > > a bit more and understand the real purpose of
> > > > > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > > > > which situation this can be different from the allocated length.
> > > > > > >
> > > > > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > > > > that was actually sent or actually received.
> > > > > > >
> > > > > > > Neither of these values has to be the same as the size of the buffer --
> > > > > > > but they better not be bigger!
> > > > > >
> > > > > > Thanks. Now things are a bit clearer.
> > > > > > I guess that for the outcoming URB what I proposed made no sense. For
> > > > > > incoming URB, I guess that most of the drivers want to set
> > > > > > urb::transfer_buffer once for all with the allocated size and never
> > > > > > touch it again.
> > > > > >
> > > > > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > > > > sense to keep it but with an additional check to trigger a dmesg
> > > > > > warning if this is used on an outcoming endpoint and with additional
> > > > > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > > > > the allocated size?
> > > > >
> > > > > IIRC urb are pretty big.
> > > >
> > > > What exactly do you mean by "pretty big" here?  And what is wrong with
> > > > that, I have never seen any issues with the current size of that
> > > > structure in any benchmark or performance results.  All USB bottlenecks
> > > > that I know of are either in the hardware layer, or in the protocol
> > > > layer itself (i.e. usb-storage protocol).
> > > >
> > > > > You'd be unlucky if adding an extra field to hold the allocated
> > > > > size would ever need more memory.
> > > > > So it might just be worth saving the allocated size.
> > > >
> > > > Maybe, yes, then we could transition to allocating the urb and buffer at
> > > > the same time like we do partially for iso streams in an urb.  But that
> > > > still might be overkill for just this one driver.
> > >
> > > Well, I wouldn't have proposed the patch if it only applied to a
> > > single driver. If we add a urb::allocated_transfer_size as suggested
> > > by David, I believe that the majority of the drivers using DMA memory
> > > will be able to rely on that URB_FREE_COHERENT flag for the garbage
> > > collection.
> > >
> > > The caveat, as you pointed before, is that the developper still needs
> > > to be aware of the limitations of DMA and that it should not be freed
> > > in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> > > functions that would lead to urb_destroy().
> > >
> > > > I'm curious as to why
> > > > a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> > > > its buffers in the first place.
> > >
> > > The CAN protocol, in its latest revision, allows for transfer speed up
> > > to ~5Mbits. For low performance CPUs, this starts to be a significant
> > > load. Also, the CAN PDU being small (0 to 64 bytes), many small
> > > transfers occur.
> >
> > And is the memcpy the actual issue here?  Even tiny cpus can do large
> > and small memcopy very very very fast.
> >
> > > Unfortunately I did not do any benchmark myself so I won't be able to
> > > back my explanation with numbers.
> >
> > That might be the simplest solution here :)
>
> Yes, this would give a clear answer whether or not DMA was needed in
> the first place. But I do not own that gs_usb device to do the
> benchmark myself (and to be honest I do not have time to dedicate for
> this at the moment, maybe I will do it later on some other devices).
>
> Has anyone from the linux-can mailing list ever done such a benchmark?
> Else, is there anyone who would like to volunteer?

I have access to a couple of gs_usb devices but I am afraid I have no
experience performing this sort of benchmarking and also would have to
squeeze it in as a weekend project or something similar.  That said, if
someone's willing to help step me through it, I can see if it's feasible
for me to do.

That said, the gs_usb driver is mostly following along a very well
established pattern for writing USB CAN devices.  Both the pattern
followed that created the memory leak, as well as the pattern I followed
to resolve the memory leak, were also seen in the esd2 USB CAN driver as
well, and likely others are following suit.  So, I don't know that we'd
need to keep it specific to gs_usb to gain good information here.

Best,
Rhett

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Thu. 23 Jun 2022 at 03:13, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> On Thu, 23 Jun 2022, Vincent MAILHOL wrote:
> > On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> > > > On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > > > > From: Vincent MAILHOL
> > > > > > > Sent: 21 June 2022 16:56
> > > > > > >
> > > > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > > > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > > > > > I (probably wrongly) assumed that urb::transfer_buffer_length was the
> > > > > > > > > allocated length and urb::actual_length was what was actually being
> > > > > > > > > transferred. Right now, I am just confused. Seems that I need to study
> > > > > > > > > a bit more and understand the real purpose of
> > > > > > > > > urb::transfer_buffer_length because I still fail to understand in
> > > > > > > > > which situation this can be different from the allocated length.
> > > > > > > >
> > > > > > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > > > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > > > > > that was actually sent or actually received.
> > > > > > > >
> > > > > > > > Neither of these values has to be the same as the size of the buffer --
> > > > > > > > but they better not be bigger!
> > > > > > >
> > > > > > > Thanks. Now things are a bit clearer.
> > > > > > > I guess that for the outcoming URB what I proposed made no sense. For
> > > > > > > incoming URB, I guess that most of the drivers want to set
> > > > > > > urb::transfer_buffer once for all with the allocated size and never
> > > > > > > touch it again.
> > > > > > >
> > > > > > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > > > > > sense to keep it but with an additional check to trigger a dmesg
> > > > > > > warning if this is used on an outcoming endpoint and with additional
> > > > > > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > > > > > the allocated size?
> > > > > >
> > > > > > IIRC urb are pretty big.
> > > > >
> > > > > What exactly do you mean by "pretty big" here?  And what is wrong with
> > > > > that, I have never seen any issues with the current size of that
> > > > > structure in any benchmark or performance results.  All USB bottlenecks
> > > > > that I know of are either in the hardware layer, or in the protocol
> > > > > layer itself (i.e. usb-storage protocol).
> > > > >
> > > > > > You'd be unlucky if adding an extra field to hold the allocated
> > > > > > size would ever need more memory.
> > > > > > So it might just be worth saving the allocated size.
> > > > >
> > > > > Maybe, yes, then we could transition to allocating the urb and buffer at
> > > > > the same time like we do partially for iso streams in an urb.  But that
> > > > > still might be overkill for just this one driver.
> > > >
> > > > Well, I wouldn't have proposed the patch if it only applied to a
> > > > single driver. If we add a urb::allocated_transfer_size as suggested
> > > > by David, I believe that the majority of the drivers using DMA memory
> > > > will be able to rely on that URB_FREE_COHERENT flag for the garbage
> > > > collection.
> > > >
> > > > The caveat, as you pointed before, is that the developper still needs
> > > > to be aware of the limitations of DMA and that it should not be freed
> > > > in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> > > > functions that would lead to urb_destroy().
> > > >
> > > > > I'm curious as to why
> > > > > a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> > > > > its buffers in the first place.
> > > >
> > > > The CAN protocol, in its latest revision, allows for transfer speed up
> > > > to ~5Mbits. For low performance CPUs, this starts to be a significant
> > > > load. Also, the CAN PDU being small (0 to 64 bytes), many small
> > > > transfers occur.
> > >
> > > And is the memcpy the actual issue here?  Even tiny cpus can do large
> > > and small memcopy very very very fast.
> > >
> > > > Unfortunately I did not do any benchmark myself so I won't be able to
> > > > back my explanation with numbers.
> > >
> > > That might be the simplest solution here :)
> >
> > Yes, this would give a clear answer whether or not DMA was needed in
> > the first place. But I do not own that gs_usb device to do the
> > benchmark myself (and to be honest I do not have time to dedicate for
> > this at the moment, maybe I will do it later on some other devices).
> >
> > Has anyone from the linux-can mailing list ever done such a benchmark?
> > Else, is there anyone who would like to volunteer?
>
> I have access to a couple of gs_usb devices but I am afraid I have no
> experience performing this sort of benchmarking and also would have to
> squeeze it in as a weekend project or something similar.  That said, if
> someone's willing to help step me through it, I can see if it's feasible
> for me to do.

I can throw a few hints which might be helpful.

First, you should obviously prepare two versions of the gs_usb driver:
one using usb_alloc_coherent() (the current one), the other using
kmalloc() and compare the two.

Right now, I can think of two relevant benchmarks: transmission
latency and CPU load.

For the transmission latency, I posted one on my tools:
https://lore.kernel.org/linux-can/20220626075317.746535-1-mailhol.vincent@wanadoo.fr/T/#u

For the CPU load, I suggest to put the bus on full load, for example using:
| cangen -g0 -p1 can0
(you might also want to play with other parameters such as the length using -L)
Then use an existing tool to get the CPU load figures. I don't know
for sure which tool is a good one to benchmark CPU usage in kernel
land so you will have to research that part. If anyone has a
suggestion…

> That said, the gs_usb driver is mostly following along a very well
> established pattern for writing USB CAN devices.  Both the pattern
> followed that created the memory leak, as well as the pattern I followed
> to resolve the memory leak, were also seen in the esd2 USB CAN driver as
> well, and likely others are following suit.  So, I don't know that we'd
> need to keep it specific to gs_usb to gain good information here.

Yes, I looked at the log, the very first CAN USB driver is ems_usb and
was using DMA memory from the beginning. From that point on, nearly
all the drivers copied the trend (the only exception I am aware of is
peak_usb).

I agree that the scope is wider than the gs_can (thus my proposal to
fix it at API level).


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Rhett Aultman]

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



On Sun, 26 Jun 2022, Vincent MAILHOL wrote:

> On Thu. 23 Jun 2022 at 03:13, Rhett Aultman <rhett.aultman@samsara.com> wrote:

> > On Thu, 23 Jun 2022, Vincent MAILHOL wrote:

> > > On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman

> > > Yes, this would give a clear answer whether or not DMA was needed in
> > > the first place. But I do not own that gs_usb device to do the
> > > benchmark myself (and to be honest I do not have time to dedicate for
> > > this at the moment, maybe I will do it later on some other devices).
> > >
> > > Has anyone from the linux-can mailing list ever done such a benchmark?
> > > Else, is there anyone who would like to volunteer?
> >
> > I have access to a couple of gs_usb devices but I am afraid I have no
> > experience performing this sort of benchmarking and also would have to
> > squeeze it in as a weekend project or something similar.  That said, if
> > someone's willing to help step me through it, I can see if it's feasible
> > for me to do.
>
> I can throw a few hints which might be helpful.
>
> First, you should obviously prepare two versions of the gs_usb driver:
> one using usb_alloc_coherent() (the current one), the other using
> kmalloc() and compare the two.
>
> Right now, I can think of two relevant benchmarks: transmission
> latency and CPU load.
>
> For the transmission latency, I posted one on my tools:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dcan_20220626075317.746535-2D1-2Dmailhol.vincent-40wanadoo.fr_T_-23u&d=DwIFaQ&c=5cz3ZESzsFPW6Kn30oD8Yg&r=yZeJccB4JMhCRfLQXCMV_s56v3-BAi0tMrD3qzCwGTk&m=E5qqM5zYANpQqfZ0c8AHYrd-lkJZsS6-u-Jj2iTfHIjLle6JxCMRuTlmC_3bH8oA&s=sqvGqOvbtLqlZGMC-9q6gY1nF3203MT7gJIIqbKEXUM&e=
>
> For the CPU load, I suggest to put the bus on full load, for example using:
> | cangen -g0 -p1 can0
> (you might also want to play with other parameters such as the length using -L)
> Then use an existing tool to get the CPU load figures. I don't know
> for sure which tool is a good one to benchmark CPU usage in kernel
> land so you will have to research that part. If anyone has a
> suggestion…
>
> > That said, the gs_usb driver is mostly following along a very well
> > established pattern for writing USB CAN devices.  Both the pattern
> > followed that created the memory leak, as well as the pattern I followed
> > to resolve the memory leak, were also seen in the esd2 USB CAN driver as
> > well, and likely others are following suit.  So, I don't know that we'd
> > need to keep it specific to gs_usb to gain good information here.
>
> Yes, I looked at the log, the very first CAN USB driver is ems_usb and
> was using DMA memory from the beginning. From that point on, nearly
> all the drivers copied the trend (the only exception I am aware of is
> peak_usb).
>
> I agree that the scope is wider than the gs_can (thus my proposal to
> fix it at API level).

(removed the USB mailing list since this is CAN driver related
specifically)

I appreciate these pointers and I can look into making the time for this.
As I mentioned, I do have a gs_usb device (a Canable using the Candlelight
firmware) which can help shed some light on this question.  I do
understand the ideas being expressed in these pointers.  I do want to
bring up some practical matters around it.

First, it seems there's a pretty strong set of permutations to consider,
given that this memory allocation scheme is common to so many drivers.  I
only have a gs_usb device (a Canable using its CandleLight firmware).  I
also cannot rule out the possibility that the underlying hardware of the
host matters here.  For example, I discovered this leak in the first place
because I work with a specific ARM platform where it's easy to exhaust the
DMA memory.

Secondly, this sort of benchmarking work will require lab setup time and
my locating adequate free time in which to do it.  This isn't exactly
labor covered under the original mandate from my employer, so I'm going to
have to figure out how to work it in.

In light of this, while I remain committed to helping work the problem, I
can't help but wonder if it's worth it to consider my original patch in a
new light?  The code is less elegant than it otherwise could be, but it's
consistent with practices found in the other drivers and it does resolve
the original issue of leaking DMA memory.  I'd hate to see a long-standing
issue continue to languish because I struggle to find adequate time to
devote to the benchmarking needed to reach a decision about the USB API
changes we've proposed.

Best,
Rhett

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Tue. 28 Jun 2022 at 04:37, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> On Sun, 26 Jun 2022, Vincent MAILHOL wrote:
> > On Thu. 23 Jun 2022 at 03:13, Rhett Aultman <rhett.aultman@samsara.com> wrote:
> > > On Thu, 23 Jun 2022, Vincent MAILHOL wrote:
> > > > On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
> > > > Yes, this would give a clear answer whether or not DMA was needed in
> > > > the first place. But I do not own that gs_usb device to do the
> > > > benchmark myself (and to be honest I do not have time to dedicate for
> > > > this at the moment, maybe I will do it later on some other devices).
> > > >
> > > > Has anyone from the linux-can mailing list ever done such a benchmark?
> > > > Else, is there anyone who would like to volunteer?
> > >
> > > I have access to a couple of gs_usb devices but I am afraid I have no
> > > experience performing this sort of benchmarking and also would have to
> > > squeeze it in as a weekend project or something similar.  That said, if
> > > someone's willing to help step me through it, I can see if it's feasible
> > > for me to do.
> >
> > I can throw a few hints which might be helpful.
> >
> > First, you should obviously prepare two versions of the gs_usb driver:
> > one using usb_alloc_coherent() (the current one), the other using
> > kmalloc() and compare the two.
> >
> > Right now, I can think of two relevant benchmarks: transmission
> > latency and CPU load.
> >
> > For the transmission latency, I posted one on my tools:
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__lore.kernel.org_linux-2Dcan_20220626075317.746535-2D1-2Dmailhol.vincent-40wanadoo.fr_T_-23u&d=DwIFaQ&c=5cz3ZESzsFPW6Kn30oD8Yg&r=yZeJccB4JMhCRfLQXCMV_s56v3-BAi0tMrD3qzCwGTk&m=E5qqM5zYANpQqfZ0c8AHYrd-lkJZsS6-u-Jj2iTfHIjLle6JxCMRuTlmC_3bH8oA&s=sqvGqOvbtLqlZGMC-9q6gY1nF3203MT7gJIIqbKEXUM&e=
> >
> > For the CPU load, I suggest to put the bus on full load, for example using:
> > | cangen -g0 -p1 can0
> > (you might also want to play with other parameters such as the length using -L)
> > Then use an existing tool to get the CPU load figures. I don't know
> > for sure which tool is a good one to benchmark CPU usage in kernel
> > land so you will have to research that part. If anyone has a
> > suggestion…
> >
> > > That said, the gs_usb driver is mostly following along a very well
> > > established pattern for writing USB CAN devices.  Both the pattern
> > > followed that created the memory leak, as well as the pattern I followed
> > > to resolve the memory leak, were also seen in the esd2 USB CAN driver as
> > > well, and likely others are following suit.  So, I don't know that we'd
> > > need to keep it specific to gs_usb to gain good information here.
> >
> > Yes, I looked at the log, the very first CAN USB driver is ems_usb and
> > was using DMA memory from the beginning. From that point on, nearly
> > all the drivers copied the trend (the only exception I am aware of is
> > peak_usb).
> >
> > I agree that the scope is wider than the gs_can (thus my proposal to
> > fix it at API level).
>
> (removed the USB mailing list since this is CAN driver related
> specifically)
>
> I appreciate these pointers and I can look into making the time for this.
> As I mentioned, I do have a gs_usb device (a Canable using the Candlelight
> firmware) which can help shed some light on this question.  I do
> understand the ideas being expressed in these pointers.  I do want to
> bring up some practical matters around it.
>
> First, it seems there's a pretty strong set of permutations to consider,
> given that this memory allocation scheme is common to so many drivers.  I
> only have a gs_usb device (a Canable using its CandleLight firmware).  I
> also cannot rule out the possibility that the underlying hardware of the
> host matters here.  For example, I discovered this leak in the first place
> because I work with a specific ARM platform where it's easy to exhaust the
> DMA memory.
>
> Secondly, this sort of benchmarking work will require lab setup time and
> my locating adequate free time in which to do it.  This isn't exactly
> labor covered under the original mandate from my employer, so I'm going to
> have to figure out how to work it in.

There is no rush. If this is interesting for you, go ahead, but I
won’t blame you if you prefer to give up for lack of time or
motivation.

> In light of this, while I remain committed to helping work the problem, I
> can't help but wonder if it's worth it to consider my original patch in a
> new light?

Yes, it makes sense to take your initial patch. I will reiterate that
I do not like the way it is done but you are fixing a memory leak and
delaying the fix furthermore is not good.

I am curious to see the benchmark results but at the same time, I do
not want to force anyone to do it.

If Marc agrees, I think we should just take your initial patch as is.
And later we can reconsider those two options:
  * apply the URB_FREE_COHERENT flag if the flag gets accepted (not
sure anymore that would be the case).
  * change from DMA memory to normal kmalloc()ed memory depending on
the benchmark result

Personally, I will try to push a bit more for the inclusion of the
URB_FREE_COHERENT flag.

> The code is less elegant than it otherwise could be, but it's
> consistent with practices found in the other drivers and it does resolve
> the original issue of leaking DMA memory.  I'd hate to see a long-standing
> issue continue to languish because I struggle to find adequate time to
> devote to the benchmarking needed to reach a decision about the USB API
> changes we've proposed.


Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Marc Kleine-Budde]

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

On 28.06.2022 10:09:16, Vincent MAILHOL wrote:
> > In light of this, while I remain committed to helping work the problem, I
> > can't help but wonder if it's worth it to consider my original patch in a
> > new light?
> 
> Yes, it makes sense to take your initial patch.

done

> I will reiterate that
> I do not like the way it is done but you are fixing a memory leak and
> delaying the fix furthermore is not good.

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 v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Rhett Aultman]

On Mon, 4 Jul 2022, Marc Kleine-Budde wrote:

> On 28.06.2022 10:09:16, Vincent MAILHOL wrote:
> > > In light of this, while I remain committed to helping work the problem, I
> > > can't help but wonder if it's worth it to consider my original patch in a
> > > new light?
> >
> > Yes, it makes sense to take your initial patch.
>
> done

Thanks.  At this point, is there anything more needed from me on that
patch?  Addition of Reviewed-by: lines or similar?

> > I will reiterate that
> > I do not like the way it is done but you are fixing a memory leak and
> > delaying the fix furthermore is not good.

As an addendum, I think I might have found a way to scare up resources
and time for that benchmarking work, so while I don't want to start making
promises, there's a reasonable chance we might be able to do this in the
next few weeks.

Best,
Rhett

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Marc Kleine-Budde]

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

On 04.07.2022 11:35:52, Rhett Aultman wrote:
> 
> 
> On Mon, 4 Jul 2022, Marc Kleine-Budde wrote:
> 
> > On 28.06.2022 10:09:16, Vincent MAILHOL wrote:
> > > > In light of this, while I remain committed to helping work the problem, I
> > > > can't help but wonder if it's worth it to consider my original patch in a
> > > > new light?
> > >
> > > Yes, it makes sense to take your initial patch.
> >
> > done
> 
> Thanks.  At this point, is there anything more needed from me on that
> patch?  Addition of Reviewed-by: lines or similar?

No need to, it's already mainline :)

> > > I will reiterate that
> > > I do not like the way it is done but you are fixing a memory leak and
> > > delaying the fix furthermore is not good.
> 
> As an addendum, I think I might have found a way to scare up resources
> and time for that benchmarking work, so while I don't want to start making
> promises, there's a reasonable chance we might be able to do this in the
> next few weeks.

Nice!

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 v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Hongren Zenithal Zheng]

On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote:
> 
> In order to have all the flags in numerical order, URB_DIR_IN is
> renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> value 0x0200.

>  #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
> +#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
>  
>  /* The following flags are used internally by usbcore and HCDs */
> -#define URB_DIR_IN		0x0200	/* Transfer from device to host */
> +#define URB_DIR_IN		0x0400	/* Transfer from device to host */
>  #define URB_DIR_OUT		0
>  #define URB_DIR_MASK		URB_DIR_IN
>  
> -- 
> 2.30.2
>

I'm afraid this is a change of uapi as this field is, unfortunately,
exported by usbip to userspace as TCP packets.

This may also cause incompatibility (surprisingly not for this case,
detailed below) between usbip server and client
when one kernel is using the new flags and the other one is not.

If we do change this, we may need to bump usbip protocol version
accordingly.

A copy of Alan Stern's suggestion here for reference
> I don't see anything wrong with this, except that it would be nice to keep 
> the flag values in numerical order.  In other words, set URB_FREE_COHERENT 
> to 0x0200 and change URB_DIR_IN to 0x0400.
> 
> Alan Stern

In usbip, an URB is packed by client (vhci)
into a TCP packet by usbip_pack_cmd_submit,
in which transfer_flags is copied almost as-is,
except it clears the DMA flag.
Then the server (usbip-host) would accept
the packet, mask some flags and
re-submit_urb to usbcore.

In usbip protocol, URB_DIR_IN is not used
as it has a `direction' field, the in-kernel
implementation (usbip-host) also does not
use it as when re-submit_urb the usb_submit_urb
will calculate this specific bit again.

For FREE_COHERENT, since DMA flag is
cleared, it does not affect the protocol
if both server and client are using the new version.

If we are using vhci in newer kernel and
the other side is using the old version,
the USB_FREE_COHERENT flag would be 0x0200,
and will be transmitted via TCP/IP to usbip-host
or a userspace implementation (there are many of them),
which will perceive it as URB_DIR_IN.
usbip-host is not affected, but userspace
program might be affected.

If we are using vhci in old kernel, and
the other side is using the new version,
all the URB_DIR_IN would be conceived by
usbip-host as USB_FREE_COHERENT.
Fortunately, it will be masked by
masking_bogus_flags in stub_rx.c,
and usbip-host will behave correctly,
but userspace program might be affected.

From the analysis above it turned
out that this change does not affect
in-kernel usbip implementation, but in a
quite tricky way.

One way to solve this issue for usbip
is to add some boilerplate transform
from URB_* to USBIP_FLAGS_*
as it is de facto uapi now.

Another way is to use 0x0400 for FREE_COHERENT.
usbip will not take care of this bit as
it would be masked.

Cc Shuah Khan here since she is the maintainer
on usbip.

Regards,

Hongren

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Shuah Khan]

On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote:
> On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote:
>>
>> In order to have all the flags in numerical order, URB_DIR_IN is
>> renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
>> value 0x0200.
> 
>>   #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
>> +#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
>>   
>>   /* The following flags are used internally by usbcore and HCDs */
>> -#define URB_DIR_IN		0x0200	/* Transfer from device to host */
>> +#define URB_DIR_IN		0x0400	/* Transfer from device to host */
>>   #define URB_DIR_OUT		0
>>   #define URB_DIR_MASK		URB_DIR_IN
>>   
>> -- 
>> 2.30.2
>>
> 
> I'm afraid this is a change of uapi as this field is, unfortunately,
> exported by usbip to userspace as TCP packets.
> 
> This may also cause incompatibility (surprisingly not for this case,
> detailed below) between usbip server and client
> when one kernel is using the new flags and the other one is not.
> 
> If we do change this, we may need to bump usbip protocol version
> accordingly.
> 


> A copy of Alan Stern's suggestion here for reference
>> I don't see anything wrong with this, except that it would be nice to keep
>> the flag values in numerical order.  In other words, set URB_FREE_COHERENT
>> to 0x0200 and change URB_DIR_IN to 0x0400.
>>
>> Alan Stern

Thank you Alan for this detailed analysis of uapi impacts and
usbip host side and vhci incompatibilities. Userspace is going
to be affected. In addition to the usbip tool in the kernel repo,
there are other versions floating around that would break if we
were to change the flags.

> One way to solve this issue for usbip
> is to add some boilerplate transform
> from URB_* to USBIP_FLAGS_*
> as it is de facto uapi now.

It doesn't sound like a there is a compelling reason other than
"it would be nice to keep the flag values in numerical order".

I would not recommend this option. I am not seeing any value to adding
change URB_* to USBIP_FLAGS_* layer without some serious techinical
concerns.

> 
> Another way is to use 0x0400 for FREE_COHERENT.
> usbip will not take care of this bit as
> it would be masked.
> 

I would go with this option adding a clear comment with link to this
discussion.

> Cc Shuah Khan here since she is the maintainer
> on usbip.
> 

Thank you adding me to the discussion.

thanks,
-- Shuah

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Alan Stern]

On Thu, Jun 23, 2022 at 11:45:13AM -0600, Shuah Khan wrote:
> On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote:
> > On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote:
> > > 
> > > In order to have all the flags in numerical order, URB_DIR_IN is
> > > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> > > value 0x0200.
> > 
> > >   #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
> > > +#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
> > >   /* The following flags are used internally by usbcore and HCDs */
> > > -#define URB_DIR_IN		0x0200	/* Transfer from device to host */
> > > +#define URB_DIR_IN		0x0400	/* Transfer from device to host */
> > >   #define URB_DIR_OUT		0
> > >   #define URB_DIR_MASK		URB_DIR_IN
> > > -- 
> > > 2.30.2
> > > 
> > 
> > I'm afraid this is a change of uapi as this field is, unfortunately,
> > exported by usbip to userspace as TCP packets.
> > 
> > This may also cause incompatibility (surprisingly not for this case,
> > detailed below) between usbip server and client
> > when one kernel is using the new flags and the other one is not.
> > 
> > If we do change this, we may need to bump usbip protocol version
> > accordingly.
> > 
> 
> 
> > A copy of Alan Stern's suggestion here for reference
> > > I don't see anything wrong with this, except that it would be nice to keep
> > > the flag values in numerical order.  In other words, set URB_FREE_COHERENT
> > > to 0x0200 and change URB_DIR_IN to 0x0400.
> > > 
> > > Alan Stern
> 
> Thank you Alan for this detailed analysis of uapi impacts and
> usbip host side and vhci incompatibilities. Userspace is going
> to be affected. In addition to the usbip tool in the kernel repo,
> there are other versions floating around that would break if we
> were to change the flags.
> 
> > One way to solve this issue for usbip
> > is to add some boilerplate transform
> > from URB_* to USBIP_FLAGS_*
> > as it is de facto uapi now.
> 
> It doesn't sound like a there is a compelling reason other than
> "it would be nice to keep the flag values in numerical order".
> 
> I would not recommend this option. I am not seeing any value to adding
> change URB_* to USBIP_FLAGS_* layer without some serious techinical
> concerns.
> 
> > 
> > Another way is to use 0x0400 for FREE_COHERENT.
> > usbip will not take care of this bit as
> > it would be masked.
> > 
> 
> I would go with this option adding a clear comment with link to this
> discussion.
> 
> > Cc Shuah Khan here since she is the maintainer
> > on usbip.
> > 
> 
> Thank you adding me to the discussion.

I can see this causing more problems in the future.  There's no hint in 
include/linux/usb.h that any of the values it defines are part of a user 
API.  If they are, they should be moved to include/uapi/linux/usb/.

In general, if a user program depends on kernel details that are not 
designed to be part of a user API, you should expect that the program 
will sometimes break from one kernel version to another.

Yes, I know Linus insists that kernel changes should not cause 
regressions in userspace, but the line has to be drawn somewhere.  
Otherwise the kernel could never change at all.

Alan Stern

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Hongren Zenithal Zheng]

On Fri, Jun 24, 2022 at 10:43:34AM -0400, Alan Stern wrote:
> > 
> > > One way to solve this issue for usbip
> > > is to add some boilerplate transform
> > > from URB_* to USBIP_FLAGS_*
> > > as it is de facto uapi now.
> > 
> > It doesn't sound like a there is a compelling reason other than
> > "it would be nice to keep the flag values in numerical order".
> > 
> > I would not recommend this option. I am not seeing any value to adding
> > change URB_* to USBIP_FLAGS_* layer without some serious techinical
> > concerns.

The transfer_flag in usbip is de facto uapi,
That's why I'm proposing the USBIP_FLAGS_* way and
further more I think usbip could move some flags/structs
in usbip_common.h to include/uapi/linux/usb/usbip.h,
instead of the userspace copying them into their own
header.

I will start a new thread if Shuah think that is acceptable.

If this patch is to be landed, I think it should be
landed along with the usbip change so there would be no
userspace change;

Even without this patch, making usbip flags/structs uapi alone
is still worth doing.

> > 
> > > 
> > > Another way is to use 0x0400 for FREE_COHERENT.
> > > usbip will not take care of this bit as
> > > it would be masked.
> > > 
> > 
> > I would go with this option adding a clear comment with link to this
> > discussion.
> > 
> > > Cc Shuah Khan here since she is the maintainer
> > > on usbip.
> > > 
> > 
> > Thank you adding me to the discussion.
> 
> I can see this causing more problems in the future.  There's no hint in 
> include/linux/usb.h that any of the values it defines are part of a user 
> API.  If they are, they should be moved to include/uapi/linux/usb/.

I agree with this argument.

> 
> In general, if a user program depends on kernel details that are not 
> designed to be part of a user API, you should expect that the program 
> will sometimes break from one kernel version to another.
> 
> Yes, I know Linus insists that kernel changes should not cause 
> regressions in userspace, but the line has to be drawn somewhere.  
> Otherwise the kernel could never change at all.
> 
> Alan Stern

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Shuah Khan]

On 6/24/22 8:43 AM, Alan Stern wrote:
> On Thu, Jun 23, 2022 at 11:45:13AM -0600, Shuah Khan wrote:
>> On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote:
>>> On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote:
>>>>
>>>> In order to have all the flags in numerical order, URB_DIR_IN is
>>>> renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
>>>> value 0x0200.
>>>
>>>>    #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
>>>> +#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
>>>>    /* The following flags are used internally by usbcore and HCDs */
>>>> -#define URB_DIR_IN		0x0200	/* Transfer from device to host */
>>>> +#define URB_DIR_IN		0x0400	/* Transfer from device to host */
>>>>    #define URB_DIR_OUT		0
>>>>    #define URB_DIR_MASK		URB_DIR_IN
>>>> -- 
>>>> 2.30.2
>>>>
>>>
>>> I'm afraid this is a change of uapi as this field is, unfortunately,
>>> exported by usbip to userspace as TCP packets.
>>>
>>> This may also cause incompatibility (surprisingly not for this case,
>>> detailed below) between usbip server and client
>>> when one kernel is using the new flags and the other one is not.
>>>
>>> If we do change this, we may need to bump usbip protocol version
>>> accordingly.
>>>
>>
>>
>>> A copy of Alan Stern's suggestion here for reference
>>>> I don't see anything wrong with this, except that it would be nice to keep
>>>> the flag values in numerical order.  In other words, set URB_FREE_COHERENT
>>>> to 0x0200 and change URB_DIR_IN to 0x0400.
>>>>
>>>> Alan Stern
>>
>> Thank you Alan for this detailed analysis of uapi impacts and
>> usbip host side and vhci incompatibilities. Userspace is going
>> to be affected. In addition to the usbip tool in the kernel repo,
>> there are other versions floating around that would break if we
>> were to change the flags.
>>
>>> One way to solve this issue for usbip
>>> is to add some boilerplate transform
>>> from URB_* to USBIP_FLAGS_*
>>> as it is de facto uapi now.
>>
>> It doesn't sound like a there is a compelling reason other than
>> "it would be nice to keep the flag values in numerical order".
>>
>> I would not recommend this option. I am not seeing any value to adding
>> change URB_* to USBIP_FLAGS_* layer without some serious techinical
>> concerns.
>>
>>>
>>> Another way is to use 0x0400 for FREE_COHERENT.
>>> usbip will not take care of this bit as
>>> it would be masked.
>>>
>>
>> I would go with this option adding a clear comment with link to this
>> discussion.
>>
>>> Cc Shuah Khan here since she is the maintainer
>>> on usbip.
>>>
>>
>> Thank you adding me to the discussion.
> 
> I can see this causing more problems in the future.  There's no hint in
> include/linux/usb.h that any of the values it defines are part of a user
> API.  If they are, they should be moved to include/uapi/linux/usb/.
> 

Please elaborate on more problems in the future.

> In general, if a user program depends on kernel details that are not
> designed to be part of a user API, you should expect that the program
> will sometimes break from one kernel version to another.
> 
> Yes, I know Linus insists that kernel changes should not cause
> regressions in userspace, but the line has to be drawn somewhere.
> Otherwise the kernel could never change at all.
> 

I have had to change the usbip sysfs interface api in the past to
address security bugs related to information leaks. I am not saying
no. I am asking if there is a good reason to do this. So far I haven't
heard one.

thanks,
-- Shuah

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Alan Stern]

On Fri, Jun 24, 2022 at 10:31:06AM -0600, Shuah Khan wrote:
> On 6/24/22 8:43 AM, Alan Stern wrote:
> > > It doesn't sound like a there is a compelling reason other than
> > > "it would be nice to keep the flag values in numerical order".
> > > 
> > > I would not recommend this option. I am not seeing any value to adding
> > > change URB_* to USBIP_FLAGS_* layer without some serious techinical
> > > concerns.
> > > 
> > > > 
> > > > Another way is to use 0x0400 for FREE_COHERENT.
> > > > usbip will not take care of this bit as
> > > > it would be masked.
> > > > 
> > > 
> > > I would go with this option adding a clear comment with link to this
> > > discussion.
> > > 
> > > > Cc Shuah Khan here since she is the maintainer
> > > > on usbip.
> > > > 
> > > 
> > > Thank you adding me to the discussion.
> > 
> > I can see this causing more problems in the future.  There's no hint in
> > include/linux/usb.h that any of the values it defines are part of a user
> > API.  If they are, they should be moved to include/uapi/linux/usb/.
> > 
> 
> Please elaborate on more problems in the future.

In the future people will want to make other changes to 
include/linux/usb.h and they will not be aware that those changes will 
adversely affect usbip, because there is no documentation saying that 
the values defined in usb.h are part of a user API.  That will be a 
problem, because those changes may be serious and important ones, not 
just decorative or stylistic as in this case.

> > In general, if a user program depends on kernel details that are not
> > designed to be part of a user API, you should expect that the program
> > will sometimes break from one kernel version to another.
> > 
> > Yes, I know Linus insists that kernel changes should not cause
> > regressions in userspace, but the line has to be drawn somewhere.
> > Otherwise the kernel could never change at all.
> > 
> 
> I have had to change the usbip sysfs interface api in the past to
> address security bugs related to information leaks. I am not saying
> no. I am asking if there is a good reason to do this. So far I haven't
> heard one.

I agree with Hongren that values defined in include/linux/ should not be 
part of a user API.  There are two choices:

	Move the definitions into include/uapi/linux/, or

	Add code to translate the values between the numbers used in 
	userspace and the numbers used in the kernel.  (This is what
	was done for urb->transfer_flags in devio.c:proc_do_submiturb() 
	near line 1862.)

Alan Stern

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Shuah Khan]

On 6/24/22 12:07 PM, Alan Stern wrote:
> On Fri, Jun 24, 2022 at 10:31:06AM -0600, Shuah Khan wrote:
>> On 6/24/22 8:43 AM, Alan Stern wrote:
>>>> It doesn't sound like a there is a compelling reason other than
>>>> "it would be nice to keep the flag values in numerical order".
>>>>
>>>> I would not recommend this option. I am not seeing any value to adding
>>>> change URB_* to USBIP_FLAGS_* layer without some serious techinical
>>>> concerns.
>>>>
>>>>>
>>>>> Another way is to use 0x0400 for FREE_COHERENT.
>>>>> usbip will not take care of this bit as
>>>>> it would be masked.
>>>>>
>>>>
>>>> I would go with this option adding a clear comment with link to this
>>>> discussion.
>>>>
>>>>> Cc Shuah Khan here since she is the maintainer
>>>>> on usbip.
>>>>>
>>>>
>>>> Thank you adding me to the discussion.
>>>
>>> I can see this causing more problems in the future.  There's no hint in
>>> include/linux/usb.h that any of the values it defines are part of a user
>>> API.  If they are, they should be moved to include/uapi/linux/usb/.
>>>
>>
>> Please elaborate on more problems in the future.
> 
> In the future people will want to make other changes to
> include/linux/usb.h and they will not be aware that those changes will
> adversely affect usbip, because there is no documentation saying that
> the values defined in usb.h are part of a user API.  That will be a
> problem, because those changes may be serious and important ones, not
> just decorative or stylistic as in this case.
> 

How often do these values change based on our past experience with these
fields?

>>> In general, if a user program depends on kernel details that are not
>>> designed to be part of a user API, you should expect that the program
>>> will sometimes break from one kernel version to another.
>>>
>>> Yes, I know Linus insists that kernel changes should not cause
>>> regressions in userspace, but the line has to be drawn somewhere.
>>> Otherwise the kernel could never change at all.
>>>
>>
>> I have had to change the usbip sysfs interface api in the past to
>> address security bugs related to information leaks. I am not saying
>> no. I am asking if there is a good reason to do this. So far I haven't
>> heard one.
> 
> I agree with Hongren that values defined in include/linux/ should not be
> part of a user API.  There are two choices:
> 

I agree with this in general. I don't think this is an explicit decision
to make them part of API. It is a consequence of simply copying the
transfer_flags. I am with you both on not being able to recognize the
impact until as this is rather obscure usage hidden away in the packets.
These defines aren't directly referenced.

> 	Move the definitions into include/uapi/linux/, or
> 

Wouldn't this be easier way to handle the change? With this option
the uapi will be well documented.

> 	Add code to translate the values between the numbers used in
> 	userspace and the numbers used in the kernel.  (This is what
> 	was done for urb->transfer_flags in devio.c:proc_do_submiturb()
> 	near line 1862.)
> 

I looked at the code and looks simple enough. I am okay going this route
if we see issues with the option 1.

thanks,
-- Shuah

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Alan Stern]

On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
> On 6/24/22 12:07 PM, Alan Stern wrote:
> > In the future people will want to make other changes to
> > include/linux/usb.h and they will not be aware that those changes will
> > adversely affect usbip, because there is no documentation saying that
> > the values defined in usb.h are part of a user API.  That will be a
> > problem, because those changes may be serious and important ones, not
> > just decorative or stylistic as in this case.
> > 
> 
> How often do these values change based on our past experience with these
> fields?

I don't know.  You could check the git history to find out for certain.  
My guess would be every eight or ten years.

> > I agree with Hongren that values defined in include/linux/ should not be
> > part of a user API.  There are two choices:
> > 
> 
> I agree with this in general. I don't think this is an explicit decision
> to make them part of API. It is a consequence of simply copying the
> transfer_flags. I am with you both on not being able to recognize the
> impact until as this is rather obscure usage hidden away in the packets.
> These defines aren't directly referenced.
> 
> > 	Move the definitions into include/uapi/linux/, or
> > 
> 
> Wouldn't this be easier way to handle the change? With this option
> the uapi will be well documented.
> 
> > 	Add code to translate the values between the numbers used in
> > 	userspace and the numbers used in the kernel.  (This is what
> > 	was done for urb->transfer_flags in devio.c:proc_do_submiturb()
> > 	near line 1862.)
> > 
> 
> I looked at the code and looks simple enough. I am okay going this route
> if we see issues with the option 1.

It's up to you; either approach is okay with me.  However, I do think 
that the second option is a little better; I don't see any good reason 
why the kernel should be forced to use the same numeric values for these 
flags forever.  Especially since the only user program that needs to 
know them is usbip, which is fairly closely tied to the kernel; if there 
were more programs using those values then they would constitute a good 
reason for choosing the first option.

Alan Stern

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Shuah Khan]

On 6/27/22 7:35 PM, Alan Stern wrote:
> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
>> On 6/24/22 12:07 PM, Alan Stern wrote:
>>> In the future people will want to make other changes to
>>> include/linux/usb.h and they will not be aware that those changes will
>>> adversely affect usbip, because there is no documentation saying that
>>> the values defined in usb.h are part of a user API.  That will be a
>>> problem, because those changes may be serious and important ones, not
>>> just decorative or stylistic as in this case.
>>>
>>
>> How often do these values change based on our past experience with these
>> fields?
> 
> I don't know.  You could check the git history to find out for certain.
> My guess would be every eight or ten years.
> 
>>> I agree with Hongren that values defined in include/linux/ should not be
>>> part of a user API.  There are two choices:
>>>
>>
>> I agree with this in general. I don't think this is an explicit decision
>> to make them part of API. It is a consequence of simply copying the
>> transfer_flags. I am with you both on not being able to recognize the
>> impact until as this is rather obscure usage hidden away in the packets.
>> These defines aren't directly referenced.
>>
>>> 	Move the definitions into include/uapi/linux/, or
>>>
>>
>> Wouldn't this be easier way to handle the change? With this option
>> the uapi will be well documented.
>>
>>> 	Add code to translate the values between the numbers used in
>>> 	userspace and the numbers used in the kernel.  (This is what
>>> 	was done for urb->transfer_flags in devio.c:proc_do_submiturb()
>>> 	near line 1862.)
>>>
>>
>> I looked at the code and looks simple enough. I am okay going this route
>> if we see issues with the option 1.
> 
> It's up to you; either approach is okay with me.  However, I do think
> that the second option is a little better; I don't see any good reason
> why the kernel should be forced to use the same numeric values for these
> flags forever.  Especially since the only user program that needs to
> know them is usbip, which is fairly closely tied to the kernel; if there
> were more programs using those values then they would constitute a good
> reason for choosing the first option.
> 

Thank you Alan and Hongren for your help with this problem. Since there
are no changes to the flags for the time being, I am comfortable going
with the second option.

I will send a patch soon.

thanks,
-- Shuah

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Shuah Khan]

On 6/30/22 8:10 PM, Shuah Khan wrote:
> On 6/27/22 7:35 PM, Alan Stern wrote:
>> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
>>> On 6/24/22 12:07 PM, Alan Stern wrote:
>>>> In the future people will want to make other changes to
>>>> include/linux/usb.h and they will not be aware that those changes will
>>>> adversely affect usbip, because there is no documentation saying that
>>>> the values defined in usb.h are part of a user API.  That will be a
>>>> problem, because those changes may be serious and important ones, not
>>>> just decorative or stylistic as in this case.
>>>>
>>>
>>> How often do these values change based on our past experience with these
>>> fields?
>>
>> I don't know.  You could check the git history to find out for certain.
>> My guess would be every eight or ten years.
>>
>>>> I agree with Hongren that values defined in include/linux/ should not be
>>>> part of a user API.  There are two choices:
>>>>
>>>
>>> I agree with this in general. I don't think this is an explicit decision
>>> to make them part of API. It is a consequence of simply copying the
>>> transfer_flags. I am with you both on not being able to recognize the
>>> impact until as this is rather obscure usage hidden away in the packets.
>>> These defines aren't directly referenced.
>>>
>>>>     Move the definitions into include/uapi/linux/, or
>>>>
>>>
>>> Wouldn't this be easier way to handle the change? With this option
>>> the uapi will be well documented.
>>>
>>>>     Add code to translate the values between the numbers used in
>>>>     userspace and the numbers used in the kernel.  (This is what
>>>>     was done for urb->transfer_flags in devio.c:proc_do_submiturb()
>>>>     near line 1862.)
>>>>
>>>
>>> I looked at the code and looks simple enough. I am okay going this route
>>> if we see issues with the option 1.
>>
>> It's up to you; either approach is okay with me.  However, I do think
>> that the second option is a little better; I don't see any good reason
>> why the kernel should be forced to use the same numeric values for these
>> flags forever.  Especially since the only user program that needs to
>> know them is usbip, which is fairly closely tied to the kernel; if there
>> were more programs using those values then they would constitute a good
>> reason for choosing the first option.
>>
> 
> Thank you Alan and Hongren for your help with this problem. Since there
> are no changes to the flags for the time being, I am comfortable going
> with the second option.
> 
> I will send a patch soon.
> 

Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to
get this done sooner - summer vacations didn't cooperate.

Just an update that I haven't forgotten and it will taken care of.
thanks,
-- Shuah

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Vincent MAILHOL]

On Tue. 2 Aug. 2022 at 02:48, Shuah Khan <skhan@linuxfoundation.org> wrote:
> On 6/30/22 8:10 PM, Shuah Khan wrote:
> > On 6/27/22 7:35 PM, Alan Stern wrote:
> >> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
> >>> On 6/24/22 12:07 PM, Alan Stern wrote:
> >>>> In the future people will want to make other changes to
> >>>> include/linux/usb.h and they will not be aware that those changes will
> >>>> adversely affect usbip, because there is no documentation saying that
> >>>> the values defined in usb.h are part of a user API.  That will be a
> >>>> problem, because those changes may be serious and important ones, not
> >>>> just decorative or stylistic as in this case.
> >>>>
> >>>
> >>> How often do these values change based on our past experience with these
> >>> fields?
> >>
> >> I don't know.  You could check the git history to find out for certain.
> >> My guess would be every eight or ten years.
> >>
> >>>> I agree with Hongren that values defined in include/linux/ should not be
> >>>> part of a user API.  There are two choices:
> >>>>
> >>>
> >>> I agree with this in general. I don't think this is an explicit decision
> >>> to make them part of API. It is a consequence of simply copying the
> >>> transfer_flags. I am with you both on not being able to recognize the
> >>> impact until as this is rather obscure usage hidden away in the packets.
> >>> These defines aren't directly referenced.
> >>>
> >>>>     Move the definitions into include/uapi/linux/, or
> >>>>
> >>>
> >>> Wouldn't this be easier way to handle the change? With this option
> >>> the uapi will be well documented.
> >>>
> >>>>     Add code to translate the values between the numbers used in
> >>>>     userspace and the numbers used in the kernel.  (This is what
> >>>>     was done for urb->transfer_flags in devio.c:proc_do_submiturb()
> >>>>     near line 1862.)
> >>>>
> >>>
> >>> I looked at the code and looks simple enough. I am okay going this route
> >>> if we see issues with the option 1.
> >>
> >> It's up to you; either approach is okay with me.  However, I do think
> >> that the second option is a little better; I don't see any good reason
> >> why the kernel should be forced to use the same numeric values for these
> >> flags forever.  Especially since the only user program that needs to
> >> know them is usbip, which is fairly closely tied to the kernel; if there
> >> were more programs using those values then they would constitute a good
> >> reason for choosing the first option.
> >>
> >
> > Thank you Alan and Hongren for your help with this problem. Since there
> > are no changes to the flags for the time being, I am comfortable going
> > with the second option.
> >
> > I will send a patch soon.
> >
>
> Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to
> get this done sooner - summer vacations didn't cooperate.
>
> Just an update that I haven't forgotten and it will taken care of.
> thanks,

Thanks for keeping this under your radar. I also have on my TODO list
to send a new version of my patch to add the `URB_FREE_COHERENT' flag
but this time adding an `allocated_length' field to struct urb. I will
wait for your patch to go first. By the way, I will be out for summer
holiday for the next couple of weeks so I wasn't planning to submit
anything soon regardless.

Yours sincerely,
Vincent Mailhol

Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Shuah Khan]

On 8/1/22 12:28 PM, Vincent MAILHOL wrote:
> On Tue. 2 Aug. 2022 at 02:48, Shuah Khan <skhan@linuxfoundation.org> wrote:
>> On 6/30/22 8:10 PM, Shuah Khan wrote:
>>> On 6/27/22 7:35 PM, Alan Stern wrote:
>>>> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
>>>>> On 6/24/22 12:07 PM, Alan Stern wrote:
>>>>>> In the future people will want to make other changes to
>>>>>> include/linux/usb.h and they will not be aware that those changes will
>>>>>> adversely affect usbip, because there is no documentation saying that
>>>>>> the values defined in usb.h are part of a user API.  That will be a
>>>>>> problem, because those changes may be serious and important ones, not
>>>>>> just decorative or stylistic as in this case.
>>>>>>
>>>>>
>>>>> How often do these values change based on our past experience with these
>>>>> fields?
>>>>
>>>> I don't know.  You could check the git history to find out for certain.
>>>> My guess would be every eight or ten years.
>>>>
>>>>>> I agree with Hongren that values defined in include/linux/ should not be
>>>>>> part of a user API.  There are two choices:
>>>>>>
>>>>>
>>>>> I agree with this in general. I don't think this is an explicit decision
>>>>> to make them part of API. It is a consequence of simply copying the
>>>>> transfer_flags. I am with you both on not being able to recognize the
>>>>> impact until as this is rather obscure usage hidden away in the packets.
>>>>> These defines aren't directly referenced.
>>>>>
>>>>>>      Move the definitions into include/uapi/linux/, or
>>>>>>
>>>>>
>>>>> Wouldn't this be easier way to handle the change? With this option
>>>>> the uapi will be well documented.
>>>>>
>>>>>>      Add code to translate the values between the numbers used in
>>>>>>      userspace and the numbers used in the kernel.  (This is what
>>>>>>      was done for urb->transfer_flags in devio.c:proc_do_submiturb()
>>>>>>      near line 1862.)
>>>>>>
>>>>>
>>>>> I looked at the code and looks simple enough. I am okay going this route
>>>>> if we see issues with the option 1.
>>>>
>>>> It's up to you; either approach is okay with me.  However, I do think
>>>> that the second option is a little better; I don't see any good reason
>>>> why the kernel should be forced to use the same numeric values for these
>>>> flags forever.  Especially since the only user program that needs to
>>>> know them is usbip, which is fairly closely tied to the kernel; if there
>>>> were more programs using those values then they would constitute a good
>>>> reason for choosing the first option.
>>>>
>>>
>>> Thank you Alan and Hongren for your help with this problem. Since there
>>> are no changes to the flags for the time being, I am comfortable going
>>> with the second option.
>>>
>>> I will send a patch soon.
>>>
>>
>> Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to
>> get this done sooner - summer vacations didn't cooperate.
>>
>> Just an update that I haven't forgotten and it will taken care of.
>> thanks,
> 
> Thanks for keeping this under your radar. I also have on my TODO list
> to send a new version of my patch to add the `URB_FREE_COHERENT' flag
> but this time adding an `allocated_length' field to struct urb. I will
> wait for your patch to go first. By the way, I will be out for summer
> holiday for the next couple of weeks so I wasn't planning to submit
> anything soon regardless.
> 

Sounds good. I now have the patch ready to be sent out. I will wait for
the merge window to close before I send it out.

thanks,
-- Shuah

[PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close

[Rhett Aultman]

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;
 
 			usb_anchor_urb(urb, &parent->rx_submitted);
 
-- 
2.30.2

Re: [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close

[Marc Kleine-Budde]

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

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

-- 
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 v3 2/2] can: gs_usb: fix DMA memory leak on close

[Vincent MAILHOL]

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 |

RE: [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close

[David Laight]

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)

Re: [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close

[Marc Kleine-Budde]

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

On 12.06.2022 21:28:06, David Laight wrote:
> 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?

This chance is intentional, Vincent suggested that in

| https://lore.kernel.org/all/CAMZ6RqKwvSswxThiKqEB8VhD5MyHvRbSwO_9-ZNwLgmnm-0iBw@mail.gmail.com

I think there are no other flags set, yet.

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

[PATCH v4 0/1] URB_FREE_COHERENT gs_usb memory leak fix

[Rhett Aultman]

This patchset resolves a memory leak which can occur with successive
iterations of calling gs_can_open() and gs_can_close().  The central
cause of this memory leak, which is an issue common to many of the USB
CAN drivers, is that memory allocated for RX buffers using
usb_alloc_coherent() and then kept in the URB will be properly freed
when the URB is killed.  This assumption is incorrect, as memory
allocated with usb_alloc_coherent() must be freed with
usb_free_coherent(), and there is no provision for this in the existing
URB code.

The common solution to this, found in v1 of my patches as well as in
already merged patches for other CAN USB drivers is for the driver
itself to maintain an array of addresses of the buffers allocated with
usb_alloc_coherent() and to then iteratively call usb_free_coherent() on
them in their close function.  This solution requires a driver developer
to understand this unclear nuance, and it has historically been solved
in a piecemeal way one driver at a time (note: the gs_usb driver has had
this issue since the 3.x.x kernel series).

Rather than continue to place the burden of complexity on the drivers,
this patchset adds a new URB flag which allows the DMA buffer to be
correctly freed with the URB is killed.  This results in a much simpler
solution at the driver level and with minimal additional code in the USB
core.

This v4 patch only updates the commit message for one of the patches in
the v3 patchset.  No code has changed.

This patch depends on:
https://lore.kernel.org/all/20220610213335.3077375-2-rhett.aultman@samsara.com

** Changelog **

v3 -> v4:

  * Add Fixes tag and correct formatting for reference to prior commit

v2 -> v3:

  * Resolve style nits in gs_usb.c
  * Squash commits in urb.c and correct authorship

v1 -> v2:

  * Add URB_FREE_COHERENT flag to urb.c
  * Use URB_FREE_COHERENT flag rather than arrays of buffers in gs_usb.c


Rhett Aultman (1):
  can: gs_usb: fix DMA memory leak on close

 drivers/net/can/usb/gs_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.30.2

[PATCH v4 1/1] can: gs_usb: fix DMA memory leak on close

[Rhett Aultman]

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:
928150fad41b ("can: esd_usb2: fix memory leak")

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.

Fixes: d08e973a77d1 ("can: gs_usb: Added support for the GS_USB CAN devices")
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;
 
 			usb_anchor_urb(urb, &parent->rx_submitted);
 
-- 
2.30.2