From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id EE302C433EF for ; Fri, 10 Jun 2022 00:05:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235021AbiFJAFY (ORCPT ); Thu, 9 Jun 2022 20:05:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:47768 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231197AbiFJAFX (ORCPT ); Thu, 9 Jun 2022 20:05:23 -0400 Received: from mail-yw1-x112b.google.com (mail-yw1-x112b.google.com [IPv6:2607:f8b0:4864:20::112b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B7D4C2EA404; Thu, 9 Jun 2022 17:05:21 -0700 (PDT) Received: by mail-yw1-x112b.google.com with SMTP id 00721157ae682-30ce6492a60so257368847b3.8; Thu, 09 Jun 2022 17:05:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=Rw54DKLgsReePuyIeN6kM1mexgWFFu3bCYqVwRXSeus=; b=gHKqH6AnuMCobVdh8glhec8XDoxyJQHav52Xw9NVESk31p3JwcAM/Euk/DS0yauVyj 62l6Byqf0hCILoqf0pNbrzVPb/BJuDOeklaxqLPU5ywycqJARAce28uzIsp9yvxpJPUz seXRVd6AuAfLRym1rqpb63uL1MN6cFKOLajYERU4E2PvxAF2puUT1F30fNEVLVdCVnCB 8Hk6CT6zFBHEt3LwmpcccNLPKeh8GvQz6qU1QY3vBKIGFi8/bI4zq1hvJLC8KgwCO3zl HznHxSTnr5G2yCK1a5SJxBAeiPUK+RIGMbEloDCKBO5xf288FxhXR+UEhW/2+E0GLPL5 tFfg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=Rw54DKLgsReePuyIeN6kM1mexgWFFu3bCYqVwRXSeus=; b=3TE6iPDBvSypw/vFEn0U8iTCCw7nGmxuVXXYOjOARHwK1ayZL3DtNf3iA/W26BDZhb 9HAQ07OldAfFh15kCAnCj1PtugbL1lJ/KuqmMT3UMcSscpf8lBk+uy5vijn97ZWeAO/N m3KrVSH3fUefA075Cfe+QCLT433DuRu1F9Nz3vjFz47fCjX9bGSnoXYqzS1CNBaULK+i BfugGFqPMfBroBo9Ug4vWOoDvVwFAJEJL/btUKe4sciFSfEjhBvrYqJcpAu3QnSRGl3X 0tmycgUPAp7toyHKMXIBWHWa7+5h+LmdFxEDSrvgeILbyoNcUQZiyq6n/99Gc/oIPL2P lhxQ== X-Gm-Message-State: AOAM531n89w3JQR6x6zU/f73q5W/sofxPndnwoKFhFG+QNCSF107pPqr wQaNnYjtkCm+afkgsUlNFRdh7Ghv8snYVg83TzA= X-Google-Smtp-Source: ABdhPJzsxKHS6848+DMMPwQeWowkHq7828o5WykA6d6FeE7wp1UeLOWMVtRdzTnGvszTWPjk0zZhPmG8OnRR+tnlE+8= X-Received: by 2002:a81:60c6:0:b0:30f:ea59:10c7 with SMTP id u189-20020a8160c6000000b0030fea5910c7mr44589707ywb.78.1654819520875; Thu, 09 Jun 2022 17:05:20 -0700 (PDT) MIME-Version: 1.0 References: <20220609204714.2715188-1-rhett.aultman@samsara.com> <20220609204714.2715188-4-rhett.aultman@samsara.com> In-Reply-To: <20220609204714.2715188-4-rhett.aultman@samsara.com> From: Vincent Mailhol Date: Fri, 10 Jun 2022 09:05:09 +0900 Message-ID: Subject: Re: [PATCH v2 3/3] can: gs_usb: fix DMA memory leak on close To: Rhett Aultman Cc: linux-usb@vger.kernel.org, linux-can , Oliver Neukum , Alan Stern , Marc Kleine-Budde , Greg Kroah-Hartman Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-can@vger.kernel.org On Fri. 10 Jun 2022 at 05:53, Rhett Aultman wro= te: > 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 > --- > 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, p= arent); > > - urb->transfer_flags |=3D URB_NO_TRANSFER_DMA_MAP; > + urb->transfer_flags |=3D (URB_NO_TRANSFER_DMA_MAP= | URB_FREE_COHERENT); Nitpick but parenthesis are not needed here. Also, there are no reasons to do a |=3D. I would prefer to see this: urb->transfer_flags =3D 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=E2=80=A6 Aside from the nitpicks, the patch looks good to me. You can add this to your v3: Reviewed-by: Vincent Mailhol Yours sincerely, Vincent Mailhol