[dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
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. >> >> >
[dpdk-dev] [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 > 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. 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. > >
[dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
> -Original Message- > From: Gonzalez Monroy, Sergio > Sent: Thursday, April 21, 2016 1:19 PM > To: David Marchand > Cc: dev at dpdk.org; Kerlin, MarcinX > 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 > 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? 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. > > > >
[dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
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 can't see cleanup for previously mapped segments when mapping one fails. Do we want this cleanup as well ? CC Sergio. -- David Marchand
[dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
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); + 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) -- 1.9.1