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

[Vincent Mailhol <mailhol.vincent@wanadoo.fr>]

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