From patchwork Thu Dec 3 16:22:37 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Micha=C5=82_Nazarewicz?= X-Patchwork-Id: 7761711 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 1578C9F30B for ; Thu, 3 Dec 2015 16:25:30 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B724A20511 for ; Thu, 3 Dec 2015 16:25:28 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 54E002050E for ; Thu, 3 Dec 2015 16:25:27 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1a4Wf0-0002TD-N4; Thu, 03 Dec 2015 16:23:10 +0000 Received: from mail-wm0-x22c.google.com ([2a00:1450:400c:c09::22c]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1a4Wes-0002E3-8U for linux-arm-kernel@lists.infradead.org; Thu, 03 Dec 2015 16:23:07 +0000 Received: by wmww144 with SMTP id w144so29254664wmw.0 for ; Thu, 03 Dec 2015 08:22:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=sender:from:to:cc:subject:in-reply-to:organization:references :user-agent:face:date:message-id:mime-version:content-type :content-transfer-encoding; bh=d6Bj8vdoA0htYjKfRCKR1Ig2QFOIGLOrRxwqN7abuNo=; b=oMikfdxshPGK0L811RSt2u+FF/uwYr3CM4UsHJCn956zpjDl5BL63EHr4jMV38SuR9 DkKuRXgoNIOnr2lvRbjyt0ERPijXWEWzUSAIJJR2BIyFX5WFSoFwgiAmv7fKDxXq6C6A fNgzTBPxYzllzMJniptQn1RUcUrtvlHm0DBZCKOInKBvPPpwpQfIVzmxOxpk5ldEVf/E mVN7SxLuSeLJWuqJoX7/H1bgye1ymlMFH+AxqyH7gix/GEKB5VLAc1qIkyj6qNpavbxR o1Ro/MxDpFNP6vadHa2Ec7RDGQuJtCBbI5c4gUmHjYdMlLpu1tqCLD6S3wq3CyE7wFYX Nbzg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:in-reply-to :organization:references:user-agent:face:date:message-id :mime-version:content-type:content-transfer-encoding; bh=d6Bj8vdoA0htYjKfRCKR1Ig2QFOIGLOrRxwqN7abuNo=; b=E9Su3DH06dJlkNr6r0+yKfvQnFSUBECkO6aRjbQexZ80YOVKFl0AF6MXJpnedwFIMG xJi/gDF0SWTmBfQeGPENTQdXzgH9sIiei64uvoHNhk4bvwGgFtZeYc1s9l+4LE3iQsoc 55WbujWxDoU5MxVAsfr+DOMpG/icTATZ1olUXTt8yp+uTBm+gpjW9sAZFEASzsp0EsQn gekoI03kok1wiDbmBjWMj5SdLxv2a3ei5wKV7dalOP3n39KgKFdvixQ1sJqU4atb1CSw p7xRweyLDaq596gvYD0bcFEs6v8r0oTf4q6Cqq3h3vg5NUgfbyNdHkaP21VWRJXXbBV0 yMjw== X-Gm-Message-State: ALoCoQmimSkJw/gmzPNaxeWA9y/L3cQK2WAAFUg3BFNrv5Gj/7hUyTs4/QKqpPTUk2EXftJZITGI X-Received: by 10.28.232.136 with SMTP id f8mr55432474wmi.1.1449159759964; Thu, 03 Dec 2015 08:22:39 -0800 (PST) Received: from mpn-glaptop ([2620:0:105f:302:3d46:a36d:3ead:bb5e]) by smtp.gmail.com with ESMTPSA id w141sm35960508wmw.24.2015.12.03.08.22.38 (version=TLS1_2 cipher=AES128-SHA bits=128/128); Thu, 03 Dec 2015 08:22:39 -0800 (PST) From: Michal Nazarewicz To: Russell King - ARM Linux , Duan Andy Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to init DMA memory pool In-Reply-To: <20151203153937.GP8644@n2100.arm.linux.org.uk> Organization: http://mina86.com/ References: <1449138962-8264-1-git-send-email-b38611@freescale.com> <20151203153937.GP8644@n2100.arm.linux.org.uk> User-Agent: Notmuch/0.19+53~g2e63a09 (http://notmuchmail.org) Emacs/25.1.50.1 (x86_64-unknown-linux-gnu) Face: iVBORw0KGgoAAAANSUhEUgAAADAAAAAwBAMAAAClLOS0AAAAJFBMVEWbfGlUPDDHgE57V0jUupKjgIObY0PLrom9mH4dFRK4gmjPs41MxjOgAAACP0lEQVQ4T23Sv2vbQBQHcBk1xE6WyALX107VUEgmn6+ouUwpEQQ6uRjttkWP4CkBg2M0BQLBdPFZYPsyFYo7qEtKDQ7on+t7+nF2Ux8ahD587717OmNYrOvycHsZ+o2r051wHTHysAvGb8ygvgu4QWT0sCmkgZCIEnlV2X8BtyraazFGDuxhmKSQJMlwHQ7v5MHSNxmz78rfElwAa3ieVD9e+hBhjaPDDG6NgFo2f4wBMNIo5YmRtF0RyDgFjJjlMIWbnuM4x9MMfABGTlN4qgIQB4A1DEyA1BHWtfeWNUMwiVJKoqh97KrkOO+qzgluVYLvFCUKAX73nONeBr7BGMdM6Sg0kuep03VywLaIzRiVr+GAzKlpQIsAFnWAG2e6DT5WmWDiudZMIc6hYrMOmeMQK9WX0B+/RfjzL9DI7Y9/Iayn29Ci0r2i4f9gMimMSZLCDMalgQGU5hnUtqAN0OGvEmO1Wnl0C0wWSCEHnuHBqmygxdxA8oWXwbipoc1EoNR9DqOpBpOJrnr0criQab9ZT4LL+wI+K7GBQH30CrhUruilgP9DRTrhVWZCiAyILP+wiuLeCKGTD6r/nc8LOJcAwR6IBTUs+7CASw3QFZ0MdA2PI3zNziH4ZKVhXCRMBjeZ1DWMekKwDCASwExy+NQ86TaykaDAFHO4aP48y4fIcDM5yOG8GcTLbOyp8A8azjJI93JFd1EA6yN8sSxMQJWoABqniRZVykYgRXErzrdqExAoUrRb0xfRp8p2A/4XmfilTtkDZ4cAAAAASUVORK5CYII= X-Face: -TR8(rDTHy/(xl?SfWd1|3:TTgDIatE^t'vop%*gVg[kn$t{EpK(P"VQ=~T2#ysNmJKN$"yTRLB4YQs$4{[.]Fc1)*O]3+XO^oXM>Q#b^ix, O)Zbn)q[y06$`e3?C)`CwR9y5riE=fv^X@x$y?D:XO6L&x4f-}}I4=VRNwiA^t1-ZrVK^07.Pi/57c_du'& X-PGP: 50751FF4 X-PGP-FP: AC1F 5F5C D418 88F8 CC84 5858 2060 4012 5075 1FF4 X-Hashcash: 1:20:151203:arnd@arndb.de::TtQtzY1Bu9tzoaOI:00000Sop X-Hashcash: 1:20:151203:linux@arm.linux.org.uk::k1wuzWV+Afsh2net:00000000000000000000000000000000000000000a/ X-Hashcash: 1:20:151203:iamjoonsoo.kim@lge.com::EufhnP4JMZiDQVrc:0000000000000000000000000000000000000001ERD X-Hashcash: 1:20:151203:m.szyprowski@samsung.com::08R1EmIlLFmtcCwl:00000000000000000000000000000000000001b9+ X-Hashcash: 1:20:151203:fugang.duan@freescale.com::Ll2lBP0647k/0t1g:0000000000000000000000000000000000002jpd X-Hashcash: 1:20:151203:torvalds@linux-foundation.org::ZILbl6qcLVOpHYGR:000000000000000000000000000000001s2g X-Hashcash: 1:20:151203:gregkh@linuxfoundation.org::f9zrYMizUnV2QaQ6:000000000000000000000000000000000003Bxs X-Hashcash: 1:20:151203:linux-arm-kernel@lists.infradead.org::7+APwxjapMzlBgj0:00000000000000000000000003ajF Date: Thu, 03 Dec 2015 17:22:37 +0100 Message-ID: MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20151203_082302_880507_C02F1561 X-CRM114-Status: GOOD ( 32.20 ) X-Spam-Score: -2.6 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "arnd@arndb.de" , "gregkh@linuxfoundation.org" , "iamjoonsoo.kim@lge.com" , "torvalds@linux-foundation.org" , "linux-arm-kernel@lists.infradead.org" , "m.szyprowski@samsung.com" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Thu, Dec 03 2015, Russell King - ARM Linux wrote: > On Thu, Dec 03, 2015 at 01:54:05PM +0000, Duan Andy wrote: >> From: Mike Nazarewicz Sent: Thursday, December 03, 2015 8:51 PM >> > To: Duan Fugang-B38611; torvalds@linux-foundation.org; >> > gregkh@linuxfoundation.org; m.szyprowski@samsung.com >> > Cc: linux-arm-kernel@lists.infradead.org; arnd@arndb.de; >> > iamjoonsoo.kim@lge.com; Duan Fugang-B38611 >> > Subject: Re: [PATCH] drivers: dma-coherent: free memory when failed to >> > init DMA memory pool >> > >> > On Thu, Dec 03 2015, Fugang Duan wrote: >> > > Free dma coherent memory when it failed to init DMA memory pool after >> > > calling .dma_init_coherent_memory(), otherwise it causes memmory leak. >> > > >> > > Signed-off-by: Fugang Duan >> > > --- >> > > drivers/base/dma-coherent.c | 1 + >> > > 1 file changed, 1 insertion(+) >> > > >> > > diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c >> > > index 55b8398..beb6bbe 100644 >> > > --- a/drivers/base/dma-coherent.c >> > > +++ b/drivers/base/dma-coherent.c >> > > @@ -286,6 +286,7 @@ static int rmem_dma_device_init(struct reserved_mem >> > *rmem, struct device *dev) >> > > &mem) != DMA_MEMORY_MAP) { >> > > pr_err("Reserved memory: failed to init DMA memory pool >> > at %pa, size %ld MiB\n", >> > > &rmem->base, (unsigned long)rmem->size / SZ_1M); >> > > + kfree(mem); >> > >> > mem == NULL at this point. If dma_init_coherent_memory doesn’t return >> > DMA_MEMORY_MAP, mem pointer is not assigned any value. If you think >> > there is a memory leak, please demonstrate a path of it happening. >> > >> Kfree() will check mem NULL condition, so no need to add check in here. > > That isn't what the reviewer was stating. > > However, had the reviewer looked at the existing code, then he may have > stated a different opinion. :) I’m not sure that I would. Here’s code I’m looking at: static int dma_init_coherent_memory(phys_addr_t phys_addr, dma_addr_t device_addr, size_t size, int flags, struct dma_coherent_mem **mem) { struct dma_coherent_mem *dma_mem = NULL; void __iomem *mem_base = NULL; int pages = size >> PAGE_SHIFT; int bitmap_size = BITS_TO_LONGS(pages) * sizeof(long); if ((flags & (DMA_MEMORY_MAP | DMA_MEMORY_IO)) == 0) goto out; if (!size) goto out; mem_base = ioremap(phys_addr, size); if (!mem_base) goto out; dma_mem = kzalloc(sizeof(struct dma_coherent_mem), GFP_KERNEL); if (!dma_mem) goto out; dma_mem->bitmap = kzalloc(bitmap_size, GFP_KERNEL); if (!dma_mem->bitmap) goto out; dma_mem->virt_base = mem_base; dma_mem->device_base = device_addr; dma_mem->pfn_base = PFN_DOWN(phys_addr); dma_mem->size = pages; dma_mem->flags = flags; spin_lock_init(&dma_mem->spinlock); /**************************************************************/ /* Here's the only place where mem is set to non-NULL value. */ /**************************************************************/ *mem = dma_mem; if (flags & DMA_MEMORY_MAP) /******************************************************/ /* Here's where function returns. */ /******************************************************/ return DMA_MEMORY_MAP; return DMA_MEMORY_IO; out: kfree(dma_mem); if (mem_base) iounmap(mem_base); return 0; } static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) { struct dma_coherent_mem *mem = rmem->priv; if (!mem && /**********************************************************/ /* Note that flags & DMA_MEMORY_MAP is non-zero. */ /**********************************************************/ dma_init_coherent_memory(rmem->base, rmem->base, rmem->size, DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE, &mem) != DMA_MEMORY_MAP) { pr_err("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n", &rmem->base, (unsigned long)rmem->size / SZ_1M); return -ENODEV; } rmem->priv = mem; dma_assign_coherent_memory(dev, mem); return 0; } > When you look at the existing code, there are three possible return > values from dma_init_coherent_memory(): > > - zero, which means failure, and the mem pointer is left alone. In > the above code, we know that it's guaranteed to be NULL, as we > won't get into the if() unless it was. > - DMA_MEMORY_IO, which would mean that mem points at a kmalloc()d > chunk of memory, and this case is treated as failure because we > want memory. No. This never happens in the code that is being patched. flags & DMA_MEMORY_MAP is non-zero thus dma_init_coherent_memory returns DMA_MEMORY_MAP. > - DMA_MEMORY_MAP, which also means that mem points at a kmalloc()d > chunk of memory, which we treat as success. > > So, in the case of DMA_MEMORY_IO, Which never happens. > we have a failure case where the kmalloc'd memory is dropped on the > floor - aka a memory leak. To me it seems that a better patch would be the following (not tested in any capacity): From 49cece0c0a052022022776f2555d26308deba959 Mon Sep 17 00:00:00 2001 From: Michal Nazarewicz Date: Thu, 3 Dec 2015 17:15:07 +0100 Subject: [PATCH] drivers: dma-coherent: simplify dma_init_coherent_memory return value Since only dma_declare_coherent_memory cares about dma_init_coherent_memory returning part of flags as it return value, move the condition to the former and simplify the latter. This in turn makes rmem_dma_device_init less confusing. Signed-off-by: Michal Nazarewicz --- drivers/base/dma-coherent.c | 28 +++++++++++----------------- 1 file changed, 11 insertions(+), 17 deletions(-) \o/ negative delta. diff --git a/drivers/base/dma-coherent.c b/drivers/base/dma-coherent.c index 55b8398..87b8083 100644 --- a/drivers/base/dma-coherent.c +++ b/drivers/base/dma-coherent.c @@ -17,9 +17,9 @@ struct dma_coherent_mem { spinlock_t spinlock; }; -static int dma_init_coherent_memory(phys_addr_t phys_addr, dma_addr_t device_addr, - size_t size, int flags, - struct dma_coherent_mem **mem) +static bool dma_init_coherent_memory( + phys_addr_t phys_addr, dma_addr_t device_addr, size_t size, int flags, + struct dma_coherent_mem **mem) { struct dma_coherent_mem *dma_mem = NULL; void __iomem *mem_base = NULL; @@ -50,17 +50,13 @@ static int dma_init_coherent_memory(phys_addr_t phys_addr, dma_addr_t device_add spin_lock_init(&dma_mem->spinlock); *mem = dma_mem; - - if (flags & DMA_MEMORY_MAP) - return DMA_MEMORY_MAP; - - return DMA_MEMORY_IO; + return true; out: kfree(dma_mem); if (mem_base) iounmap(mem_base); - return 0; + return false; } static void dma_release_coherent_memory(struct dma_coherent_mem *mem) @@ -88,15 +84,13 @@ int dma_declare_coherent_memory(struct device *dev, phys_addr_t phys_addr, dma_addr_t device_addr, size_t size, int flags) { struct dma_coherent_mem *mem; - int ret; - ret = dma_init_coherent_memory(phys_addr, device_addr, size, flags, - &mem); - if (ret == 0) + if (!dma_init_coherent_memory(phys_addr, device_addr, size, flags, + &mem)) return 0; if (dma_assign_coherent_memory(dev, mem) == 0) - return ret; + return flags & DMA_MEMORY_MAP ? DMA_MEMORY_MAP : DMA_MEMORY_IO; dma_release_coherent_memory(mem); return 0; @@ -281,9 +275,9 @@ static int rmem_dma_device_init(struct reserved_mem *rmem, struct device *dev) struct dma_coherent_mem *mem = rmem->priv; if (!mem && - dma_init_coherent_memory(rmem->base, rmem->base, rmem->size, - DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE, - &mem) != DMA_MEMORY_MAP) { + !dma_init_coherent_memory(rmem->base, rmem->base, rmem->size, + DMA_MEMORY_MAP | DMA_MEMORY_EXCLUSIVE, + &mem)) { pr_err("Reserved memory: failed to init DMA memory pool at %pa, size %ld MiB\n", &rmem->base, (unsigned long)rmem->size / SZ_1M); return -ENODEV;