From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id 91E9D2C4B for ; Fri, 22 Apr 2016 12:42:13 +0200 (CEST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 103E33B3C0; Fri, 22 Apr 2016 10:42:13 +0000 (UTC) Received: from sopuli.koti.laiskiainen.org (vpn1-5-182.ams2.redhat.com [10.36.5.182]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u3MAgBG3012812; Fri, 22 Apr 2016 06:42:12 -0400 To: Sergio Gonzalez Monroy , David Marchand References: <1461083251-31140-1-git-send-email-marcinx.kerlin@intel.com> <5718B726.5040300@intel.com> Cc: "dev@dpdk.org" , Marcin Kerlin From: Panu Matilainen Message-ID: <571A0003.1040202@redhat.com> Date: Fri, 22 Apr 2016 13:42:11 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 MIME-Version: 1.0 In-Reply-To: <5718B726.5040300@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 Subject: Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Apr 2016 10:42:13 -0000 On 04/21/2016 02:19 PM, Sergio Gonzalez Monroy wrote: > On 20/04/2016 10:15, David Marchand wrote: >> On Tue, Apr 19, 2016 at 6:27 PM, Marcin Kerlin >> wrote: >>> Fix issue reported by Coverity. >>> >>> Coverity ID 13295, 13296, 13303: >>> Resource leak: The system resource will not be reclaimed >>> and reused, reducing the future availability of the resource. >>> In rte_eal_hugepage_attach: Leak of memory or pointers to system >>> resources. >>> >>> Fixes: af75078fece3 ("first public release") >>> >>> Signed-off-by: Marcin Kerlin >>> --- >>> lib/librte_eal/linuxapp/eal/eal_memory.c | 12 +++++++++++- >>> 1 file changed, 11 insertions(+), 1 deletion(-) >>> >>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c >>> b/lib/librte_eal/linuxapp/eal/eal_memory.c >>> index 5b9132c..6320aa0 100644 >>> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c >>> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c >>> @@ -1475,13 +1475,17 @@ rte_eal_hugepage_attach(void) >>> "and retry running both >>> primary " >>> "and secondary processes\n"); >>> } >>> + >>> + if (base_addr != MAP_FAILED) >>> + munmap((void *)(uintptr_t)base_addr, >>> mcfg->memseg[s].len); >>> + >> What is the point of this casting ? >> Idem for the rest of the patch. > > I don't see the point either. > Marcin? > >> >> I can't see cleanup for previously mapped segments when mapping one >> fails. >> Do we want this cleanup as well ? > > Good point. > > We are not really leaking resources because we panic if we fail to > initialize eal memory. FWIW, the panic-attack mode is something I'd like to see eliminated eventually and hopefully will be submitting patches for sooner or later. Aborting from library code is rather antisocial behavior, even if its on just initialization code that usually runs fairly early in process lifetime. > > Now, if we are going to do the cleanup, I think that as David points out > we should be > cleaning up all previous mappings too. +1 Even if the current code just panics, it doesn't mean it always will. - Panu - > > Sergio >> CC Sergio. >> >> >