| Message ID | 1461083251-31140-1-git-send-email-marcinx.kerlin@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id C9C302E81; Tue, 19 Apr 2016 17:19:43 +0200 (CEST) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id 08E342BC3 for <dev@dpdk.org>; Tue, 19 Apr 2016 17:19:41 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga103.jf.intel.com with ESMTP; 19 Apr 2016 08:19:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,506,1455004800"; d="scan'208";a="948456480" Received: from gklab-246-021.igk.intel.com (HELO HANLANCREEK9755-232) ([10.217.246.21]) by fmsmga001.fm.intel.com with SMTP; 19 Apr 2016 08:19:35 -0700 Received: by HANLANCREEK9755-232 (sSMTP sendmail emulation); Tue, 19 Apr 2016 18:27:43 +0200 From: Marcin Kerlin <marcinx.kerlin@intel.com> To: dev@dpdk.org Cc: david.marchand@6wind.com, Marcin Kerlin <marcinx.kerlin@intel.com> Date: Tue, 19 Apr 2016 18:27:31 +0200 Message-Id: <1461083251-31140-1-git-send-email-marcinx.kerlin@intel.com> X-Mailer: git-send-email 1.9.1 Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Marcin Kerlin
April 19, 2016, 4:27 p.m. UTC
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 <marcinx.kerlin@intel.com>
---
lib/librte_eal/linuxapp/eal/eal_memory.c | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
Comments
On Tue, Apr 19, 2016 at 6:27 PM, Marcin Kerlin <marcinx.kerlin@intel.com> 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 <marcinx.kerlin@intel.com> > --- > 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 can't see cleanup for previously mapped segments when mapping one fails. Do we want this cleanup as well ? CC Sergio.
On 20/04/2016 10:15, David Marchand wrote: > On Tue, Apr 19, 2016 at 6:27 PM, Marcin Kerlin <marcinx.kerlin@intel.com> 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 <marcinx.kerlin@intel.com> >> --- >> 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. 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. Sergio > CC Sergio. > >
> -----Original Message----- > From: Gonzalez Monroy, Sergio > Sent: Thursday, April 21, 2016 1:19 PM > To: David Marchand <david.marchand@6wind.com> > Cc: dev@dpdk.org; Kerlin, MarcinX <marcinx.kerlin@intel.com> > Subject: Re: [PATCH 1/1] lib/librte_eal: fix resource leak > > On 20/04/2016 10:15, David Marchand wrote: > > On Tue, Apr 19, 2016 at 6:27 PM, Marcin Kerlin <marcinx.kerlin@intel.com> > 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 <marcinx.kerlin@intel.com> > >> --- > >> 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? Oh sorry, right, an oversight with the redundant casting. > > > > > 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. > > 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. Exactly app panic after fail so do we need to worry about these warnings from Coverity and try to improve or leave it without affecting? > > Sergio > > CC Sergio. > > > >
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 >> <marcinx.kerlin@intel.com> 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 <marcinx.kerlin@intel.com> >>> --- >>> 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. >> >> >
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); + goto error; } } size = getFileSize(fd_hugepage); hp = mmap(NULL, size, PROT_READ, MAP_PRIVATE, fd_hugepage, 0); - if (hp == NULL) { + if (hp == MAP_FAILED) { RTE_LOG(ERR, EAL, "Could not mmap %s\n", eal_hugepage_info_path()); goto error; } @@ -1535,6 +1539,10 @@ rte_eal_hugepage_attach(void) addr != RTE_PTR_ADD(base_addr, offset)) { RTE_LOG(ERR, EAL, "Could not mmap %s\n", hp[i].filepath); + + if (addr != MAP_FAILED) + munmap((void *)(uintptr_t)addr, mapping_size); + goto error; } offset+=mapping_size; @@ -1551,6 +1559,8 @@ rte_eal_hugepage_attach(void) return 0; error: + if (hp != NULL && hp != MAP_FAILED) + munmap((void *) (uintptr_t) hp, size); if (fd_zero >= 0) close(fd_zero); if (fd_hugepage >= 0)