[dpdk-dev] [PATCH] mem: fix incorrect munmap in error unwind

[dpdk-dev] [PATCH] mem: fix incorrect munmap in error unwind

[Stephen Hemminger]

The loop to unwind existing mmaps was only unmapping the
first segment.

Also, remove obvious redundant assignment.

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 lib/librte_eal/linux/eal/eal_memory.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c
index 43e4ffc757bd..cf5b2433614b 100644
--- a/lib/librte_eal/linux/eal/eal_memory.c
+++ b/lib/librte_eal/linux/eal/eal_memory.c
@@ -1967,9 +1967,8 @@ eal_legacy_hugepage_attach(void)
 	close(fd);
 error:
 	/* map all segments into memory to make sure we get the addrs */
-	cur_seg = 0;
 	for (cur_seg = 0; cur_seg < i; cur_seg++) {
-		struct hugepage_file *hf = &hp[i];
+		struct hugepage_file *hf = &hp[cur_seg];
 		size_t map_sz = hf->size;
 		void *map_addr = hf->final_va;
 
-- 
2.20.1

Re: [dpdk-dev] [dpdk-stable] [PATCH] mem: fix incorrect munmap in error unwind

[David Marchand]

On Mon, Jan 6, 2020 at 9:56 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> The loop to unwind existing mmaps was only unmapping the
> first segment.
>
> Also, remove obvious redundant assignment.
>
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
>  lib/librte_eal/linux/eal/eal_memory.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c
> index 43e4ffc757bd..cf5b2433614b 100644
> --- a/lib/librte_eal/linux/eal/eal_memory.c
> +++ b/lib/librte_eal/linux/eal/eal_memory.c
> @@ -1967,9 +1967,8 @@ eal_legacy_hugepage_attach(void)
>         close(fd);
>  error:
>         /* map all segments into memory to make sure we get the addrs */

This looks like a copy of the same loop from a few lines before in the
same function.
The comment can be removed.


> -       cur_seg = 0;
>         for (cur_seg = 0; cur_seg < i; cur_seg++) {
> -               struct hugepage_file *hf = &hp[i];
> +               struct hugepage_file *hf = &hp[cur_seg];
>                 size_t map_sz = hf->size;
>                 void *map_addr = hf->final_va;
>

map_addr and map_sz are unnecessary.

We should be safe with dereferencing hp if i != 0, but still how about:

error:
        if (hp != NULL && hp != MAP_FAILED) {
                unsigned int cur_seg;

                for (cur_seg = 0; cur_seg < i; cur_seg++)
                        munmap(hp[cur_seg].final_va, hp[cur_seg].size);
                munmap(hp, size);
        }
        if (fd_hugepage >= 0)
                close(fd_hugepage);
        return -1;


-- 
David Marchand

Re: [dpdk-dev] [PATCH] mem: fix incorrect munmap in error unwind

[Burakov, Anatoly]

On 06-Jan-20 8:55 PM, Stephen Hemminger wrote:
> The loop to unwind existing mmaps was only unmapping the
> first segment.
> 
> Also, remove obvious redundant assignment.
> 
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---

Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>

-- 
Thanks,
Anatoly

[dpdk-dev] [PATCH v2] mem: fix incorrect munmap in error unwind

[Stephen Hemminger]

The loop to unwind existing mmaps was only unmapping the
first segment and the error paths after mmap() were not
doing munmap of the current segment.

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com
Cc: stable@dpdk.org
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
v2 - incorporate feedback from David Marchand
     fix missing munmap of current segment

 lib/librte_eal/linux/eal/eal_memory.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/lib/librte_eal/linux/eal/eal_memory.c b/lib/librte_eal/linux/eal/eal_memory.c
index 43e4ffc757bd..5604c2a7c04d 100644
--- a/lib/librte_eal/linux/eal/eal_memory.c
+++ b/lib/librte_eal/linux/eal/eal_memory.c
@@ -1928,7 +1928,7 @@ eal_legacy_hugepage_attach(void)
 		if (flock(fd, LOCK_SH) < 0) {
 			RTE_LOG(DEBUG, EAL, "%s(): Locking file failed: %s\n",
 				__func__, strerror(errno));
-			goto fd_error;
+			goto mmap_error;
 		}
 
 		/* find segment data */
@@ -1936,13 +1936,13 @@ eal_legacy_hugepage_attach(void)
 		if (msl == NULL) {
 			RTE_LOG(DEBUG, EAL, "%s(): Cannot find memseg list\n",
 				__func__);
-			goto fd_error;
+			goto mmap_error;
 		}
 		ms = rte_mem_virt2memseg(map_addr, msl);
 		if (ms == NULL) {
 			RTE_LOG(DEBUG, EAL, "%s(): Cannot find memseg\n",
 				__func__);
-			goto fd_error;
+			goto mmap_error;
 		}
 
 		msl_idx = msl - mcfg->memsegs;
@@ -1950,7 +1950,7 @@ eal_legacy_hugepage_attach(void)
 		if (ms_idx < 0) {
 			RTE_LOG(DEBUG, EAL, "%s(): Cannot find memseg idx\n",
 				__func__);
-			goto fd_error;
+			goto mmap_error;
 		}
 
 		/* store segment fd internally */
@@ -1963,18 +1963,15 @@ eal_legacy_hugepage_attach(void)
 	close(fd_hugepage);
 	return 0;
 
+mmap_error:
+	munmap(hp[i].final_va, hp[i].size);
 fd_error:
 	close(fd);
 error:
-	/* map all segments into memory to make sure we get the addrs */
-	cur_seg = 0;
-	for (cur_seg = 0; cur_seg < i; cur_seg++) {
-		struct hugepage_file *hf = &hp[i];
-		size_t map_sz = hf->size;
-		void *map_addr = hf->final_va;
+	/* unwind mmap's done so far */
+	for (cur_seg = 0; cur_seg < i; cur_seg++)
+		munmap(hp[cur_seg].final_va, hp[cur_seg].size);
 
-		munmap(map_addr, map_sz);
-	}
 	if (hp != NULL && hp != MAP_FAILED)
 		munmap(hp, size);
 	if (fd_hugepage >= 0)
-- 
2.20.1

Re: [dpdk-dev] [dpdk-stable] [PATCH v2] mem: fix incorrect munmap in error unwind

[Thomas Monjalon]

22/01/2020 18:06, Stephen Hemminger:
> The loop to unwind existing mmaps was only unmapping the
> first segment and the error paths after mmap() were not
> doing munmap of the current segment.
> 
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> Cc: stable@dpdk.org
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> Acked-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> v2 - incorporate feedback from David Marchand
>      fix missing munmap of current segment

Applied, thanks