diff mbox

[dpdk-dev,v2,1/1] eal: fix resource leak of mapped memory

Message ID 1465918435-30973-1-git-send-email-marcinx.kerlin@intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Marcin Kerlin June 14, 2016, 3:33 p.m. UTC 
  Patch fixes resource leak in rte_eal_hugepage_attach() where mapped files
were not freed back to the OS in case of failure. Patch uses the behavior
of Linux munmap: "It is not an error if the indicated range does not 
contain any mapped pages".

Coverity issue: 13295, 13296, 13303
Fixes: af75078fece3 ("first public release")

Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Sergio Gonzalez Monroy June 15, 2016, 8:49 a.m. UTC | #1 
Hi Marcin,

On 14/06/2016 16:33, Marcin Kerlin wrote:
> Patch fixes resource leak in rte_eal_hugepage_attach() where mapped files
> were not freed back to the OS in case of failure. Patch uses the behavior
> of Linux munmap: "It is not an error if the indicated range does not
> contain any mapped pages".
>
> Coverity issue: 13295, 13296, 13303
> Fixes: af75078fece3 ("first public release")
>
> Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
> ---
>   lib/librte_eal/linuxapp/eal/eal_memory.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index 79d1d2d..1834fa0 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> @@ -1417,7 +1417,7 @@ rte_eal_hugepage_attach(void)
>   	if (internal_config.xen_dom0_support) {
>   #ifdef RTE_LIBRTE_XEN_DOM0
>   		if (rte_xen_dom0_memory_attach() < 0) {
> -			RTE_LOG(ERR, EAL,"Failed to attach memory setments of primay "
> +			RTE_LOG(ERR, EAL, "Failed to attach memory segments of primary "
>   					"process\n");

If you want to fix spelling of error message it probably should go in a 
different patch.

>   			return -1;
>   		}
> @@ -1481,7 +1481,7 @@ rte_eal_hugepage_attach(void)
>   
>   	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;
>   	}
> @@ -1551,6 +1551,13 @@ rte_eal_hugepage_attach(void)
>   	return 0;
>   
>   error:
> +	s = 0;
> +	while (s < RTE_MAX_MEMSEG && mcfg->memseg[s].len > 0) {
> +		munmap(mcfg->memseg[s].addr, mcfg->memseg[s].len);
> +		s++;
> +	}
> +	if (hp != NULL && hp != MAP_FAILED)
> +		munmap((void *)(uintptr_t)hp, size);

No need for double casting, nor to cast to (void *).

Sergio

>   	if (fd_zero >= 0)
>   		close(fd_zero);
>   	if (fd_hugepage >= 0)
Marcin Kerlin June 15, 2016, 9:35 a.m. UTC | #2 
Hi Sergio,

Thanks for tips, I removed double casting and I leave (void *) casting because pointer hp is const and compiler returns warnings.

Regards,
Marcin

> -----Original Message-----
> From: Gonzalez Monroy, Sergio
> Sent: Wednesday, June 15, 2016 10:49 AM
> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>; david.marchand@6wind.com
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2 1/1] eal: fix resource leak of mapped memory
> 
> Hi Marcin,
> 
> On 14/06/2016 16:33, Marcin Kerlin wrote:
> > Patch fixes resource leak in rte_eal_hugepage_attach() where mapped
> > files were not freed back to the OS in case of failure. Patch uses the
> > behavior of Linux munmap: "It is not an error if the indicated range
> > does not contain any mapped pages".
> >
> > Coverity issue: 13295, 13296, 13303
> > Fixes: af75078fece3 ("first public release")
> >
> > Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
> > ---
> >   lib/librte_eal/linuxapp/eal/eal_memory.c | 11 +++++++++--
> >   1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c
> > b/lib/librte_eal/linuxapp/eal/eal_memory.c
> > index 79d1d2d..1834fa0 100644
> > --- a/lib/librte_eal/linuxapp/eal/eal_memory.c
> > +++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
> > @@ -1417,7 +1417,7 @@ rte_eal_hugepage_attach(void)
> >   	if (internal_config.xen_dom0_support) {
> >   #ifdef RTE_LIBRTE_XEN_DOM0
> >   		if (rte_xen_dom0_memory_attach() < 0) {
> > -			RTE_LOG(ERR, EAL,"Failed to attach memory setments
> of primay "
> > +			RTE_LOG(ERR, EAL, "Failed to attach memory
> segments of primary "
> >   					"process\n");
> 
> If you want to fix spelling of error message it probably should go in a different
> patch.
> 
> >   			return -1;
> >   		}
> > @@ -1481,7 +1481,7 @@ rte_eal_hugepage_attach(void)
> >
> >   	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;
> >   	}
> > @@ -1551,6 +1551,13 @@ rte_eal_hugepage_attach(void)
> >   	return 0;
> >
> >   error:
> > +	s = 0;
> > +	while (s < RTE_MAX_MEMSEG && mcfg->memseg[s].len > 0) {
> > +		munmap(mcfg->memseg[s].addr, mcfg->memseg[s].len);
> > +		s++;
> > +	}
> > +	if (hp != NULL && hp != MAP_FAILED)
> > +		munmap((void *)(uintptr_t)hp, size);
> 
> No need for double casting, nor to cast to (void *).
> 
> Sergio
> 
> >   	if (fd_zero >= 0)
> >   		close(fd_zero);
> >   	if (fd_hugepage >= 0)
>
Panu Matilainen June 15, 2016, 10:05 a.m. UTC | #3 
On 06/15/2016 12:35 PM, Kerlin, MarcinX wrote:
> Hi Sergio,
>
> Thanks for tips, I removed double casting and I leave (void *) casting because pointer hp is const and compiler returns warnings.

If hp is something that needs freeing then it shouldn't be const in the 
first place. Please fix that instead.

	- Panu -
Marcin Kerlin June 15, 2016, 11:13 a.m. UTC | #4 
> -----Original Message-----

> From: Panu Matilainen [mailto:pmatilai@redhat.com]

> Sent: Wednesday, June 15, 2016 12:06 PM

> To: Kerlin, MarcinX <marcinx.kerlin@intel.com>; Gonzalez Monroy, Sergio

> <sergio.gonzalez.monroy@intel.com>; david.marchand@6wind.com

> Cc: dev@dpdk.org

> Subject: Re: [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped

> memory

> 

> On 06/15/2016 12:35 PM, Kerlin, MarcinX wrote:

> > Hi Sergio,

> >

> > Thanks for tips, I removed double casting and I leave (void *) casting because

> pointer hp is const and compiler returns warnings.

> 

> If hp is something that needs freeing then it shouldn't be const in the first place.

> Please fix that instead.


Thanks, done

> 

> 	- Panu -
diff mbox

Patch

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 79d1d2d..1834fa0 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1417,7 +1417,7 @@  rte_eal_hugepage_attach(void)
 	if (internal_config.xen_dom0_support) {
 #ifdef RTE_LIBRTE_XEN_DOM0
 		if (rte_xen_dom0_memory_attach() < 0) {
-			RTE_LOG(ERR, EAL,"Failed to attach memory setments of primay "
+			RTE_LOG(ERR, EAL, "Failed to attach memory segments of primary "
 					"process\n");
 			return -1;
 		}
@@ -1481,7 +1481,7 @@  rte_eal_hugepage_attach(void)
 
 	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;
 	}
@@ -1551,6 +1551,13 @@  rte_eal_hugepage_attach(void)
 	return 0;
 
 error:
+	s = 0;
+	while (s < RTE_MAX_MEMSEG && mcfg->memseg[s].len > 0) {
+		munmap(mcfg->memseg[s].addr, mcfg->memseg[s].len);
+		s++;
+	}
+	if (hp != NULL && hp != MAP_FAILED)
+		munmap((void *)(uintptr_t)hp, size);
 	if (fd_zero >= 0)
 		close(fd_zero);
 	if (fd_hugepage >= 0)