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 X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_PASS,URIBL_BLACK autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0A624C43219 for ; Mon, 29 Apr 2019 11:16:16 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 840862087B for ; Mon, 29 Apr 2019 11:16:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 840862087B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kaod.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 44t2Cd4d0RzDqL0 for ; Mon, 29 Apr 2019 21:16:13 +1000 (AEST) Authentication-Results: lists.ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=kaod.org (client-ip=178.33.111.220; helo=1.mo179.mail-out.ovh.net; envelope-from=groug@kaod.org; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=kaod.org X-Greylist: delayed 1200 seconds by postgrey-1.36 at bilbo; Mon, 29 Apr 2019 21:12:36 AEST Received: from 1.mo179.mail-out.ovh.net (1.mo179.mail-out.ovh.net [178.33.111.220]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 44t27S5dYdzDqJP for ; Mon, 29 Apr 2019 21:12:34 +1000 (AEST) Received: from player158.ha.ovh.net (unknown [10.108.57.50]) by mo179.mail-out.ovh.net (Postfix) with ESMTP id 25AC0127301 for ; Mon, 29 Apr 2019 12:37:09 +0200 (CEST) Received: from kaod.org (lns-bzn-46-82-253-208-248.adsl.proxad.net [82.253.208.248]) (Authenticated sender: groug@kaod.org) by player158.ha.ovh.net (Postfix) with ESMTPSA id C9E2051F45C6; Mon, 29 Apr 2019 10:37:01 +0000 (UTC) Date: Mon, 29 Apr 2019 12:36:59 +0200 From: Greg Kurz To: Alexey Kardashevskiy Subject: Re: [PATCH] powerpc/powernv/npu: Fix reference leak Message-ID: <20190429123659.00c0622b@bahia.lan> In-Reply-To: <962c1d9e-719c-cb82-cabc-1cf619e1510b@ozlabs.ru> References: <155568805354.600470.13376593185688810607.stgit@bahia.lan> <962c1d9e-719c-cb82-cabc-1cf619e1510b@ozlabs.ru> X-Mailer: Claws Mail 3.16.0 (GTK+ 2.24.32; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Ovh-Tracer-Id: 15264106512109181429 X-VR-SPAMSTATE: OK X-VR-SPAMSCORE: -100 X-VR-SPAMCAUSE: gggruggvucftvghtrhhoucdtuddrgeduuddriedvgdefudcutefuodetggdotefrodftvfcurfhrohhfihhlvgemucfqggfjpdevjffgvefmvefgnecuuegrihhlohhuthemucehtddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Alistair Popple Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Mon, 29 Apr 2019 16:01:29 +1000 Alexey Kardashevskiy wrote: > On 20/04/2019 01:34, Greg Kurz wrote: > > Since 902bdc57451c, get_pci_dev() calls pci_get_domain_bus_and_slot(). This > > has the effect of incrementing the reference count of the PCI device, as > > explained in drivers/pci/search.c: > > > > * Given a PCI domain, bus, and slot/function number, the desired PCI > > * device is located in the list of PCI devices. If the device is > > * found, its reference count is increased and this function returns a > > * pointer to its data structure. The caller must decrement the > > * reference count by calling pci_dev_put(). If no device is found, > > * %NULL is returned. > > > > Nothing was done to call pci_dev_put() and the reference count of GPU and > > NPU PCI devices rockets up. > > > > A natural way to fix this would be to teach the callers about the change, > > so that they call pci_dev_put() when done with the pointer. This turns > > out to be quite intrusive, as it affects many paths in npu-dma.c, > > pci-ioda.c and vfio_pci_nvlink2.c. > > > afaict this referencing is only done to protect the current traverser > and what you've done is actually a natural way (and the generic > pci_get_dev_by_id() does exactly the same), although this looks a bit weird. > Not exactly the same: pci_get_dev_by_id() always increment the refcount of the returned PCI device. The refcount is only decremented when this device is passed to pci_get_dev_by_id() to continue searching. That means that the users of the PCI device pointer returned by pci_get_dev_by_id() or its exported variants pci_get_subsys(), pci_get_device() and pci_get_class() do handle the refcount. They all pass the pointer to pci_dev_put() or continue the search, which calls pci_dev_put() internally. Direct and indirect callers of get_pci_dev() don't care for the refcount at all unless I'm missing something. > > > Also, the issue appeared in 4.16 and > > some affected code got moved around since then: it would be problematic > > to backport the fix to stable releases. > > > > All that code never cared for reference counting anyway. Call pci_dev_put() > > from get_pci_dev() to revert to the previous behavior. > >> Fixes: 902bdc57451c ("powerpc/powernv/idoa: Remove unnecessary pcidev > from pci_dn") > > Cc: stable@vger.kernel.org # v4.16 > > Signed-off-by: Greg Kurz > > --- > > arch/powerpc/platforms/powernv/npu-dma.c | 15 ++++++++++++++- > > 1 file changed, 14 insertions(+), 1 deletion(-) > > > > diff --git a/arch/powerpc/platforms/powernv/npu-dma.c b/arch/powerpc/platforms/powernv/npu-dma.c > > index e713ade30087..d8f3647e8fb2 100644 > > --- a/arch/powerpc/platforms/powernv/npu-dma.c > > +++ b/arch/powerpc/platforms/powernv/npu-dma.c > > @@ -31,9 +31,22 @@ static DEFINE_SPINLOCK(npu_context_lock); > > static struct pci_dev *get_pci_dev(struct device_node *dn) > > { > > struct pci_dn *pdn = PCI_DN(dn); > > + struct pci_dev *pdev; > > > > - return pci_get_domain_bus_and_slot(pci_domain_nr(pdn->phb->bus), > > + pdev = pci_get_domain_bus_and_slot(pci_domain_nr(pdn->phb->bus), > > pdn->busno, pdn->devfn); > > + > > + /* > > + * pci_get_domain_bus_and_slot() increased the reference count of > > + * the PCI device, but callers don't need that actually as the PE > > + * already holds a reference to the device. > > Imho this would be just enough. > > Anyway, > > Reviewed-by: Alexey Kardashevskiy > Thanks ! I now realize that I forgot to add the --cc option for stable on my stgit command line :-\. Cc'ing now. > > How did you find it? :) > While reading code to find some inspiration for OpenCAPI passthrough. :) I saw the following in vfio_pci_ibm_npu2_init(): if (!pnv_pci_get_gpu_dev(vdev->pdev)) return -ENODEV; and simply followed the function calls. > > > Since callers aren't > > + * aware of the reference count change, call pci_dev_put() now to > > + * avoid leaks. > > + */ > > + if (pdev) > > + pci_dev_put(pdev); > > + > > + return pdev; > > } > > > > /* Given a NPU device get the associated PCI device. */ > > >