On Sat. 4 Jun. 2022 at 11:12, Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> wrote: > +CC: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > +CC: linux-usb@xxxxxxxxxxxxxxx > > Rhett Aultman <rhett.aultman@xxxxxxxxxxx> 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@xxxxxxxxxxx> > > 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@xxxxxxxxxxx> > > --- > > 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 > >
