[dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak

[dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak

[Marcin Kerlin]

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);
+
 			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

Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak

[David Marchand]

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.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak

[Sergio Gonzalez Monroy]

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.
>
>

Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak

[Kerlin, MarcinX]

> -----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.
> >
> >

Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak

[Panu Matilainen]

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.
>>
>>
>

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

[Marcin Kerlin]

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");
 			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)
-- 
1.9.1

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

[Sergio Gonzalez Monroy]

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)

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

[Kerlin, MarcinX]

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)
> 

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

[Panu Matilainen]

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 -

[dpdk-dev] [PATCH v3 1/1] eal: fix resource leak of mapped memory

[Marcin Kerlin]

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".

V3:
1)removed redundant casting
2)removed update error message
V2:
1)unmapping also previous addresses 

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..44ff8e1 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -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;
 	}
@@ -1545,12 +1545,19 @@ rte_eal_hugepage_attach(void)
 		s++;
 	}
 	/* unmap the hugepage config file, since we are done using it */
-	munmap((void *)(uintptr_t)hp, size);
+	munmap((void *)hp, size);
 	close(fd_zero);
 	close(fd_hugepage);
 	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 *)hp, size);
 	if (fd_zero >= 0)
 		close(fd_zero);
 	if (fd_hugepage >= 0)
-- 
1.9.1

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

[Kerlin, MarcinX]

> -----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 -

[dpdk-dev] [PATCH v4 1/1] eal: fix resource leak of mapped memory

[Marcin Kerlin]

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".

v4:
1)removed keyword const from pointer and dependent on that casting (void *) 
v3:
1)removed redundant casting
2)removed update error message
v2:
1)unmapping also previous addresses

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 | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 79d1d2d..c935765 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1399,7 +1399,7 @@ int
 rte_eal_hugepage_attach(void)
 {
 	const struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
-	const struct hugepage_file *hp = NULL;
+	struct hugepage_file *hp = NULL;
 	unsigned num_hp = 0;
 	unsigned i, s = 0; /* s used to track the segment number */
 	off_t size;
@@ -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;
 	}
@@ -1545,12 +1545,19 @@ rte_eal_hugepage_attach(void)
 		s++;
 	}
 	/* unmap the hugepage config file, since we are done using it */
-	munmap((void *)(uintptr_t)hp, size);
+	munmap(hp, size);
 	close(fd_zero);
 	close(fd_hugepage);
 	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(hp, size);
 	if (fd_zero >= 0)
 		close(fd_zero);
 	if (fd_hugepage >= 0)
-- 
1.9.1

Re: [dpdk-dev] [PATCH v4 1/1] eal: fix resource leak of mapped memory

[Sergio Gonzalez Monroy]

On 15/06/2016 13:25, 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".
>
> v4:
> 1)removed keyword const from pointer and dependent on that casting (void *)
> v3:
> 1)removed redundant casting
> 2)removed update error message
> v2:
> 1)unmapping also previous addresses

The patch version history should be after the triple dash below so it 
won't show up
on git log.

> Coverity issue: 13295, 13296, 13303
> Fixes: af75078fece3 ("first public release")
>
> Signed-off-by: Marcin Kerlin <marcinx.kerlin@intel.com>
> ---

Insert here patch version history.

>   lib/librte_eal/linuxapp/eal/eal_memory.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
> index 79d1d2d..c935765 100644

Thomas, are you ok to update the commit message? Otherwise, please 
Mercin do v5 with changes and keep my ack.

Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

[dpdk-dev] [PATCH v5 1/1] eal: fix resource leak of mapped memory

[Marcin Kerlin]

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>
Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>
---
v5:
 -shift the history of changes
v4:
 -removed keyword const from pointer and dependent on that casting (void *)
v3:
 -removed redundant casting
 -removed update error message
v2:
 -unmapping also previous addresses

 lib/librte_eal/linuxapp/eal/eal_memory.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 79d1d2d..c935765 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1399,7 +1399,7 @@ int
 rte_eal_hugepage_attach(void)
 {
 	const struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
-	const struct hugepage_file *hp = NULL;
+	struct hugepage_file *hp = NULL;
 	unsigned num_hp = 0;
 	unsigned i, s = 0; /* s used to track the segment number */
 	off_t size;
@@ -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;
 	}
@@ -1545,12 +1545,19 @@ rte_eal_hugepage_attach(void)
 		s++;
 	}
 	/* unmap the hugepage config file, since we are done using it */
-	munmap((void *)(uintptr_t)hp, size);
+	munmap(hp, size);
 	close(fd_zero);
 	close(fd_hugepage);
 	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(hp, size);
 	if (fd_zero >= 0)
 		close(fd_zero);
 	if (fd_hugepage >= 0)
-- 
1.9.1

Re: [dpdk-dev] [PATCH v5 1/1] eal: fix resource leak of mapped memory

[Thomas Monjalon]

2016-06-16 17:14, Marcin Kerlin:
> 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>
> Acked-by: Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>

Applied, thanks