* Re: [dpdk-stable] [PATCH] mem: fix incorrect munmap in error unwind
2020-01-06 20:55 [dpdk-stable] [PATCH] mem: fix incorrect munmap in error unwind Stephen Hemminger
@ 2020-01-07 8:43 ` David Marchand
2020-01-07 16:15 ` Burakov, Anatoly
2020-01-22 17:06 ` [dpdk-stable] [PATCH v2] " Stephen Hemminger
2 siblings, 0 replies; 5+ messages in thread
From: David Marchand @ 2020-01-07 8:43 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: dev, Burakov, Anatoly, dpdk stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread* [dpdk-stable] [PATCH v2] mem: fix incorrect munmap in error unwind
2020-01-06 20:55 [dpdk-stable] [PATCH] mem: fix incorrect munmap in error unwind Stephen Hemminger
2020-01-07 8:43 ` David Marchand
2020-01-07 16:15 ` Burakov, Anatoly
@ 2020-01-22 17:06 ` Stephen Hemminger
2020-02-06 14:17 ` Thomas Monjalon
2 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2020-01-22 17:06 UTC (permalink / raw)
To: dev; +Cc: Stephen Hemminger, anatoly.burakov, stable
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
^ permalink raw reply [flat|nested] 5+ messages in thread