* [PATCH 001/001] can: gs_usb: gs_usb_open/close( ) fix memory leak @ 2022-06-03 19:52 Rhett Aultman 2022-06-04 2:11 ` [PATCH] " Vincent Mailhol 2022-06-09 20:47 ` [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 0 siblings, 2 replies; 72+ messages in thread From: Rhett Aultman @ 2022-06-03 19:52 UTC (permalink / raw) To: wg, mkl, linux-can 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak 2022-06-03 19:52 [PATCH 001/001] can: gs_usb: gs_usb_open/close( ) fix memory leak Rhett Aultman @ 2022-06-04 2:11 ` Vincent Mailhol 2022-06-04 2:26 ` Vincent MAILHOL 2022-06-09 20:47 ` [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 1 sibling, 1 reply; 72+ messages in thread From: Vincent Mailhol @ 2022-06-04 2:11 UTC (permalink / raw) To: Rhett Aultman Cc: Vincent Mailhol, wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb +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 > ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak 2022-06-04 2:11 ` [PATCH] " Vincent Mailhol @ 2022-06-04 2:26 ` Vincent MAILHOL 2022-06-04 14:08 ` Rhett Aultman 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-04 2:26 UTC (permalink / raw) To: Rhett Aultman Cc: wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 > > ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak 2022-06-04 2:26 ` Vincent MAILHOL @ 2022-06-04 14:08 ` Rhett Aultman 2022-06-04 14:41 ` [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT Vincent Mailhol 2022-06-04 14:53 ` [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak Vincent MAILHOL 0 siblings, 2 replies; 72+ messages in thread From: Rhett Aultman @ 2022-06-04 14:08 UTC (permalink / raw) To: Vincent MAILHOL Cc: Rhett Aultman, wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-04 14:08 ` Rhett Aultman @ 2022-06-04 14:41 ` Vincent Mailhol 2022-06-04 16:40 ` Alan Stern 2022-06-05 2:15 ` [RFC PATCH v2] usb: " Vincent Mailhol 2022-06-04 14:53 ` [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak Vincent MAILHOL 1 sibling, 2 replies; 72+ messages in thread From: Vincent Mailhol @ 2022-06-04 14:41 UTC (permalink / raw) To: Rhett Aultman Cc: wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb, 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-04 14:41 ` [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT Vincent Mailhol @ 2022-06-04 16:40 ` Alan Stern 2022-06-05 2:04 ` Vincent MAILHOL 2022-06-05 6:00 ` Oliver Neukum 2022-06-05 2:15 ` [RFC PATCH v2] usb: " Vincent Mailhol 1 sibling, 2 replies; 72+ messages in thread From: Alan Stern @ 2022-06-04 16:40 UTC (permalink / raw) To: Vincent Mailhol Cc: Rhett Aultman, wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-04 16:40 ` Alan Stern @ 2022-06-05 2:04 ` Vincent MAILHOL 2022-06-05 6:00 ` Oliver Neukum 1 sibling, 0 replies; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-05 2:04 UTC (permalink / raw) To: Alan Stern Cc: Rhett Aultman, wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-04 16:40 ` Alan Stern 2022-06-05 2:04 ` Vincent MAILHOL @ 2022-06-05 6:00 ` Oliver Neukum 2022-06-05 13:45 ` Vincent MAILHOL 1 sibling, 1 reply; 72+ messages in thread From: Oliver Neukum @ 2022-06-05 6:00 UTC (permalink / raw) To: Alan Stern, Vincent Mailhol Cc: Rhett Aultman, wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-05 6:00 ` Oliver Neukum @ 2022-06-05 13:45 ` Vincent MAILHOL 2022-06-07 9:49 ` Oliver Neukum 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-05 13:45 UTC (permalink / raw) To: Oliver Neukum Cc: Alan Stern, Rhett Aultman, Wolfgang Grandegger, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-05 13:45 ` Vincent MAILHOL @ 2022-06-07 9:49 ` Oliver Neukum 2022-06-07 10:18 ` Vincent MAILHOL 0 siblings, 1 reply; 72+ messages in thread From: Oliver Neukum @ 2022-06-07 9:49 UTC (permalink / raw) To: Vincent MAILHOL, Oliver Neukum Cc: Alan Stern, Rhett Aultman, Wolfgang Grandegger, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-07 9:49 ` Oliver Neukum @ 2022-06-07 10:18 ` Vincent MAILHOL 2022-06-07 11:46 ` Oliver Neukum 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-07 10:18 UTC (permalink / raw) To: Oliver Neukum Cc: Alan Stern, Rhett Aultman, Wolfgang Grandegger, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-07 10:18 ` Vincent MAILHOL @ 2022-06-07 11:46 ` Oliver Neukum 2022-06-07 12:12 ` Vincent MAILHOL 0 siblings, 1 reply; 72+ messages in thread From: Oliver Neukum @ 2022-06-07 11:46 UTC (permalink / raw) To: Vincent MAILHOL, Oliver Neukum Cc: Alan Stern, Rhett Aultman, Wolfgang Grandegger, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT 2022-06-07 11:46 ` Oliver Neukum @ 2022-06-07 12:12 ` Vincent MAILHOL 0 siblings, 0 replies; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-07 12:12 UTC (permalink / raw) To: Oliver Neukum Cc: Alan Stern, Rhett Aultman, Wolfgang Grandegger, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [RFC PATCH v2] usb: urb: add new transfer flag URB_FREE_COHERENT 2022-06-04 14:41 ` [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT Vincent Mailhol 2022-06-04 16:40 ` Alan Stern @ 2022-06-05 2:15 ` Vincent Mailhol 1 sibling, 0 replies; 72+ messages in thread From: Vincent Mailhol @ 2022-06-05 2:15 UTC (permalink / raw) To: Rhett Aultman Cc: Alan Stern, wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb, 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak 2022-06-04 14:08 ` Rhett Aultman 2022-06-04 14:41 ` [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT Vincent Mailhol @ 2022-06-04 14:53 ` Vincent MAILHOL 1 sibling, 0 replies; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-04 14:53 UTC (permalink / raw) To: Rhett Aultman Cc: wg, Marc Kleine-Budde, Greg Kroah-Hartman, linux-can, linux-usb 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix 2022-06-03 19:52 [PATCH 001/001] can: gs_usb: gs_usb_open/close( ) fix memory leak Rhett Aultman 2022-06-04 2:11 ` [PATCH] " Vincent Mailhol @ 2022-06-09 20:47 ` Rhett Aultman 2022-06-09 20:47 ` [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman ` (3 more replies) 1 sibling, 4 replies; 72+ messages in thread From: Rhett Aultman @ 2022-06-09 20:47 UTC (permalink / raw) To: linux-usb, linux-can Cc: Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-09 20:47 ` [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman @ 2022-06-09 20:47 ` Rhett Aultman 2022-06-10 0:18 ` Vincent MAILHOL 2022-06-09 20:47 ` [PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT Rhett Aultman ` (2 subsequent siblings) 3 siblings, 1 reply; 72+ messages in thread From: Rhett Aultman @ 2022-06-09 20:47 UTC (permalink / raw) To: linux-usb, linux-can Cc: Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, Rhett Aultman, 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. 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-09 20:47 ` [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman @ 2022-06-10 0:18 ` Vincent MAILHOL 2022-06-10 10:46 ` Marc Kleine-Budde 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-10 0:18 UTC (permalink / raw) To: Rhett Aultman Cc: linux-usb, linux-can, Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-10 0:18 ` Vincent MAILHOL @ 2022-06-10 10:46 ` Marc Kleine-Budde 0 siblings, 0 replies; 72+ messages in thread From: Marc Kleine-Budde @ 2022-06-10 10:46 UTC (permalink / raw) To: Vincent MAILHOL Cc: Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Alan Stern, Greg Kroah-Hartman [-- 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 --] ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT 2022-06-09 20:47 ` [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2022-06-09 20:47 ` [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman @ 2022-06-09 20:47 ` Rhett Aultman 2022-06-09 23:18 ` Vincent Mailhol 2022-06-09 20:47 ` [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close Rhett Aultman 2022-06-10 21:33 ` [PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 3 siblings, 1 reply; 72+ messages in thread From: Rhett Aultman @ 2022-06-09 20:47 UTC (permalink / raw) To: linux-usb, linux-can Cc: Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT 2022-06-09 20:47 ` [PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT Rhett Aultman @ 2022-06-09 23:18 ` Vincent Mailhol 0 siblings, 0 replies; 72+ messages in thread From: Vincent Mailhol @ 2022-06-09 23:18 UTC (permalink / raw) To: Rhett Aultman Cc: linux-usb, linux-can, Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close 2022-06-09 20:47 ` [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2022-06-09 20:47 ` [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman 2022-06-09 20:47 ` [PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT Rhett Aultman @ 2022-06-09 20:47 ` Rhett Aultman 2022-06-10 0:05 ` Vincent Mailhol 2022-06-10 21:33 ` [PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 3 siblings, 1 reply; 72+ messages in thread From: Rhett Aultman @ 2022-06-09 20:47 UTC (permalink / raw) To: linux-usb, linux-can Cc: Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close 2022-06-09 20:47 ` [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close Rhett Aultman @ 2022-06-10 0:05 ` Vincent Mailhol 2022-06-10 1:28 ` Vincent Mailhol 0 siblings, 1 reply; 72+ messages in thread From: Vincent Mailhol @ 2022-06-10 0:05 UTC (permalink / raw) To: Rhett Aultman Cc: linux-usb, linux-can, Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close 2022-06-10 0:05 ` Vincent Mailhol @ 2022-06-10 1:28 ` Vincent Mailhol 0 siblings, 0 replies; 72+ messages in thread From: Vincent Mailhol @ 2022-06-10 1:28 UTC (permalink / raw) To: Rhett Aultman Cc: linux-usb, linux-can, Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix 2022-06-09 20:47 ` [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman ` (2 preceding siblings ...) 2022-06-09 20:47 ` [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close Rhett Aultman @ 2022-06-10 21:33 ` Rhett Aultman 2022-06-10 21:33 ` [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman ` (2 more replies) 3 siblings, 3 replies; 72+ messages in thread From: Rhett Aultman @ 2022-06-10 21:33 UTC (permalink / raw) To: linux-usb, linux-can Cc: --cc=Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-10 21:33 ` [PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman @ 2022-06-10 21:33 ` Rhett Aultman 2022-06-11 15:31 ` Marc Kleine-Budde 2022-06-23 17:30 ` Hongren Zenithal Zheng 2022-06-10 21:33 ` [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close Rhett Aultman 2022-06-14 15:26 ` [PATCH v4 0/1] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2 siblings, 2 replies; 72+ messages in thread From: Rhett Aultman @ 2022-06-10 21:33 UTC (permalink / raw) To: linux-usb, linux-can Cc: --cc=Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-10 21:33 ` [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman @ 2022-06-11 15:31 ` Marc Kleine-Budde 2022-06-11 16:06 ` Vincent MAILHOL 2022-06-23 17:30 ` Hongren Zenithal Zheng 1 sibling, 1 reply; 72+ messages in thread From: Marc Kleine-Budde @ 2022-06-11 15:31 UTC (permalink / raw) To: Rhett Aultman Cc: linux-usb, linux-can, Oliver Neukum, Alan Stern, Greg Kroah-Hartman, Vincent Mailhol [-- 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 --] ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-11 15:31 ` Marc Kleine-Budde @ 2022-06-11 16:06 ` Vincent MAILHOL 2022-06-21 13:51 ` Greg Kroah-Hartman 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-11 16:06 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Alan Stern, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-11 16:06 ` Vincent MAILHOL @ 2022-06-21 13:51 ` Greg Kroah-Hartman 2022-06-21 14:59 ` Vincent MAILHOL 0 siblings, 1 reply; 72+ messages in thread From: Greg Kroah-Hartman @ 2022-06-21 13:51 UTC (permalink / raw) To: Vincent MAILHOL Cc: Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Alan Stern 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 13:51 ` Greg Kroah-Hartman @ 2022-06-21 14:59 ` Vincent MAILHOL 2022-06-21 15:03 ` Greg Kroah-Hartman 2022-06-21 15:11 ` Alan Stern 0 siblings, 2 replies; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-21 14:59 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Alan Stern 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 14:59 ` Vincent MAILHOL @ 2022-06-21 15:03 ` Greg Kroah-Hartman 2022-06-21 15:54 ` Vincent MAILHOL 2022-06-21 15:11 ` Alan Stern 1 sibling, 1 reply; 72+ messages in thread From: Greg Kroah-Hartman @ 2022-06-21 15:03 UTC (permalink / raw) To: Vincent MAILHOL Cc: Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Alan Stern 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 15:03 ` Greg Kroah-Hartman @ 2022-06-21 15:54 ` Vincent MAILHOL 0 siblings, 0 replies; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-21 15:54 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Alan Stern 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. ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 14:59 ` Vincent MAILHOL 2022-06-21 15:03 ` Greg Kroah-Hartman @ 2022-06-21 15:11 ` Alan Stern 2022-06-21 15:55 ` Vincent MAILHOL 1 sibling, 1 reply; 72+ messages in thread From: Alan Stern @ 2022-06-21 15:11 UTC (permalink / raw) To: Vincent MAILHOL Cc: Greg Kroah-Hartman, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 15:11 ` Alan Stern @ 2022-06-21 15:55 ` Vincent MAILHOL 2022-06-21 16:14 ` Alan Stern 2022-06-22 9:22 ` David Laight 0 siblings, 2 replies; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-21 15:55 UTC (permalink / raw) To: Alan Stern Cc: Greg Kroah-Hartman, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 15:55 ` Vincent MAILHOL @ 2022-06-21 16:14 ` Alan Stern 2022-06-21 16:40 ` Vincent MAILHOL 2022-06-22 9:22 ` David Laight 1 sibling, 1 reply; 72+ messages in thread From: Alan Stern @ 2022-06-21 16:14 UTC (permalink / raw) To: Vincent MAILHOL Cc: Greg Kroah-Hartman, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 16:14 ` Alan Stern @ 2022-06-21 16:40 ` Vincent MAILHOL 2022-06-21 17:00 ` Greg Kroah-Hartman 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-21 16:40 UTC (permalink / raw) To: Alan Stern Cc: Greg Kroah-Hartman, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 16:40 ` Vincent MAILHOL @ 2022-06-21 17:00 ` Greg Kroah-Hartman 2022-06-21 17:14 ` Vincent MAILHOL 0 siblings, 1 reply; 72+ messages in thread From: Greg Kroah-Hartman @ 2022-06-21 17:00 UTC (permalink / raw) To: Vincent MAILHOL Cc: Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 17:00 ` Greg Kroah-Hartman @ 2022-06-21 17:14 ` Vincent MAILHOL 2022-06-21 17:46 ` Greg Kroah-Hartman 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-21 17:14 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 17:14 ` Vincent MAILHOL @ 2022-06-21 17:46 ` Greg Kroah-Hartman 0 siblings, 0 replies; 72+ messages in thread From: Greg Kroah-Hartman @ 2022-06-21 17:46 UTC (permalink / raw) To: Vincent MAILHOL Cc: Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* RE: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-21 15:55 ` Vincent MAILHOL 2022-06-21 16:14 ` Alan Stern @ 2022-06-22 9:22 ` David Laight 2022-06-22 9:41 ` Greg Kroah-Hartman 1 sibling, 1 reply; 72+ messages in thread From: David Laight @ 2022-06-22 9:22 UTC (permalink / raw) To: 'Vincent MAILHOL', Alan Stern Cc: Greg Kroah-Hartman, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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) ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-22 9:22 ` David Laight @ 2022-06-22 9:41 ` Greg Kroah-Hartman 2022-06-22 10:03 ` David Laight 2022-06-22 10:34 ` Vincent MAILHOL 0 siblings, 2 replies; 72+ messages in thread From: Greg Kroah-Hartman @ 2022-06-22 9:41 UTC (permalink / raw) To: David Laight Cc: 'Vincent MAILHOL', Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* RE: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-22 9:41 ` Greg Kroah-Hartman @ 2022-06-22 10:03 ` David Laight 2022-06-22 11:11 ` Oliver Neukum 2022-06-22 10:34 ` Vincent MAILHOL 1 sibling, 1 reply; 72+ messages in thread From: David Laight @ 2022-06-22 10:03 UTC (permalink / raw) To: 'Greg Kroah-Hartman' Cc: 'Vincent MAILHOL', Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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) ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-22 10:03 ` David Laight @ 2022-06-22 11:11 ` Oliver Neukum 0 siblings, 0 replies; 72+ messages in thread From: Oliver Neukum @ 2022-06-22 11:11 UTC (permalink / raw) To: David Laight, 'Greg Kroah-Hartman' Cc: 'Vincent MAILHOL', Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-22 9:41 ` Greg Kroah-Hartman 2022-06-22 10:03 ` David Laight @ 2022-06-22 10:34 ` Vincent MAILHOL 2022-06-22 12:23 ` Greg Kroah-Hartman 1 sibling, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-22 10:34 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: David Laight, Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-22 10:34 ` Vincent MAILHOL @ 2022-06-22 12:23 ` Greg Kroah-Hartman 2022-06-22 15:59 ` Vincent MAILHOL 0 siblings, 1 reply; 72+ messages in thread From: Greg Kroah-Hartman @ 2022-06-22 12:23 UTC (permalink / raw) To: Vincent MAILHOL Cc: David Laight, Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-22 12:23 ` Greg Kroah-Hartman @ 2022-06-22 15:59 ` Vincent MAILHOL 2022-06-22 18:11 ` Rhett Aultman 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-22 15:59 UTC (permalink / raw) To: Greg Kroah-Hartman Cc: David Laight, Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-22 15:59 ` Vincent MAILHOL @ 2022-06-22 18:11 ` Rhett Aultman 2022-06-26 8:21 ` Vincent MAILHOL 0 siblings, 1 reply; 72+ messages in thread From: Rhett Aultman @ 2022-06-22 18:11 UTC (permalink / raw) To: Vincent MAILHOL Cc: Greg Kroah-Hartman, David Laight, Alan Stern, Marc Kleine-Budde, Rhett Aultman, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-22 18:11 ` Rhett Aultman @ 2022-06-26 8:21 ` Vincent MAILHOL 2022-06-27 19:24 ` Rhett Aultman 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-26 8:21 UTC (permalink / raw) To: Rhett Aultman Cc: Greg Kroah-Hartman, David Laight, Alan Stern, Marc Kleine-Budde, linux-usb, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-26 8:21 ` Vincent MAILHOL @ 2022-06-27 19:24 ` Rhett Aultman 2022-06-28 1:09 ` Vincent MAILHOL 0 siblings, 1 reply; 72+ messages in thread From: Rhett Aultman @ 2022-06-27 19:24 UTC (permalink / raw) To: Vincent MAILHOL Cc: Rhett Aultman, Greg Kroah-Hartman, David Laight, Alan Stern, Marc Kleine-Budde, linux-can, Oliver Neukum [-- 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-27 19:24 ` Rhett Aultman @ 2022-06-28 1:09 ` Vincent MAILHOL 2022-07-04 13:02 ` Marc Kleine-Budde 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-28 1:09 UTC (permalink / raw) To: Rhett Aultman Cc: Greg Kroah-Hartman, David Laight, Alan Stern, Marc Kleine-Budde, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-28 1:09 ` Vincent MAILHOL @ 2022-07-04 13:02 ` Marc Kleine-Budde 2022-07-04 15:35 ` Rhett Aultman 0 siblings, 1 reply; 72+ messages in thread From: Marc Kleine-Budde @ 2022-07-04 13:02 UTC (permalink / raw) To: Vincent MAILHOL Cc: Rhett Aultman, Greg Kroah-Hartman, David Laight, Alan Stern, linux-can, Oliver Neukum [-- 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 --] ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-07-04 13:02 ` Marc Kleine-Budde @ 2022-07-04 15:35 ` Rhett Aultman 2022-07-05 7:50 ` Marc Kleine-Budde 0 siblings, 1 reply; 72+ messages in thread From: Rhett Aultman @ 2022-07-04 15:35 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Vincent MAILHOL, Rhett Aultman, Greg Kroah-Hartman, David Laight, Alan Stern, linux-can, Oliver Neukum 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-07-04 15:35 ` Rhett Aultman @ 2022-07-05 7:50 ` Marc Kleine-Budde 0 siblings, 0 replies; 72+ messages in thread From: Marc Kleine-Budde @ 2022-07-05 7:50 UTC (permalink / raw) To: Rhett Aultman Cc: Vincent MAILHOL, Greg Kroah-Hartman, David Laight, Alan Stern, linux-can, Oliver Neukum [-- 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 --] ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-10 21:33 ` [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman 2022-06-11 15:31 ` Marc Kleine-Budde @ 2022-06-23 17:30 ` Hongren Zenithal Zheng 2022-06-23 17:45 ` Shuah Khan 1 sibling, 1 reply; 72+ messages in thread From: Hongren Zenithal Zheng @ 2022-06-23 17:30 UTC (permalink / raw) To: Rhett Aultman Cc: linux-usb, linux-can, Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-23 17:30 ` Hongren Zenithal Zheng @ 2022-06-23 17:45 ` Shuah Khan 2022-06-24 14:43 ` Alan Stern 0 siblings, 1 reply; 72+ messages in thread From: Shuah Khan @ 2022-06-23 17:45 UTC (permalink / raw) To: Hongren Zenithal Zheng, Rhett Aultman Cc: linux-usb, linux-can, Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-23 17:45 ` Shuah Khan @ 2022-06-24 14:43 ` Alan Stern 2022-06-24 16:01 ` Hongren Zenithal Zheng 2022-06-24 16:31 ` Shuah Khan 0 siblings, 2 replies; 72+ messages in thread From: Alan Stern @ 2022-06-24 14:43 UTC (permalink / raw) To: Shuah Khan Cc: Hongren Zenithal Zheng, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-24 14:43 ` Alan Stern @ 2022-06-24 16:01 ` Hongren Zenithal Zheng 2022-06-24 16:31 ` Shuah Khan 1 sibling, 0 replies; 72+ messages in thread From: Hongren Zenithal Zheng @ 2022-06-24 16:01 UTC (permalink / raw) To: Alan Stern Cc: Shuah Khan, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-24 14:43 ` Alan Stern 2022-06-24 16:01 ` Hongren Zenithal Zheng @ 2022-06-24 16:31 ` Shuah Khan 2022-06-24 18:07 ` Alan Stern 1 sibling, 1 reply; 72+ messages in thread From: Shuah Khan @ 2022-06-24 16:31 UTC (permalink / raw) To: Alan Stern Cc: Hongren Zenithal Zheng, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-24 16:31 ` Shuah Khan @ 2022-06-24 18:07 ` Alan Stern 2022-06-27 22:54 ` Shuah Khan 0 siblings, 1 reply; 72+ messages in thread From: Alan Stern @ 2022-06-24 18:07 UTC (permalink / raw) To: Shuah Khan Cc: Hongren Zenithal Zheng, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-24 18:07 ` Alan Stern @ 2022-06-27 22:54 ` Shuah Khan 2022-06-28 1:35 ` Alan Stern 0 siblings, 1 reply; 72+ messages in thread From: Shuah Khan @ 2022-06-27 22:54 UTC (permalink / raw) To: Alan Stern, Hongren Zenithal Zheng Cc: Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-27 22:54 ` Shuah Khan @ 2022-06-28 1:35 ` Alan Stern 2022-07-01 2:10 ` Shuah Khan 0 siblings, 1 reply; 72+ messages in thread From: Alan Stern @ 2022-06-28 1:35 UTC (permalink / raw) To: Shuah Khan Cc: Hongren Zenithal Zheng, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-06-28 1:35 ` Alan Stern @ 2022-07-01 2:10 ` Shuah Khan 2022-08-01 17:42 ` Shuah Khan 0 siblings, 1 reply; 72+ messages in thread From: Shuah Khan @ 2022-07-01 2:10 UTC (permalink / raw) To: Alan Stern Cc: Hongren Zenithal Zheng, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-07-01 2:10 ` Shuah Khan @ 2022-08-01 17:42 ` Shuah Khan 2022-08-01 18:28 ` Vincent MAILHOL 0 siblings, 1 reply; 72+ messages in thread From: Shuah Khan @ 2022-08-01 17:42 UTC (permalink / raw) To: Alan Stern, Hongren Zenithal Zheng Cc: Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol, Shuah Khan, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-08-01 17:42 ` Shuah Khan @ 2022-08-01 18:28 ` Vincent MAILHOL 2022-08-03 23:44 ` Shuah Khan 0 siblings, 1 reply; 72+ messages in thread From: Vincent MAILHOL @ 2022-08-01 18:28 UTC (permalink / raw) To: Shuah Khan Cc: Alan Stern, Hongren Zenithal Zheng, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Shuah Khan 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT 2022-08-01 18:28 ` Vincent MAILHOL @ 2022-08-03 23:44 ` Shuah Khan 0 siblings, 0 replies; 72+ messages in thread From: Shuah Khan @ 2022-08-03 23:44 UTC (permalink / raw) To: Vincent MAILHOL Cc: Alan Stern, Hongren Zenithal Zheng, Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Marc Kleine-Budde, Greg Kroah-Hartman, Shuah Khan, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close 2022-06-10 21:33 ` [PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2022-06-10 21:33 ` [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman @ 2022-06-10 21:33 ` Rhett Aultman 2022-06-11 15:35 ` Marc Kleine-Budde 2022-06-12 21:28 ` David Laight 2022-06-14 15:26 ` [PATCH v4 0/1] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2 siblings, 2 replies; 72+ messages in thread From: Rhett Aultman @ 2022-06-10 21:33 UTC (permalink / raw) To: linux-usb, linux-can Cc: --cc=Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, Rhett Aultman, Vincent Mailhol 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
* Re: [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close 2022-06-10 21:33 ` [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close Rhett Aultman @ 2022-06-11 15:35 ` Marc Kleine-Budde 2022-06-11 16:03 ` Vincent MAILHOL 2022-06-12 21:28 ` David Laight 1 sibling, 1 reply; 72+ messages in thread From: Marc Kleine-Budde @ 2022-06-11 15:35 UTC (permalink / raw) To: Rhett Aultman Cc: linux-usb, linux-can, Oliver Neukum, Alan Stern, Greg Kroah-Hartman, Vincent Mailhol [-- 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 --] ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close 2022-06-11 15:35 ` Marc Kleine-Budde @ 2022-06-11 16:03 ` Vincent MAILHOL 0 siblings, 0 replies; 72+ messages in thread From: Vincent MAILHOL @ 2022-06-11 16:03 UTC (permalink / raw) To: Marc Kleine-Budde Cc: Rhett Aultman, linux-usb, linux-can, Oliver Neukum, Alan Stern, Greg Kroah-Hartman 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 | ^ permalink raw reply [flat|nested] 72+ messages in thread
* RE: [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close 2022-06-10 21:33 ` [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close Rhett Aultman 2022-06-11 15:35 ` Marc Kleine-Budde @ 2022-06-12 21:28 ` David Laight 2022-06-12 21:33 ` Marc Kleine-Budde 1 sibling, 1 reply; 72+ messages in thread From: David Laight @ 2022-06-12 21:28 UTC (permalink / raw) To: 'Rhett Aultman', linux-usb, linux-can Cc: --cc=Oliver Neukum, Alan Stern, Marc Kleine-Budde, Greg Kroah-Hartman, Vincent Mailhol 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) ^ permalink raw reply [flat|nested] 72+ messages in thread
* Re: [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close 2022-06-12 21:28 ` David Laight @ 2022-06-12 21:33 ` Marc Kleine-Budde 0 siblings, 0 replies; 72+ messages in thread From: Marc Kleine-Budde @ 2022-06-12 21:33 UTC (permalink / raw) To: David Laight Cc: 'Rhett Aultman', linux-usb, linux-can, --cc=Oliver Neukum, Alan Stern, Greg Kroah-Hartman, Vincent Mailhol [-- 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 --] ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v4 0/1] URB_FREE_COHERENT gs_usb memory leak fix 2022-06-10 21:33 ` [PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2022-06-10 21:33 ` [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman 2022-06-10 21:33 ` [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close Rhett Aultman @ 2022-06-14 15:26 ` Rhett Aultman 2022-06-14 15:26 ` [PATCH v4 1/1] can: gs_usb: fix DMA memory leak on close Rhett Aultman 2 siblings, 1 reply; 72+ messages in thread From: Rhett Aultman @ 2022-06-14 15:26 UTC (permalink / raw) To: linux-can; +Cc: --cc=Marc Kleine-Budde, Vincent MAILHOL, 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 ^ permalink raw reply [flat|nested] 72+ messages in thread
* [PATCH v4 1/1] can: gs_usb: fix DMA memory leak on close 2022-06-14 15:26 ` [PATCH v4 0/1] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman @ 2022-06-14 15:26 ` Rhett Aultman 0 siblings, 0 replies; 72+ messages in thread From: Rhett Aultman @ 2022-06-14 15:26 UTC (permalink / raw) To: linux-can; +Cc: --cc=Marc Kleine-Budde, Vincent MAILHOL, 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 ^ permalink raw reply related [flat|nested] 72+ messages in thread
end of thread, other threads:[~2022-08-03 23:44 UTC | newest] Thread overview: 72+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-03 19:52 [PATCH 001/001] can: gs_usb: gs_usb_open/close( ) fix memory leak Rhett Aultman 2022-06-04 2:11 ` [PATCH] " Vincent Mailhol 2022-06-04 2:26 ` Vincent MAILHOL 2022-06-04 14:08 ` Rhett Aultman 2022-06-04 14:41 ` [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT Vincent Mailhol 2022-06-04 16:40 ` Alan Stern 2022-06-05 2:04 ` Vincent MAILHOL 2022-06-05 6:00 ` Oliver Neukum 2022-06-05 13:45 ` Vincent MAILHOL 2022-06-07 9:49 ` Oliver Neukum 2022-06-07 10:18 ` Vincent MAILHOL 2022-06-07 11:46 ` Oliver Neukum 2022-06-07 12:12 ` Vincent MAILHOL 2022-06-05 2:15 ` [RFC PATCH v2] usb: " Vincent Mailhol 2022-06-04 14:53 ` [PATCH] can: gs_usb: gs_usb_open/close( ) fix memory leak Vincent MAILHOL 2022-06-09 20:47 ` [PATCH v2 0/3] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2022-06-09 20:47 ` [PATCH v2 1/3] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman 2022-06-10 0:18 ` Vincent MAILHOL 2022-06-10 10:46 ` Marc Kleine-Budde 2022-06-09 20:47 ` [PATCH v2 2/3] drivers: usb/core/urb: allow URB_FREE_COHERENT Rhett Aultman 2022-06-09 23:18 ` Vincent Mailhol 2022-06-09 20:47 ` [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close Rhett Aultman 2022-06-10 0:05 ` Vincent Mailhol 2022-06-10 1:28 ` Vincent Mailhol 2022-06-10 21:33 ` [PATCH v3 0/2] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2022-06-10 21:33 ` [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT Rhett Aultman 2022-06-11 15:31 ` Marc Kleine-Budde 2022-06-11 16:06 ` Vincent MAILHOL 2022-06-21 13:51 ` Greg Kroah-Hartman 2022-06-21 14:59 ` Vincent MAILHOL 2022-06-21 15:03 ` Greg Kroah-Hartman 2022-06-21 15:54 ` Vincent MAILHOL 2022-06-21 15:11 ` Alan Stern 2022-06-21 15:55 ` Vincent MAILHOL 2022-06-21 16:14 ` Alan Stern 2022-06-21 16:40 ` Vincent MAILHOL 2022-06-21 17:00 ` Greg Kroah-Hartman 2022-06-21 17:14 ` Vincent MAILHOL 2022-06-21 17:46 ` Greg Kroah-Hartman 2022-06-22 9:22 ` David Laight 2022-06-22 9:41 ` Greg Kroah-Hartman 2022-06-22 10:03 ` David Laight 2022-06-22 11:11 ` Oliver Neukum 2022-06-22 10:34 ` Vincent MAILHOL 2022-06-22 12:23 ` Greg Kroah-Hartman 2022-06-22 15:59 ` Vincent MAILHOL 2022-06-22 18:11 ` Rhett Aultman 2022-06-26 8:21 ` Vincent MAILHOL 2022-06-27 19:24 ` Rhett Aultman 2022-06-28 1:09 ` Vincent MAILHOL 2022-07-04 13:02 ` Marc Kleine-Budde 2022-07-04 15:35 ` Rhett Aultman 2022-07-05 7:50 ` Marc Kleine-Budde 2022-06-23 17:30 ` Hongren Zenithal Zheng 2022-06-23 17:45 ` Shuah Khan 2022-06-24 14:43 ` Alan Stern 2022-06-24 16:01 ` Hongren Zenithal Zheng 2022-06-24 16:31 ` Shuah Khan 2022-06-24 18:07 ` Alan Stern 2022-06-27 22:54 ` Shuah Khan 2022-06-28 1:35 ` Alan Stern 2022-07-01 2:10 ` Shuah Khan 2022-08-01 17:42 ` Shuah Khan 2022-08-01 18:28 ` Vincent MAILHOL 2022-08-03 23:44 ` Shuah Khan 2022-06-10 21:33 ` [PATCH v3 2/2] can: gs_usb: fix DMA memory leak on close Rhett Aultman 2022-06-11 15:35 ` Marc Kleine-Budde 2022-06-11 16:03 ` Vincent MAILHOL 2022-06-12 21:28 ` David Laight 2022-06-12 21:33 ` Marc Kleine-Budde 2022-06-14 15:26 ` [PATCH v4 0/1] URB_FREE_COHERENT gs_usb memory leak fix Rhett Aultman 2022-06-14 15:26 ` [PATCH v4 1/1] can: gs_usb: fix DMA memory leak on close Rhett Aultman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.