Skip to contentSkip to conversation

mem: fix incorrect munmap in error unwind

PATCH Superseded Stephen Hemminger 4 participants 5 messages Jan 6, 2020 – Feb 6, 2020

lore
Share & tools
lore

Stephen Hemminger

author
#0 lore raw

PATCH 1/1 mem: fix incorrect munmap in error unwind

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 [email protected]
Cc [email protected]
Signed-off-by Stephen Hemminger <[email protected]>
---
 lib/librte_eal/linux/eal/eal_memory.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
lib/librte_eal/linux/eal/eal_memory.c +1 -2
index 43e4ffc757bd..cf5b2433614b 100644
@@ -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;
Signature · 1 line
2.20.1
#1 lore raw
Quoted 19 lines · 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 [email protected]
Cc [email protected]
Signed-off-by Stephen Hemminger <[email protected]>
---
 lib/librte_eal/linux/eal/eal_memory.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
lib/librte_eal/linux/eal/eal_memory.c +0 -0
index 43e4ffc757bd..cf5b2433614b 100644

lib/librte_eal/linux/eal/eal_memory.c eal_legacy_hugepage_attach(void) · line 1969

Quoted 3 lines
lib/librte_eal/linux/eal/eal_memory.c +0 -0
@@ -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.

lib/librte_eal/linux/eal/eal_memory.c eal_legacy_hugepage_attach(void) · line 1974

Quoted 7 lines
lib/librte_eal/linux/eal/eal_memory.c +1 -2
-       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;
Signature · 1 line
David Marchand
author
#3 lore raw

PATCH v2 1/1 mem: fix incorrect munmap in error unwind

In reply to 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 [email protected]
Cc [email protected]
Signed-off-by Stephen Hemminger <[email protected]>
Acked-by Anatoly Burakov <[email protected]>

---
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(-)
lib/librte_eal/linux/eal/eal_memory.c +9 -12
index 43e4ffc757bd..5604c2a7c04d 100644
@@ -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)
Signature · 1 line
2.20.1
Applied
#4 lore raw

22/01/2020 18:06, Stephen Hemminger:

Quoted 4 lines

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.

Quoted 8 lines
Fixes 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc [email protected]
Cc [email protected]
Signed-off-by Stephen Hemminger <[email protected]>
Acked-by Anatoly Burakov <[email protected]>

---
v2 - incorporate feedback from David Marchand
fix missing munmap of current segment

Applied, thanks