DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
@ 2016-04-19 16:27 Marcin Kerlin
  2016-04-20  9:15 ` David Marchand
  2016-06-14 15:33 ` [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory Marcin Kerlin
  0 siblings, 2 replies; 15+ messages in thread
From: Marcin Kerlin @ 2016-04-19 16:27 UTC (permalink / raw)
  To: dev; +Cc: david.marchand, 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
  2016-04-19 16:27 [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak Marcin Kerlin
@ 2016-04-20  9:15 ` David Marchand
  2016-04-21 11:19   ` Sergio Gonzalez Monroy
  2016-06-14 15:33 ` [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory Marcin Kerlin
  1 sibling, 1 reply; 15+ messages in thread
From: David Marchand @ 2016-04-20  9:15 UTC (permalink / raw)
  To: Marcin Kerlin; +Cc: dev, Sergio Gonzalez Monroy

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
  2016-04-20  9:15 ` David Marchand
@ 2016-04-21 11:19   ` Sergio Gonzalez Monroy
  2016-04-21 11:49     ` Kerlin, MarcinX
  2016-04-22 10:42     ` Panu Matilainen
  0 siblings, 2 replies; 15+ messages in thread
From: Sergio Gonzalez Monroy @ 2016-04-21 11:19 UTC (permalink / raw)
  To: David Marchand; +Cc: dev, Marcin Kerlin

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
  2016-04-21 11:19   ` Sergio Gonzalez Monroy
@ 2016-04-21 11:49     ` Kerlin, MarcinX
  2016-04-22 10:42     ` Panu Matilainen
  1 sibling, 0 replies; 15+ messages in thread
From: Kerlin, MarcinX @ 2016-04-21 11:49 UTC (permalink / raw)
  To: Gonzalez Monroy, Sergio, David Marchand; +Cc: dev

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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak
  2016-04-21 11:19   ` Sergio Gonzalez Monroy
  2016-04-21 11:49     ` Kerlin, MarcinX
@ 2016-04-22 10:42     ` Panu Matilainen
  1 sibling, 0 replies; 15+ messages in thread
From: Panu Matilainen @ 2016-04-22 10:42 UTC (permalink / raw)
  To: Sergio Gonzalez Monroy, David Marchand; +Cc: dev, Marcin Kerlin

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory
  2016-04-19 16:27 [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak Marcin Kerlin
  2016-04-20  9:15 ` David Marchand
@ 2016-06-14 15:33 ` Marcin Kerlin
  2016-06-15  8:49   ` Sergio Gonzalez Monroy
  2016-06-15 10:45   ` [dpdk-dev] [PATCH v3 " Marcin Kerlin
  1 sibling, 2 replies; 15+ messages in thread
From: Marcin Kerlin @ 2016-06-14 15:33 UTC (permalink / raw)
  To: david.marchand; +Cc: dev, sergio.gonzalez.monroy, 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory
  2016-06-14 15:33 ` [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory Marcin Kerlin
@ 2016-06-15  8:49   ` Sergio Gonzalez Monroy
  2016-06-15  9:35     ` Kerlin, MarcinX
  2016-06-15 10:45   ` [dpdk-dev] [PATCH v3 " Marcin Kerlin
  1 sibling, 1 reply; 15+ messages in thread
From: Sergio Gonzalez Monroy @ 2016-06-15  8:49 UTC (permalink / raw)
  To: Marcin Kerlin, david.marchand; +Cc: dev

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)

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory
  2016-06-15  8:49   ` Sergio Gonzalez Monroy
@ 2016-06-15  9:35     ` Kerlin, MarcinX
  2016-06-15 10:05       ` Panu Matilainen
  0 siblings, 1 reply; 15+ messages in thread
From: Kerlin, MarcinX @ 2016-06-15  9:35 UTC (permalink / raw)
  To: Gonzalez Monroy, Sergio, david.marchand; +Cc: dev

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory
  2016-06-15  9:35     ` Kerlin, MarcinX
@ 2016-06-15 10:05       ` Panu Matilainen
  2016-06-15 11:13         ` Kerlin, MarcinX
  0 siblings, 1 reply; 15+ messages in thread
From: Panu Matilainen @ 2016-06-15 10:05 UTC (permalink / raw)
  To: Kerlin, MarcinX, Gonzalez Monroy, Sergio, david.marchand; +Cc: dev

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 -

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v3 1/1] eal: fix resource leak of mapped memory
  2016-06-14 15:33 ` [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory Marcin Kerlin
  2016-06-15  8:49   ` Sergio Gonzalez Monroy
@ 2016-06-15 10:45   ` Marcin Kerlin
  2016-06-15 12:25     ` [dpdk-dev] [PATCH v4 " Marcin Kerlin
  1 sibling, 1 reply; 15+ messages in thread
From: Marcin Kerlin @ 2016-06-15 10:45 UTC (permalink / raw)
  To: david.marchand; +Cc: dev, sergio.gonzalez.monroy, 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory
  2016-06-15 10:05       ` Panu Matilainen
@ 2016-06-15 11:13         ` Kerlin, MarcinX
  0 siblings, 0 replies; 15+ messages in thread
From: Kerlin, MarcinX @ 2016-06-15 11:13 UTC (permalink / raw)
  To: Panu Matilainen, Gonzalez Monroy, Sergio, david.marchand; +Cc: dev



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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v4 1/1] eal: fix resource leak of mapped memory
  2016-06-15 10:45   ` [dpdk-dev] [PATCH v3 " Marcin Kerlin
@ 2016-06-15 12:25     ` Marcin Kerlin
  2016-06-16 12:57       ` Sergio Gonzalez Monroy
  2016-06-16 15:14       ` [dpdk-dev] [PATCH v5 " Marcin Kerlin
  0 siblings, 2 replies; 15+ messages in thread
From: Marcin Kerlin @ 2016-06-15 12:25 UTC (permalink / raw)
  To: david.marchand; +Cc: dev, sergio.gonzalez.monroy, 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v4 1/1] eal: fix resource leak of mapped memory
  2016-06-15 12:25     ` [dpdk-dev] [PATCH v4 " Marcin Kerlin
@ 2016-06-16 12:57       ` Sergio Gonzalez Monroy
  2016-06-16 15:14       ` [dpdk-dev] [PATCH v5 " Marcin Kerlin
  1 sibling, 0 replies; 15+ messages in thread
From: Sergio Gonzalez Monroy @ 2016-06-16 12:57 UTC (permalink / raw)
  To: Marcin Kerlin, david.marchand; +Cc: dev

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>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [dpdk-dev] [PATCH v5 1/1] eal: fix resource leak of mapped memory
  2016-06-15 12:25     ` [dpdk-dev] [PATCH v4 " Marcin Kerlin
  2016-06-16 12:57       ` Sergio Gonzalez Monroy
@ 2016-06-16 15:14       ` Marcin Kerlin
  2016-06-20  9:30         ` Thomas Monjalon
  1 sibling, 1 reply; 15+ messages in thread
From: Marcin Kerlin @ 2016-06-16 15:14 UTC (permalink / raw)
  To: david.marchand; +Cc: sergio.gonzalez.monroy, dev, 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [dpdk-dev] [PATCH v5 1/1] eal: fix resource leak of mapped memory
  2016-06-16 15:14       ` [dpdk-dev] [PATCH v5 " Marcin Kerlin
@ 2016-06-20  9:30         ` Thomas Monjalon
  0 siblings, 0 replies; 15+ messages in thread
From: Thomas Monjalon @ 2016-06-20  9:30 UTC (permalink / raw)
  To: Marcin Kerlin; +Cc: dev, david.marchand, sergio.gonzalez.monroy

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2016-06-20  9:30 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-19 16:27 [dpdk-dev] [PATCH 1/1] lib/librte_eal: fix resource leak Marcin Kerlin
2016-04-20  9:15 ` David Marchand
2016-04-21 11:19   ` Sergio Gonzalez Monroy
2016-04-21 11:49     ` Kerlin, MarcinX
2016-04-22 10:42     ` Panu Matilainen
2016-06-14 15:33 ` [dpdk-dev] [PATCH v2 1/1] eal: fix resource leak of mapped memory Marcin Kerlin
2016-06-15  8:49   ` Sergio Gonzalez Monroy
2016-06-15  9:35     ` Kerlin, MarcinX
2016-06-15 10:05       ` Panu Matilainen
2016-06-15 11:13         ` Kerlin, MarcinX
2016-06-15 10:45   ` [dpdk-dev] [PATCH v3 " Marcin Kerlin
2016-06-15 12:25     ` [dpdk-dev] [PATCH v4 " Marcin Kerlin
2016-06-16 12:57       ` Sergio Gonzalez Monroy
2016-06-16 15:14       ` [dpdk-dev] [PATCH v5 " Marcin Kerlin
2016-06-20  9:30         ` Thomas Monjalon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).