[PATCH 00/10] Coverity fixes for EAL memory

[PATCH 00/10] Coverity fixes for EAL memory

[Anatoly Burakov]

This patchset fixes a host of coverity issues in memory subsystem
introduced with recent DPDK memory hotplug patchset.

Coverity issues fixed:
- 272601 - leaking fd
- 272560 - double close fd
- 272568 - leaking fd
- 272570 - leaking fd
- 272589 - dereference before null check
- 272602 - freeing wrong pointer
- 272608 - expression does nothing
- 272577 - negative return not handled
- 272578 - negative return not handled
- 272584 - use after free

Additionally, also replace all instances of snprintf with strlcpy.

Anatoly Burakov (10):
  mem: use strlcpy instead of snprintf
  mem: fix resource leak
  mem: fix potential double close
  mem: fix potential resource leak
  mem: fix potential resource leak
  mem: fix comparing pointer to value
  mem: fix potential bad unmap
  mem: fix statement having no effect
  mem: fix negative return value
  mem: fix possible use-after-free

 lib/librte_eal/bsdapp/eal/eal_hugepage_info.c |  2 +-
 lib/librte_eal/common/eal_common_memalloc.c   |  5 +++--
 lib/librte_eal/common/eal_common_memory.c     | 23 +++++++++++++++++------
 lib/librte_eal/linuxapp/eal/eal_memalloc.c    | 22 ++++++++++++++++------
 lib/librte_eal/linuxapp/eal/eal_memory.c      |  5 +++--
 5 files changed, 40 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH 01/10] mem: use strlcpy instead of snprintf

[Anatoly Burakov]

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

Notes:
    One other instance of using snprintf is left alone as
    it is expected to be addressed by another patch [1].
    
    [1] http://dpdk.org/dev/patchwork/patch/38301/

 lib/librte_eal/bsdapp/eal/eal_hugepage_info.c | 2 +-
 lib/librte_eal/common/eal_common_memalloc.c   | 5 +++--
 lib/librte_eal/linuxapp/eal/eal_memory.c      | 4 ++--
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/librte_eal/bsdapp/eal/eal_hugepage_info.c b/lib/librte_eal/bsdapp/eal/eal_hugepage_info.c
index 38d143c..836feb6 100644
--- a/lib/librte_eal/bsdapp/eal/eal_hugepage_info.c
+++ b/lib/librte_eal/bsdapp/eal/eal_hugepage_info.c
@@ -96,7 +96,7 @@ eal_hugepage_info_init(void)
 		RTE_LOG(INFO, EAL, "Contigmem driver has %d buffers, each of size %dKB\n",
 				num_buffers, (int)(buffer_size>>10));
 
-	snprintf(hpi->hugedir, sizeof(hpi->hugedir), "%s", CONTIGMEM_DEV);
+	strlcpy(hpi->hugedir, CONTIGMEM_DEV, sizeof(hpi->hugedir));
 	hpi->hugepage_sz = buffer_size;
 	hpi->num_pages[0] = num_buffers;
 	hpi->lock_descriptor = fd;
diff --git a/lib/librte_eal/common/eal_common_memalloc.c b/lib/librte_eal/common/eal_common_memalloc.c
index 49fd53c..e983688 100644
--- a/lib/librte_eal/common/eal_common_memalloc.c
+++ b/lib/librte_eal/common/eal_common_memalloc.c
@@ -10,6 +10,7 @@
 #include <rte_memzone.h>
 #include <rte_memory.h>
 #include <rte_eal_memconfig.h>
+#include <rte_string_fns.h>
 #include <rte_rwlock.h>
 
 #include "eal_private.h"
@@ -179,7 +180,7 @@ eal_memalloc_mem_event_callback_register(const char *name,
 
 	/* callback successfully created and is valid, add it to the list */
 	entry->clb = clb;
-	snprintf(entry->name, RTE_MEM_EVENT_CALLBACK_NAME_LEN, "%s", name);
+	strlcpy(entry->name, name, RTE_MEM_EVENT_CALLBACK_NAME_LEN);
 	TAILQ_INSERT_TAIL(&mem_event_callback_list, entry, next);
 
 	ret = 0;
@@ -284,7 +285,7 @@ eal_memalloc_mem_alloc_validator_register(const char *name,
 	entry->clb = clb;
 	entry->socket_id = socket_id;
 	entry->limit = limit;
-	snprintf(entry->name, RTE_MEM_ALLOC_VALIDATOR_NAME_LEN, "%s", name);
+	strlcpy(entry->name, name, RTE_MEM_ALLOC_VALIDATOR_NAME_LEN);
 	TAILQ_INSERT_TAIL(&mem_alloc_validator_list, entry, next);
 
 	ret = 0;
diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index b7a2e95..391b0de 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1160,8 +1160,8 @@ calc_num_pages_per_socket(uint64_t * memory,
 	for (socket = 0; socket < RTE_MAX_NUMA_NODES && total_mem != 0; socket++) {
 		/* skips if the memory on specific socket wasn't requested */
 		for (i = 0; i < num_hp_info && memory[socket] != 0; i++){
-			snprintf(hp_used[i].hugedir, sizeof(hp_used[i].hugedir),
-					"%s", hp_info[i].hugedir);
+			strlcpy(hp_used[i].hugedir, hp_info[i].hugedir,
+					sizeof(hp_used[i].hugedir));
 			hp_used[i].num_pages[socket] = RTE_MIN(
 					memory[socket] / hp_info[i].hugepage_sz,
 					hp_info[i].num_pages[socket]);
-- 
2.7.4

[PATCH 02/10] mem: fix resource leak

[Anatoly Burakov]

Coverity issue: 272601

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index 391b0de..419736d 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1750,6 +1750,7 @@ eal_legacy_hugepage_attach(void)
 		if (map_addr == MAP_FAILED) {
 			RTE_LOG(ERR, EAL, "Could not map %s: %s\n",
 				hf->filepath, strerror(errno));
+			close(fd);
 			goto error;
 		}
 
-- 
2.7.4

[PATCH 03/10] mem: fix potential double close

[Anatoly Burakov]

We were closing descriptor before checking if mapping has
failed, but if it did, we did a second close afterwards. Fix
it by moving closing descriptor to after we check if mmap has
succeeded.

Coverity issue: 272560

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 1f553dd..9156f8b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -458,9 +458,6 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	 */
 	void *va = mmap(addr, alloc_sz, PROT_READ | PROT_WRITE,
 			MAP_SHARED | MAP_POPULATE | MAP_FIXED, fd, map_offset);
-	/* for non-single file segments, we can close fd here */
-	if (!internal_config.single_file_segments)
-		close(fd);
 
 	if (va == MAP_FAILED) {
 		RTE_LOG(DEBUG, EAL, "%s(): mmap() failed: %s\n", __func__,
@@ -471,6 +468,9 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 		RTE_LOG(DEBUG, EAL, "%s(): wrong mmap() address\n", __func__);
 		goto mapped;
 	}
+	/* for non-single file segments, we can close fd here */
+	if (!internal_config.single_file_segments)
+		close(fd);
 
 	rte_iova_t iova = rte_mem_virt2iova(addr);
 	if (iova == RTE_BAD_PHYS_ADDR) {
-- 
2.7.4

[PATCH 04/10] mem: fix potential resource leak

[Anatoly Burakov]

We close fd if we managed to find it in the list of allocated
segment lists (which should always be the case under normal
conditions), but if we didn't, the fd was leaking. Close it if
we couldn't find it in the segment list. This is not an issue
as if the segment is zero length, we're getting rid of it
anyway, so there's no harm in not storing the fd anywhere.

Coverity issue: 272568

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 9156f8b..fab5a98 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -569,6 +569,8 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi,
 			if (te != NULL && te->fd >= 0) {
 				close(te->fd);
 				te->fd = -1;
+			} else {
+				close(fd);
 			}
 			unlink(path);
 		}
-- 
2.7.4

[PATCH 05/10] mem: fix potential resource leak

[Anatoly Burakov]

Normally, tailq entry should have a valid fd by the time we attempt
to map the segment. However, in case it doesn't, we're leaking fd,
so fix it.

Coverity issue: 272570

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index fab5a98..b02e3a5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 			if (te != NULL && te->fd >= 0) {
 				close(te->fd);
 				te->fd = -1;
+			} else {
+				close(fd);
 			}
 			/* ignore errors, can't make it any worse */
 			unlink(path);
-- 
2.7.4

[PATCH 06/10] mem: fix comparing pointer to value

[Anatoly Burakov]

Previous code had an old rebase leftover from the time when
oldpolicy was an actual int, instead of a pointer. Fix it to
do comparison with dereferencing the pointer.

Coverity issue: 272589

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index b02e3a5..8420a26 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -146,7 +146,7 @@ resotre_numa(int *oldpolicy, struct bitmask *oldmask)
 {
 	RTE_LOG(DEBUG, EAL,
 		"Restoring previous memory policy: %d\n", *oldpolicy);
-	if (oldpolicy == MPOL_DEFAULT) {
+	if (*oldpolicy == MPOL_DEFAULT) {
 		numa_set_localalloc();
 	} else if (set_mempolicy(*oldpolicy, oldmask->maskp,
 				 oldmask->size + 1) < 0) {
-- 
2.7.4

[PATCH 07/10] mem: fix potential bad unmap

[Anatoly Burakov]

Previously, if mmap failed to map page address at requested
address, we were attempting to unmap the wrong address. Fix it
by unmapping our actual mapped address, and jump further to
avoid unmapping memory that is not allocated.

Coverity issue: 272602

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 8420a26..6a75e5b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -466,7 +466,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	}
 	if (va != addr) {
 		RTE_LOG(DEBUG, EAL, "%s(): wrong mmap() address\n", __func__);
-		goto mapped;
+		munmap(va, alloc_sz);
+		goto resized;
 	}
 	/* for non-single file segments, we can close fd here */
 	if (!internal_config.single_file_segments)
-- 
2.7.4

[PATCH 08/10] mem: fix statement having no effect

[Anatoly Burakov]

Coverity reports these lines as having no effect. Technically, we do
want for those lines to have no effect, however they would've likely
been optimized out. Add volatile qualifiers to ensure the code has
effects.

Coverity issue: 272608

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 6a75e5b..655c69e 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -503,7 +503,12 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 			(unsigned int)(alloc_sz >> 20));
 		goto mapped;
 	}
-	*(int *)addr = *(int *)addr;
+	/* we need to trigger a write to the page to enforce page fault and
+	 * ensure that page is accessible to us, but we can't overwrite value
+	 * that is already there, so read the old value, and write itback.
+	 * kernel populates the page with zeroes initially.
+	 */
+	*(volatile int *)addr = *(volatile int *)addr;
 
 	ms->addr = addr;
 	ms->hugepage_sz = alloc_sz;
-- 
2.7.4

[PATCH 09/10] mem: fix negative return value

[Anatoly Burakov]

Although unlikely during normal operation, rte_socket_id_by_idx()
may return a negative value, which would've caused an out-of-bounds
read. Fix it by making socket ID signed, and check for negative
return.

Coverity issue: 272577
Coverity issue: 272578

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_memory.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 24a9ed5..68fc70e 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -205,7 +205,8 @@ memseg_primary_init_32(void)
 {
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	int active_sockets, hpi_idx, msl_idx = 0;
-	unsigned int socket_id, i;
+	unsigned int i;
+	int socket_id;
 	struct rte_memseg_list *msl;
 	uint64_t extra_mem_per_socket, total_extra_mem, total_requested_mem;
 	uint64_t max_mem;
@@ -238,6 +239,11 @@ memseg_primary_init_32(void)
 			uint64_t mem;
 
 			socket_id = rte_socket_id_by_idx(i);
+			if (socket_id < 0) {
+				RTE_LOG(ERR, EAL, "Invalid socket index: %u\n",
+						i);
+				continue;
+			}
 			mem = internal_config.socket_mem[socket_id];
 
 			if (mem == 0)
@@ -281,6 +287,10 @@ memseg_primary_init_32(void)
 		bool skip;
 
 		socket_id = rte_socket_id_by_idx(i);
+		if (socket_id < 0) {
+			RTE_LOG(ERR, EAL, "Invalid socket index: %u\n", i);
+			continue;
+		}
 
 #ifndef RTE_EAL_NUMA_AWARE_HUGEPAGES
 		if (socket_id > 0)
@@ -294,10 +304,11 @@ memseg_primary_init_32(void)
 		 * socket, and this is not master lcore
 		 */
 		master_lcore_socket = rte_lcore_to_socket_id(cfg->master_lcore);
-		skip |= active_sockets == 0 && socket_id != master_lcore_socket;
+		skip |= active_sockets == 0 &&
+				(unsigned int)socket_id != master_lcore_socket;
 
 		if (skip) {
-			RTE_LOG(DEBUG, EAL, "Will not preallocate memory on socket %u\n",
+			RTE_LOG(DEBUG, EAL, "Will not preallocate memory on socket %i\n",
 					socket_id);
 			continue;
 		}
-- 
2.7.4

[PATCH 10/10] mem: fix possible use-after-free

[Anatoly Burakov]

If user has specified a flag to unmap the area right after mapping it,
we were passing an already-unmapped pointer to RTE_LOG. This is not an
issue since RTE_LOG doesn't actually dereference the pointer, but fix
it anyway by moving call to RTE_LOG to before unmap.

Coverity issue: 272584

Fixes: b7cc54187ea4 ("mem: move virtual area function in common directory")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 68fc70e..e979eba 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -113,12 +113,12 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
 		RTE_LOG(WARNING, EAL, "   This may cause issues with mapping memory into secondary processes\n");
 	}
 
-	if (unmap)
-		munmap(mapped_addr, map_sz);
-
 	RTE_LOG(DEBUG, EAL, "Virtual area found at %p (size = 0x%zx)\n",
 		aligned_addr, *size);
 
+	if (unmap)
+		munmap(mapped_addr, map_sz);
+
 	baseaddr_offset += *size;
 
 	return aligned_addr;
-- 
2.7.4

Re: [PATCH 00/10] Coverity fixes for EAL memory

[Thomas Monjalon]

17/04/2018 17:50, Anatoly Burakov:
> This patchset fixes a host of coverity issues in memory subsystem
> introduced with recent DPDK memory hotplug patchset.
> 
> Coverity issues fixed:
> - 272601 - leaking fd
> - 272560 - double close fd
> - 272568 - leaking fd
> - 272570 - leaking fd
> - 272589 - dereference before null check
> - 272602 - freeing wrong pointer
> - 272608 - expression does nothing
> - 272577 - negative return not handled
> - 272578 - negative return not handled
> - 272584 - use after free

Looks like you should buy a Coverity license
to run it locally before submitting ;)

Re: [PATCH 00/10] Coverity fixes for EAL memory

[Burakov, Anatoly]

On 17-Apr-18 4:56 PM, Thomas Monjalon wrote:
> 17/04/2018 17:50, Anatoly Burakov:
>> This patchset fixes a host of coverity issues in memory subsystem
>> introduced with recent DPDK memory hotplug patchset.
>>
>> Coverity issues fixed:
>> - 272601 - leaking fd
>> - 272560 - double close fd
>> - 272568 - leaking fd
>> - 272570 - leaking fd
>> - 272589 - dereference before null check
>> - 272602 - freeing wrong pointer
>> - 272608 - expression does nothing
>> - 272577 - negative return not handled
>> - 272578 - negative return not handled
>> - 272584 - use after free
> 
> Looks like you should buy a Coverity license
> to run it locally before submitting ;)
> 

Well, in fairness, most of these issues (and similar issues submitted in 
another patchset) will not ever manifest themselves under normal 
circumstances, so it's not as bad as it perhaps might present itself.

-- 
Thanks,
Anatoly

Re: [PATCH 00/10] Coverity fixes for EAL memory

[Thomas Monjalon]

17/04/2018 18:09, Burakov, Anatoly:
> On 17-Apr-18 4:56 PM, Thomas Monjalon wrote:
> > 17/04/2018 17:50, Anatoly Burakov:
> >> This patchset fixes a host of coverity issues in memory subsystem
> >> introduced with recent DPDK memory hotplug patchset.
> >>
> >> Coverity issues fixed:
> >> - 272601 - leaking fd
> >> - 272560 - double close fd
> >> - 272568 - leaking fd
> >> - 272570 - leaking fd
> >> - 272589 - dereference before null check
> >> - 272602 - freeing wrong pointer
> >> - 272608 - expression does nothing
> >> - 272577 - negative return not handled
> >> - 272578 - negative return not handled
> >> - 272584 - use after free
> > 
> > Looks like you should buy a Coverity license
> > to run it locally before submitting ;)
> 
> Well, in fairness, most of these issues (and similar issues submitted in 
> another patchset) will not ever manifest themselves under normal 
> circumstances, so it's not as bad as it perhaps might present itself.

Just kidding :)
Thanks for the reactivity.

[PATCH v2 00/10] Coverity fixes for EAL memory

[Anatoly Burakov]

This patchset fixes a host of coverity issues in memory subsystem
introduced with recent DPDK memory hotplug patchset.

Coverity issues fixed:
- 272601 - leaking fd
- 272560 - double close fd
- 272568 - leaking fd
- 272570 - leaking fd
- 272589 - dereference before null check
- 272602 - freeing wrong pointer
- 272608 - expression does nothing
- 272577 - negative return not handled
- 272578 - negative return not handled
- 272584 - use after free

Additionally, also replace all instances of snprintf with strlcpy.

v2:
- Rebase on top of latest master

Anatoly Burakov (10):
  mem: use strlcpy instead of snprintf
  mem: fix resource leak
  mem: fix potential double close
  mem: fix potential resource leak
  mem: fix potential resource leak
  mem: fix comparing pointer to value
  mem: fix potential bad unmap
  mem: fix statement having no effect
  mem: fix negative return value
  mem: fix possible use-after-free

 lib/librte_eal/common/eal_common_memalloc.c |  5 +++--
 lib/librte_eal/common/eal_common_memory.c   | 23 +++++++++++++++++------
 lib/librte_eal/linuxapp/eal/eal_memalloc.c  | 22 ++++++++++++++++------
 lib/librte_eal/linuxapp/eal/eal_memory.c    |  1 +
 4 files changed, 37 insertions(+), 14 deletions(-)

-- 
2.7.4

[PATCH v2 01/10] mem: use strlcpy instead of snprintf

[Anatoly Burakov]

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

Notes:
    One other instance of using snprintf is left alone as
    it is expected to be addressed by another patch [1].
    
    [1] http://dpdk.org/dev/patchwork/patch/38301/

 lib/librte_eal/common/eal_common_memalloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memalloc.c b/lib/librte_eal/common/eal_common_memalloc.c
index 49fd53c..e983688 100644
--- a/lib/librte_eal/common/eal_common_memalloc.c
+++ b/lib/librte_eal/common/eal_common_memalloc.c
@@ -10,6 +10,7 @@
 #include <rte_memzone.h>
 #include <rte_memory.h>
 #include <rte_eal_memconfig.h>
+#include <rte_string_fns.h>
 #include <rte_rwlock.h>
 
 #include "eal_private.h"
@@ -179,7 +180,7 @@ eal_memalloc_mem_event_callback_register(const char *name,
 
 	/* callback successfully created and is valid, add it to the list */
 	entry->clb = clb;
-	snprintf(entry->name, RTE_MEM_EVENT_CALLBACK_NAME_LEN, "%s", name);
+	strlcpy(entry->name, name, RTE_MEM_EVENT_CALLBACK_NAME_LEN);
 	TAILQ_INSERT_TAIL(&mem_event_callback_list, entry, next);
 
 	ret = 0;
@@ -284,7 +285,7 @@ eal_memalloc_mem_alloc_validator_register(const char *name,
 	entry->clb = clb;
 	entry->socket_id = socket_id;
 	entry->limit = limit;
-	snprintf(entry->name, RTE_MEM_ALLOC_VALIDATOR_NAME_LEN, "%s", name);
+	strlcpy(entry->name, name, RTE_MEM_ALLOC_VALIDATOR_NAME_LEN);
 	TAILQ_INSERT_TAIL(&mem_alloc_validator_list, entry, next);
 
 	ret = 0;
-- 
2.7.4

[PATCH v2 02/10] mem: fix resource leak

[Anatoly Burakov]

Coverity issue: 272601

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index fadc1de..9351e84 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1750,6 +1750,7 @@ eal_legacy_hugepage_attach(void)
 		if (map_addr == MAP_FAILED) {
 			RTE_LOG(ERR, EAL, "Could not map %s: %s\n",
 				hf->filepath, strerror(errno));
+			close(fd);
 			goto error;
 		}
 
-- 
2.7.4

[PATCH v2 03/10] mem: fix potential double close

[Anatoly Burakov]

We were closing descriptor before checking if mapping has
failed, but if it did, we did a second close afterwards. Fix
it by moving closing descriptor to after we check if mmap has
succeeded.

Coverity issue: 272560

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 1f553dd..9156f8b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -458,9 +458,6 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	 */
 	void *va = mmap(addr, alloc_sz, PROT_READ | PROT_WRITE,
 			MAP_SHARED | MAP_POPULATE | MAP_FIXED, fd, map_offset);
-	/* for non-single file segments, we can close fd here */
-	if (!internal_config.single_file_segments)
-		close(fd);
 
 	if (va == MAP_FAILED) {
 		RTE_LOG(DEBUG, EAL, "%s(): mmap() failed: %s\n", __func__,
@@ -471,6 +468,9 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 		RTE_LOG(DEBUG, EAL, "%s(): wrong mmap() address\n", __func__);
 		goto mapped;
 	}
+	/* for non-single file segments, we can close fd here */
+	if (!internal_config.single_file_segments)
+		close(fd);
 
 	rte_iova_t iova = rte_mem_virt2iova(addr);
 	if (iova == RTE_BAD_PHYS_ADDR) {
-- 
2.7.4

[PATCH v2 04/10] mem: fix potential resource leak

[Anatoly Burakov]

We close fd if we managed to find it in the list of allocated
segment lists (which should always be the case under normal
conditions), but if we didn't, the fd was leaking. Close it if
we couldn't find it in the segment list. This is not an issue
as if the segment is zero length, we're getting rid of it
anyway, so there's no harm in not storing the fd anywhere.

Coverity issue: 272568

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 9156f8b..fab5a98 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -569,6 +569,8 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi,
 			if (te != NULL && te->fd >= 0) {
 				close(te->fd);
 				te->fd = -1;
+			} else {
+				close(fd);
 			}
 			unlink(path);
 		}
-- 
2.7.4

[PATCH v2 05/10] mem: fix potential resource leak

[Anatoly Burakov]

Normally, tailq entry should have a valid fd by the time we attempt
to map the segment. However, in case it doesn't, we're leaking fd,
so fix it.

Coverity issue: 272570

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index fab5a98..b02e3a5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 			if (te != NULL && te->fd >= 0) {
 				close(te->fd);
 				te->fd = -1;
+			} else {
+				close(fd);
 			}
 			/* ignore errors, can't make it any worse */
 			unlink(path);
-- 
2.7.4

[PATCH v2 06/10] mem: fix comparing pointer to value

[Anatoly Burakov]

Previous code had an old rebase leftover from the time when
oldpolicy was an actual int, instead of a pointer. Fix it to
do comparison with dereferencing the pointer.

Coverity issue: 272589

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index b02e3a5..8420a26 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -146,7 +146,7 @@ resotre_numa(int *oldpolicy, struct bitmask *oldmask)
 {
 	RTE_LOG(DEBUG, EAL,
 		"Restoring previous memory policy: %d\n", *oldpolicy);
-	if (oldpolicy == MPOL_DEFAULT) {
+	if (*oldpolicy == MPOL_DEFAULT) {
 		numa_set_localalloc();
 	} else if (set_mempolicy(*oldpolicy, oldmask->maskp,
 				 oldmask->size + 1) < 0) {
-- 
2.7.4

[PATCH v2 07/10] mem: fix potential bad unmap

[Anatoly Burakov]

Previously, if mmap failed to map page address at requested
address, we were attempting to unmap the wrong address. Fix it
by unmapping our actual mapped address, and jump further to
avoid unmapping memory that is not allocated.

Coverity issue: 272602

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 8420a26..6a75e5b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -466,7 +466,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	}
 	if (va != addr) {
 		RTE_LOG(DEBUG, EAL, "%s(): wrong mmap() address\n", __func__);
-		goto mapped;
+		munmap(va, alloc_sz);
+		goto resized;
 	}
 	/* for non-single file segments, we can close fd here */
 	if (!internal_config.single_file_segments)
-- 
2.7.4

[PATCH v2 08/10] mem: fix statement having no effect

[Anatoly Burakov]

Coverity reports these lines as having no effect. Technically, we do
want for those lines to have no effect, however they would've likely
been optimized out. Add volatile qualifiers to ensure the code has
effects.

Coverity issue: 272608

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 6a75e5b..655c69e 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -503,7 +503,12 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 			(unsigned int)(alloc_sz >> 20));
 		goto mapped;
 	}
-	*(int *)addr = *(int *)addr;
+	/* we need to trigger a write to the page to enforce page fault and
+	 * ensure that page is accessible to us, but we can't overwrite value
+	 * that is already there, so read the old value, and write itback.
+	 * kernel populates the page with zeroes initially.
+	 */
+	*(volatile int *)addr = *(volatile int *)addr;
 
 	ms->addr = addr;
 	ms->hugepage_sz = alloc_sz;
-- 
2.7.4

[PATCH v2 09/10] mem: fix negative return value

[Anatoly Burakov]

Although unlikely during normal operation, rte_socket_id_by_idx()
may return a negative value, which would've caused an out-of-bounds
read. Fix it by making socket ID signed, and check for negative
return.

Coverity issue: 272577
Coverity issue: 272578

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_memory.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 24a9ed5..68fc70e 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -205,7 +205,8 @@ memseg_primary_init_32(void)
 {
 	struct rte_mem_config *mcfg = rte_eal_get_configuration()->mem_config;
 	int active_sockets, hpi_idx, msl_idx = 0;
-	unsigned int socket_id, i;
+	unsigned int i;
+	int socket_id;
 	struct rte_memseg_list *msl;
 	uint64_t extra_mem_per_socket, total_extra_mem, total_requested_mem;
 	uint64_t max_mem;
@@ -238,6 +239,11 @@ memseg_primary_init_32(void)
 			uint64_t mem;
 
 			socket_id = rte_socket_id_by_idx(i);
+			if (socket_id < 0) {
+				RTE_LOG(ERR, EAL, "Invalid socket index: %u\n",
+						i);
+				continue;
+			}
 			mem = internal_config.socket_mem[socket_id];
 
 			if (mem == 0)
@@ -281,6 +287,10 @@ memseg_primary_init_32(void)
 		bool skip;
 
 		socket_id = rte_socket_id_by_idx(i);
+		if (socket_id < 0) {
+			RTE_LOG(ERR, EAL, "Invalid socket index: %u\n", i);
+			continue;
+		}
 
 #ifndef RTE_EAL_NUMA_AWARE_HUGEPAGES
 		if (socket_id > 0)
@@ -294,10 +304,11 @@ memseg_primary_init_32(void)
 		 * socket, and this is not master lcore
 		 */
 		master_lcore_socket = rte_lcore_to_socket_id(cfg->master_lcore);
-		skip |= active_sockets == 0 && socket_id != master_lcore_socket;
+		skip |= active_sockets == 0 &&
+				(unsigned int)socket_id != master_lcore_socket;
 
 		if (skip) {
-			RTE_LOG(DEBUG, EAL, "Will not preallocate memory on socket %u\n",
+			RTE_LOG(DEBUG, EAL, "Will not preallocate memory on socket %i\n",
 					socket_id);
 			continue;
 		}
-- 
2.7.4

[PATCH v2 10/10] mem: fix possible use-after-free

[Anatoly Burakov]

If user has specified a flag to unmap the area right after mapping it,
we were passing an already-unmapped pointer to RTE_LOG. This is not an
issue since RTE_LOG doesn't actually dereference the pointer, but fix
it anyway by moving call to RTE_LOG to before unmap.

Coverity issue: 272584

Fixes: b7cc54187ea4 ("mem: move virtual area function in common directory")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 68fc70e..e979eba 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -113,12 +113,12 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
 		RTE_LOG(WARNING, EAL, "   This may cause issues with mapping memory into secondary processes\n");
 	}
 
-	if (unmap)
-		munmap(mapped_addr, map_sz);
-
 	RTE_LOG(DEBUG, EAL, "Virtual area found at %p (size = 0x%zx)\n",
 		aligned_addr, *size);
 
+	if (unmap)
+		munmap(mapped_addr, map_sz);
+
 	baseaddr_offset += *size;
 
 	return aligned_addr;
-- 
2.7.4

[PATCH v3 0/9] Coverity fixes for EAL memory

[Anatoly Burakov]

This patchset fixes a host of coverity issues in memory subsystem
introduced with recent DPDK memory hotplug patchset.

Coverity issues fixed:
- 272601 - leaking fd
- 272560 - double close fd
- 272568 - leaking fd
- 272570 - leaking fd
- 272589 - dereference before null check
- 272602 - freeing wrong pointer
- 272608 - expression does nothing
- 272584 - use after free

Coverity issues not fixed:
- 272577 - negative return not handled
- 272578 - negative return not handled
  - Proper usage of API guarantees no negative returns

Additionally, also replace all instances of snprintf with strlcpy.

v3:
- Drop fixes for 272577 and 272578 and mark them as false positives

v2:
- Rebase on top of latest master

Anatoly Burakov (9):
  mem: use strlcpy instead of snprintf
  mem: fix resource leak
  mem: fix potential double close
  mem: fix potential resource leak
  mem: fix potential resource leak
  mem: fix comparing pointer to value
  mem: fix potential bad unmap
  mem: fix statement having no effect
  mem: fix possible use-after-free

 lib/librte_eal/common/eal_common_memalloc.c |  5 +++--
 lib/librte_eal/common/eal_common_memory.c   |  6 +++---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c  | 22 ++++++++++++++++------
 lib/librte_eal/linuxapp/eal/eal_memory.c    |  1 +
 4 files changed, 23 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH v3 1/9] mem: use strlcpy instead of snprintf

[Anatoly Burakov]

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

Notes:
    One other instance of using snprintf is left alone as
    it is expected to be addressed by another patch [1].
    
    [1] http://dpdk.org/dev/patchwork/patch/38301/

 lib/librte_eal/common/eal_common_memalloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memalloc.c b/lib/librte_eal/common/eal_common_memalloc.c
index 49fd53c..e983688 100644
--- a/lib/librte_eal/common/eal_common_memalloc.c
+++ b/lib/librte_eal/common/eal_common_memalloc.c
@@ -10,6 +10,7 @@
 #include <rte_memzone.h>
 #include <rte_memory.h>
 #include <rte_eal_memconfig.h>
+#include <rte_string_fns.h>
 #include <rte_rwlock.h>
 
 #include "eal_private.h"
@@ -179,7 +180,7 @@ eal_memalloc_mem_event_callback_register(const char *name,
 
 	/* callback successfully created and is valid, add it to the list */
 	entry->clb = clb;
-	snprintf(entry->name, RTE_MEM_EVENT_CALLBACK_NAME_LEN, "%s", name);
+	strlcpy(entry->name, name, RTE_MEM_EVENT_CALLBACK_NAME_LEN);
 	TAILQ_INSERT_TAIL(&mem_event_callback_list, entry, next);
 
 	ret = 0;
@@ -284,7 +285,7 @@ eal_memalloc_mem_alloc_validator_register(const char *name,
 	entry->clb = clb;
 	entry->socket_id = socket_id;
 	entry->limit = limit;
-	snprintf(entry->name, RTE_MEM_ALLOC_VALIDATOR_NAME_LEN, "%s", name);
+	strlcpy(entry->name, name, RTE_MEM_ALLOC_VALIDATOR_NAME_LEN);
 	TAILQ_INSERT_TAIL(&mem_alloc_validator_list, entry, next);
 
 	ret = 0;
-- 
2.7.4

Re: [PATCH v3 1/9] mem: use strlcpy instead of snprintf

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:39AM +0100, Anatoly Burakov wrote:
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 2/9] mem: fix resource leak

[Anatoly Burakov]

Coverity issue: 272601

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index fadc1de..9351e84 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1750,6 +1750,7 @@ eal_legacy_hugepage_attach(void)
 		if (map_addr == MAP_FAILED) {
 			RTE_LOG(ERR, EAL, "Could not map %s: %s\n",
 				hf->filepath, strerror(errno));
+			close(fd);
 			goto error;
 		}
 
-- 
2.7.4

Re: [PATCH v3 2/9] mem: fix resource leak

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:40AM +0100, Anatoly Burakov wrote:
> Coverity issue: 272601
> 
> Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 3/9] mem: fix potential double close

[Anatoly Burakov]

We were closing descriptor before checking if mapping has
failed, but if it did, we did a second close afterwards. Fix
it by moving closing descriptor to after we check if mmap has
succeeded.

Coverity issue: 272560

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 1f553dd..9156f8b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -458,9 +458,6 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	 */
 	void *va = mmap(addr, alloc_sz, PROT_READ | PROT_WRITE,
 			MAP_SHARED | MAP_POPULATE | MAP_FIXED, fd, map_offset);
-	/* for non-single file segments, we can close fd here */
-	if (!internal_config.single_file_segments)
-		close(fd);
 
 	if (va == MAP_FAILED) {
 		RTE_LOG(DEBUG, EAL, "%s(): mmap() failed: %s\n", __func__,
@@ -471,6 +468,9 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 		RTE_LOG(DEBUG, EAL, "%s(): wrong mmap() address\n", __func__);
 		goto mapped;
 	}
+	/* for non-single file segments, we can close fd here */
+	if (!internal_config.single_file_segments)
+		close(fd);
 
 	rte_iova_t iova = rte_mem_virt2iova(addr);
 	if (iova == RTE_BAD_PHYS_ADDR) {
-- 
2.7.4

Re: [PATCH v3 3/9] mem: fix potential double close

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:41AM +0100, Anatoly Burakov wrote:
> We were closing descriptor before checking if mapping has
> failed, but if it did, we did a second close afterwards. Fix
> it by moving closing descriptor to after we check if mmap has
> succeeded.
> 
> Coverity issue: 272560
> 
> Fixes: 2a04139f66b4 ("eal: add single file segments option")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Is a better fix not to assign fd to -1 after closing and then checking that
in the error leg?

Re: [PATCH v3 3/9] mem: fix potential double close

[Burakov, Anatoly]

On 27-Apr-18 4:15 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:41AM +0100, Anatoly Burakov wrote:
>> We were closing descriptor before checking if mapping has
>> failed, but if it did, we did a second close afterwards. Fix
>> it by moving closing descriptor to after we check if mmap has
>> succeeded.
>>
>> Coverity issue: 272560
>>
>> Fixes: 2a04139f66b4 ("eal: add single file segments option")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
> Is a better fix not to assign fd to -1 after closing and then checking that
> in the error leg?
> 

A betterer fix would've been to move close() to until after all errors 
are checked. Will do that instead :)

-- 
Thanks,
Anatoly

[PATCH v3 4/9] mem: fix potential resource leak

[Anatoly Burakov]

We close fd if we managed to find it in the list of allocated
segment lists (which should always be the case under normal
conditions), but if we didn't, the fd was leaking. Close it if
we couldn't find it in the segment list. This is not an issue
as if the segment is zero length, we're getting rid of it
anyway, so there's no harm in not storing the fd anywhere.

Coverity issue: 272568

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 9156f8b..fab5a98 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -569,6 +569,8 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi,
 			if (te != NULL && te->fd >= 0) {
 				close(te->fd);
 				te->fd = -1;
+			} else {
+				close(fd);
 			}
 			unlink(path);
 		}
-- 
2.7.4

Re: [PATCH v3 4/9] mem: fix potential resource leak

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:42AM +0100, Anatoly Burakov wrote:
> We close fd if we managed to find it in the list of allocated
> segment lists (which should always be the case under normal
> conditions), but if we didn't, the fd was leaking. Close it if
> we couldn't find it in the segment list. This is not an issue
> as if the segment is zero length, we're getting rid of it
> anyway, so there's no harm in not storing the fd anywhere.
> 
> Coverity issue: 272568
> 

This coverity issue indicates two resource leaks, while I think this patch
only closes one of them.

/Bruce

> Fixes: 2a04139f66b4 ("eal: add single file segments option")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> index 9156f8b..fab5a98 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> @@ -569,6 +569,8 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi,
>  			if (te != NULL && te->fd >= 0) {
>  				close(te->fd);
>  				te->fd = -1;
> +			} else {
> +				close(fd);
>  			}
>  			unlink(path);
>  		}
> -- 
> 2.7.4

Re: [PATCH v3 4/9] mem: fix potential resource leak

[Burakov, Anatoly]

On 27-Apr-18 4:18 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:42AM +0100, Anatoly Burakov wrote:
>> We close fd if we managed to find it in the list of allocated
>> segment lists (which should always be the case under normal
>> conditions), but if we didn't, the fd was leaking. Close it if
>> we couldn't find it in the segment list. This is not an issue
>> as if the segment is zero length, we're getting rid of it
>> anyway, so there's no harm in not storing the fd anywhere.
>>
>> Coverity issue: 272568
>>
> 
> This coverity issue indicates two resource leaks, while I think this patch
> only closes one of them.
> 

You're right. Didn't notice this Coverity "feature"...

-- 
Thanks,
Anatoly

Re: [PATCH v3 4/9] mem: fix potential resource leak

[Burakov, Anatoly]

On 27-Apr-18 4:18 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:42AM +0100, Anatoly Burakov wrote:
>> We close fd if we managed to find it in the list of allocated
>> segment lists (which should always be the case under normal
>> conditions), but if we didn't, the fd was leaking. Close it if
>> we couldn't find it in the segment list. This is not an issue
>> as if the segment is zero length, we're getting rid of it
>> anyway, so there's no harm in not storing the fd anywhere.
>>
>> Coverity issue: 272568
>>
> 
> This coverity issue indicates two resource leaks, while I think this patch
> only closes one of them.

The other issue is actually a false positive. We couldn't have gotten 
the fd if there wasn't a tailq entry for that fd, but if we don't resize 
and remove the file, we want to keep the fd. So the "int fd" goes out of 
scope, but actually it's stored in the tailq, and thus doesn't leak.

-- 
Thanks,
Anatoly

Re: [PATCH v3 4/9] mem: fix potential resource leak

[Bruce Richardson]

On Fri, Apr 27, 2018 at 04:39:34PM +0100, Burakov, Anatoly wrote:
> On 27-Apr-18 4:18 PM, Bruce Richardson wrote:
> > On Wed, Apr 25, 2018 at 10:56:42AM +0100, Anatoly Burakov wrote:
> > > We close fd if we managed to find it in the list of allocated
> > > segment lists (which should always be the case under normal
> > > conditions), but if we didn't, the fd was leaking. Close it if
> > > we couldn't find it in the segment list. This is not an issue
> > > as if the segment is zero length, we're getting rid of it
> > > anyway, so there's no harm in not storing the fd anywhere.
> > > 
> > > Coverity issue: 272568
> > > 
> > 
> > This coverity issue indicates two resource leaks, while I think this patch
> > only closes one of them.
> 
> The other issue is actually a false positive. We couldn't have gotten the fd
> if there wasn't a tailq entry for that fd, but if we don't resize and remove
> the file, we want to keep the fd. So the "int fd" goes out of scope, but
> actually it's stored in the tailq, and thus doesn't leak.
> 
I'm not sure coverity is going to recognise that fact. However, given that
this is a fix for one of the flagged problems:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 5/9] mem: fix potential resource leak

[Anatoly Burakov]

Normally, tailq entry should have a valid fd by the time we attempt
to map the segment. However, in case it doesn't, we're leaking fd,
so fix it.

Coverity issue: 272570

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index fab5a98..b02e3a5 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 			if (te != NULL && te->fd >= 0) {
 				close(te->fd);
 				te->fd = -1;
+			} else {
+				close(fd);
 			}
 			/* ignore errors, can't make it any worse */
 			unlink(path);
-- 
2.7.4

Re: [PATCH v3 5/9] mem: fix potential resource leak

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
> Normally, tailq entry should have a valid fd by the time we attempt
> to map the segment. However, in case it doesn't, we're leaking fd,
> so fix it.
> 
> Coverity issue: 272570
> 
> Fixes: 2a04139f66b4 ("eal: add single file segments option")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> index fab5a98..b02e3a5 100644
> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
>  			if (te != NULL && te->fd >= 0) {
>  				close(te->fd);
>  				te->fd = -1;

Is "fd" still not being leaked here, since we won't hit the else case and
then jump to the end of the function where it goes out of scope?

> +			} else {
> +				close(fd);
>  			}
>  			/* ignore errors, can't make it any worse */
>  			unlink(path);
> -- 
> 2.7.4

Re: [PATCH v3 5/9] mem: fix potential resource leak

[Burakov, Anatoly]

On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
>> Normally, tailq entry should have a valid fd by the time we attempt
>> to map the segment. However, in case it doesn't, we're leaking fd,
>> so fix it.
>>
>> Coverity issue: 272570
>>
>> Fixes: 2a04139f66b4 ("eal: add single file segments option")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> index fab5a98..b02e3a5 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
>>   			if (te != NULL && te->fd >= 0) {
>>   				close(te->fd);
>>   				te->fd = -1;
> 
> Is "fd" still not being leaked here, since we won't hit the else case and
> then jump to the end of the function where it goes out of scope?

Technically, the "else" case is never valid here. If we have a tailq 
entry - we always have a valid fd. So perhaps it should be classified as 
a false positive.

-- 
Thanks,
Anatoly

Re: [PATCH v3 5/9] mem: fix potential resource leak

[Bruce Richardson]

On Fri, Apr 27, 2018 at 04:49:03PM +0100, Burakov, Anatoly wrote:
> On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
> > On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
> > > Normally, tailq entry should have a valid fd by the time we attempt
> > > to map the segment. However, in case it doesn't, we're leaking fd,
> > > so fix it.
> > > 
> > > Coverity issue: 272570
> > > 
> > > Fixes: 2a04139f66b4 ("eal: add single file segments option")
> > > Cc: anatoly.burakov@intel.com
> > > 
> > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > > ---
> > >   lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > index fab5a98..b02e3a5 100644
> > > --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
> > >   			if (te != NULL && te->fd >= 0) {
> > >   				close(te->fd);
> > >   				te->fd = -1;
> > 
> > Is "fd" still not being leaked here, since we won't hit the else case and
> > then jump to the end of the function where it goes out of scope?
> 
> Technically, the "else" case is never valid here. If we have a tailq entry -
> we always have a valid fd. So perhaps it should be classified as a false
> positive.
> 
If there is a (non-harmful) way to fix it in the code, I'd definitely thing
we should do so. It means that any other projects which use coverity scans,
or other static analysis scans, on DPDK code won't have to re-disposition
the issue. Also, if we ever start having separate scans of the different
trees, we'll similarly see benefit of not having to mark false positives
multiple times.

/Bruce

Re: [PATCH v3 5/9] mem: fix potential resource leak

[Burakov, Anatoly]

On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
> On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
>> Normally, tailq entry should have a valid fd by the time we attempt
>> to map the segment. However, in case it doesn't, we're leaking fd,
>> so fix it.
>>
>> Coverity issue: 272570
>>
>> Fixes: 2a04139f66b4 ("eal: add single file segments option")
>> Cc: anatoly.burakov@intel.com
>>
>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>> ---
>>   lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> index fab5a98..b02e3a5 100644
>> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>> @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
>>   			if (te != NULL && te->fd >= 0) {
>>   				close(te->fd);
>>   				te->fd = -1;
> 
> Is "fd" still not being leaked here, since we won't hit the else case and
> then jump to the end of the function where it goes out of scope?

Perhaps i should clarify - te->fd and fd are the same fd.

> 
>> +			} else {
>> +				close(fd);
>>   			}
>>   			/* ignore errors, can't make it any worse */
>>   			unlink(path);
>> -- 
>> 2.7.4
> 


-- 
Thanks,
Anatoly

Re: [PATCH v3 5/9] mem: fix potential resource leak

[Bruce Richardson]

On Fri, Apr 27, 2018 at 04:55:51PM +0100, Burakov, Anatoly wrote:
> On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
> > On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
> > > Normally, tailq entry should have a valid fd by the time we attempt
> > > to map the segment. However, in case it doesn't, we're leaking fd,
> > > so fix it.
> > > 
> > > Coverity issue: 272570
> > > 
> > > Fixes: 2a04139f66b4 ("eal: add single file segments option")
> > > Cc: anatoly.burakov@intel.com
> > > 
> > > Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> > > ---
> > >   lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > index fab5a98..b02e3a5 100644
> > > --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
> > > @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
> > >   			if (te != NULL && te->fd >= 0) {
> > >   				close(te->fd);
> > >   				te->fd = -1;
> > 
> > Is "fd" still not being leaked here, since we won't hit the else case and
> > then jump to the end of the function where it goes out of scope?
> 
> Perhaps i should clarify - te->fd and fd are the same fd.
> 
Can you clarify that to coverity somehow?

Re: [PATCH v3 5/9] mem: fix potential resource leak

[Burakov, Anatoly]

On 27-Apr-18 5:27 PM, Bruce Richardson wrote:
> On Fri, Apr 27, 2018 at 04:55:51PM +0100, Burakov, Anatoly wrote:
>> On 27-Apr-18 4:21 PM, Bruce Richardson wrote:
>>> On Wed, Apr 25, 2018 at 10:56:43AM +0100, Anatoly Burakov wrote:
>>>> Normally, tailq entry should have a valid fd by the time we attempt
>>>> to map the segment. However, in case it doesn't, we're leaking fd,
>>>> so fix it.
>>>>
>>>> Coverity issue: 272570
>>>>
>>>> Fixes: 2a04139f66b4 ("eal: add single file segments option")
>>>> Cc: anatoly.burakov@intel.com
>>>>
>>>> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
>>>> ---
>>>>    lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>>>> index fab5a98..b02e3a5 100644
>>>> --- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>>>> +++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
>>>> @@ -524,6 +524,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
>>>>    			if (te != NULL && te->fd >= 0) {
>>>>    				close(te->fd);
>>>>    				te->fd = -1;
>>>
>>> Is "fd" still not being leaked here, since we won't hit the else case and
>>> then jump to the end of the function where it goes out of scope?
>>
>> Perhaps i should clarify - te->fd and fd are the same fd.
>>
> Can you clarify that to coverity somehow?
> 

I don't think i can. The fd comes from get_seg_fd(), which finds the 
tailq entry and either returns existing fd, or opens a new one - and the 
same tailq entry is later looked up by alloc_seg(). Technically, of 
course, tailq contents might change inbetween the calls, but really 
that's not possible as only one thread in any given process is ever 
running through this code.

-- 
Thanks,
Anatoly

[PATCH v3 6/9] mem: fix comparing pointer to value

[Anatoly Burakov]

Previous code had an old rebase leftover from the time when
oldpolicy was an actual int, instead of a pointer. Fix it to
do comparison with dereferencing the pointer.

Coverity issue: 272589

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index b02e3a5..8420a26 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -146,7 +146,7 @@ resotre_numa(int *oldpolicy, struct bitmask *oldmask)
 {
 	RTE_LOG(DEBUG, EAL,
 		"Restoring previous memory policy: %d\n", *oldpolicy);
-	if (oldpolicy == MPOL_DEFAULT) {
+	if (*oldpolicy == MPOL_DEFAULT) {
 		numa_set_localalloc();
 	} else if (set_mempolicy(*oldpolicy, oldmask->maskp,
 				 oldmask->size + 1) < 0) {
-- 
2.7.4

Re: [PATCH v3 6/9] mem: fix comparing pointer to value

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:44AM +0100, Anatoly Burakov wrote:
> Previous code had an old rebase leftover from the time when
> oldpolicy was an actual int, instead of a pointer. Fix it to
> do comparison with dereferencing the pointer.
> 
> Coverity issue: 272589
> 
> Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 7/9] mem: fix potential bad unmap

[Anatoly Burakov]

Previously, if mmap failed to map page address at requested
address, we were attempting to unmap the wrong address. Fix it
by unmapping our actual mapped address, and jump further to
avoid unmapping memory that is not allocated.

Coverity issue: 272602

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 8420a26..6a75e5b 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -466,7 +466,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	}
 	if (va != addr) {
 		RTE_LOG(DEBUG, EAL, "%s(): wrong mmap() address\n", __func__);
-		goto mapped;
+		munmap(va, alloc_sz);
+		goto resized;
 	}
 	/* for non-single file segments, we can close fd here */
 	if (!internal_config.single_file_segments)
-- 
2.7.4

Re: [PATCH v3 7/9] mem: fix potential bad unmap

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:45AM +0100, Anatoly Burakov wrote:
> Previously, if mmap failed to map page address at requested
> address, we were attempting to unmap the wrong address. Fix it
> by unmapping our actual mapped address, and jump further to
> avoid unmapping memory that is not allocated.
> 
> Coverity issue: 272602
> 
> Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 8/9] mem: fix statement having no effect

[Anatoly Burakov]

Coverity reports these lines as having no effect. Technically, we do
want for those lines to have no effect, however they would've likely
been optimized out. Add volatile qualifiers to ensure the code has
effects.

Coverity issue: 272608

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 6a75e5b..655c69e 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -503,7 +503,12 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 			(unsigned int)(alloc_sz >> 20));
 		goto mapped;
 	}
-	*(int *)addr = *(int *)addr;
+	/* we need to trigger a write to the page to enforce page fault and
+	 * ensure that page is accessible to us, but we can't overwrite value
+	 * that is already there, so read the old value, and write itback.
+	 * kernel populates the page with zeroes initially.
+	 */
+	*(volatile int *)addr = *(volatile int *)addr;
 
 	ms->addr = addr;
 	ms->hugepage_sz = alloc_sz;
-- 
2.7.4

Re: [PATCH v3 8/9] mem: fix statement having no effect

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:46AM +0100, Anatoly Burakov wrote:
> Coverity reports these lines as having no effect. Technically, we do
> want for those lines to have no effect, however they would've likely
> been optimized out. Add volatile qualifiers to ensure the code has
> effects.
> 
> Coverity issue: 272608
> 
> Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v3 9/9] mem: fix possible use-after-free

[Anatoly Burakov]

If user has specified a flag to unmap the area right after mapping it,
we were passing an already-unmapped pointer to RTE_LOG. This is not an
issue since RTE_LOG doesn't actually dereference the pointer, but fix
it anyway by moving call to RTE_LOG to before unmap.

Coverity issue: 272584

Fixes: b7cc54187ea4 ("mem: move virtual area function in common directory")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
---
 lib/librte_eal/common/eal_common_memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 24a9ed5..3e30c58 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -113,12 +113,12 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
 		RTE_LOG(WARNING, EAL, "   This may cause issues with mapping memory into secondary processes\n");
 	}
 
-	if (unmap)
-		munmap(mapped_addr, map_sz);
-
 	RTE_LOG(DEBUG, EAL, "Virtual area found at %p (size = 0x%zx)\n",
 		aligned_addr, *size);
 
+	if (unmap)
+		munmap(mapped_addr, map_sz);
+
 	baseaddr_offset += *size;
 
 	return aligned_addr;
-- 
2.7.4

Re: [PATCH v3 9/9] mem: fix possible use-after-free

[Bruce Richardson]

On Wed, Apr 25, 2018 at 10:56:47AM +0100, Anatoly Burakov wrote:
> If user has specified a flag to unmap the area right after mapping it,
> we were passing an already-unmapped pointer to RTE_LOG. This is not an
> issue since RTE_LOG doesn't actually dereference the pointer, but fix
> it anyway by moving call to RTE_LOG to before unmap.
> 
> Coverity issue: 272584
> 
> Fixes: b7cc54187ea4 ("mem: move virtual area function in common directory")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
>  lib/librte_eal/common/eal_common_memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
> index 24a9ed5..3e30c58 100644
> --- a/lib/librte_eal/common/eal_common_memory.c
> +++ b/lib/librte_eal/common/eal_common_memory.c
> @@ -113,12 +113,12 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
>  		RTE_LOG(WARNING, EAL, "   This may cause issues with mapping memory into secondary processes\n");
>  	}
>  
> -	if (unmap)
> -		munmap(mapped_addr, map_sz);
> -
>  	RTE_LOG(DEBUG, EAL, "Virtual area found at %p (size = 0x%zx)\n",
>  		aligned_addr, *size);
>  
> +	if (unmap)
> +		munmap(mapped_addr, map_sz);
> +
>  	baseaddr_offset += *size;
>  
>  	return aligned_addr;
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v4 0/9] Coverity fixes for EAL memory

[Anatoly Burakov]

This patchset fixes a host of coverity issues in memory subsystem
introduced with recent DPDK memory hotplug patchset.

Coverity issues fixed:
- 272601 - leaking fd
- 272560 - double close fd
- 272568 - leaking fd
- 272570 - leaking fd
- 272589 - dereference before null check
- 272602 - freeing wrong pointer
- 272608 - expression does nothing
- 272584 - use after free

Coverity issues not fixed:
- 272577 - negative return not handled
- 272578 - negative return not handled
  - Proper usage of API guarantees no negative returns

Additionally, also replace all instances of snprintf with strlcpy.

v4:
- Better comments for fd leak fixes
- Improve fix for double close fd

v3:
- Drop fixes for 272577 and 272578 and mark them as false positives

v2:
- Rebase on top of latest master

Anatoly Burakov (9):
  mem: use strlcpy instead of snprintf
  mem: fix resource leak
  mem: fix potential double close
  mem: fix potential resource leak
  mem: fix potential resource leak
  mem: fix comparing pointer to value
  mem: fix potential bad unmap
  mem: fix statement having no effect
  mem: fix possible use-after-free

 lib/librte_eal/common/eal_common_memalloc.c |  5 +++--
 lib/librte_eal/common/eal_common_memory.c   |  6 +++---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c  | 33 ++++++++++++++++++-----------
 lib/librte_eal/linuxapp/eal/eal_memory.c    |  1 +
 4 files changed, 28 insertions(+), 17 deletions(-)

-- 
2.7.4

[PATCH v4 1/9] mem: use strlcpy instead of snprintf

[Anatoly Burakov]

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---

Notes:
    One other instance of using snprintf is left alone as
    it is expected to be addressed by another patch [1].
    
    [1] http://dpdk.org/dev/patchwork/patch/38301/

 lib/librte_eal/common/eal_common_memalloc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memalloc.c b/lib/librte_eal/common/eal_common_memalloc.c
index 49fd53c..e983688 100644
--- a/lib/librte_eal/common/eal_common_memalloc.c
+++ b/lib/librte_eal/common/eal_common_memalloc.c
@@ -10,6 +10,7 @@
 #include <rte_memzone.h>
 #include <rte_memory.h>
 #include <rte_eal_memconfig.h>
+#include <rte_string_fns.h>
 #include <rte_rwlock.h>
 
 #include "eal_private.h"
@@ -179,7 +180,7 @@ eal_memalloc_mem_event_callback_register(const char *name,
 
 	/* callback successfully created and is valid, add it to the list */
 	entry->clb = clb;
-	snprintf(entry->name, RTE_MEM_EVENT_CALLBACK_NAME_LEN, "%s", name);
+	strlcpy(entry->name, name, RTE_MEM_EVENT_CALLBACK_NAME_LEN);
 	TAILQ_INSERT_TAIL(&mem_event_callback_list, entry, next);
 
 	ret = 0;
@@ -284,7 +285,7 @@ eal_memalloc_mem_alloc_validator_register(const char *name,
 	entry->clb = clb;
 	entry->socket_id = socket_id;
 	entry->limit = limit;
-	snprintf(entry->name, RTE_MEM_ALLOC_VALIDATOR_NAME_LEN, "%s", name);
+	strlcpy(entry->name, name, RTE_MEM_ALLOC_VALIDATOR_NAME_LEN);
 	TAILQ_INSERT_TAIL(&mem_alloc_validator_list, entry, next);
 
 	ret = 0;
-- 
2.7.4

[PATCH v4 2/9] mem: fix resource leak

[Anatoly Burakov]

Coverity issue: 272601

Fixes: 66cc45e293ed ("mem: replace memseg with memseg lists")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memory.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memory.c b/lib/librte_eal/linuxapp/eal/eal_memory.c
index fadc1de..9351e84 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memory.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memory.c
@@ -1750,6 +1750,7 @@ eal_legacy_hugepage_attach(void)
 		if (map_addr == MAP_FAILED) {
 			RTE_LOG(ERR, EAL, "Could not map %s: %s\n",
 				hf->filepath, strerror(errno));
+			close(fd);
 			goto error;
 		}
 
-- 
2.7.4

[PATCH v4 3/9] mem: fix potential double close

[Anatoly Burakov]

We were closing descriptor before checking if mapping has
failed, but if it did, we did a second close afterwards. Fix
it by moving closing descriptor to after we've done all error
checks.

Coverity issue: 272560

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

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

Notes:
    v4:
    - Moved fd close to until after all error checks are done

 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 1f553dd..3391ed1 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -458,9 +458,6 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	 */
 	void *va = mmap(addr, alloc_sz, PROT_READ | PROT_WRITE,
 			MAP_SHARED | MAP_POPULATE | MAP_FIXED, fd, map_offset);
-	/* for non-single file segments, we can close fd here */
-	if (!internal_config.single_file_segments)
-		close(fd);
 
 	if (va == MAP_FAILED) {
 		RTE_LOG(DEBUG, EAL, "%s(): mmap() failed: %s\n", __func__,
@@ -502,6 +499,10 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 			(unsigned int)(alloc_sz >> 20));
 		goto mapped;
 	}
+	/* for non-single file segments, we can close fd here */
+	if (!internal_config.single_file_segments)
+		close(fd);
+
 	*(int *)addr = *(int *)addr;
 
 	ms->addr = addr;
-- 
2.7.4

Re: [PATCH v4 3/9] mem: fix potential double close

[Bruce Richardson]

On Fri, Apr 27, 2018 at 06:07:04PM +0100, Anatoly Burakov wrote:
> We were closing descriptor before checking if mapping has
> failed, but if it did, we did a second close afterwards. Fix
> it by moving closing descriptor to after we've done all error
> checks.
> 
> Coverity issue: 272560
> 
> Fixes: 2a04139f66b4 ("eal: add single file segments option")
> Cc: anatoly.burakov@intel.com
> 
> Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
> ---
> 
> Notes:
>     v4:
>     - Moved fd close to until after all error checks are done
> 
Acked-by: Bruce Richardson <bruce.richardson@intel.com>

[PATCH v4 4/9] mem: fix potential resource leak

[Anatoly Burakov]

We close fd if we managed to find it in the list of allocated
segment lists (which should always be the case under normal
conditions), but if we didn't, the fd was leaking. Close it if
we couldn't find it in the segment list. This is not an issue
as if the segment is zero length, we're getting rid of it
anyway, so there's no harm in not storing the fd anywhere.

Coverity issue: 272568

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---

Notes:
    v4:
    - Unconditionally close fd
    - Added comments explaining what happens if we don't remove the file

 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 3391ed1..5ea6dd3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -567,12 +567,13 @@ free_seg(struct rte_memseg *ms, struct hugepage_info *hi,
 		 */
 		if (is_zero_length(fd)) {
 			struct msl_entry *te = get_msl_entry_by_idx(list_idx);
-			if (te != NULL && te->fd >= 0) {
-				close(te->fd);
+			/* te->fd is equivalent to fd */
+			if (te != NULL && te->fd >= 0)
 				te->fd = -1;
-			}
 			unlink(path);
+			close(fd);
 		}
+		/* if we're not removing the file, fd stays in the tailq */
 		ret = 0;
 	} else {
 		/* if we're able to take out a write lock, we're the last one
-- 
2.7.4

[PATCH v4 5/9] mem: fix potential resource leak

[Anatoly Burakov]

Normally, tailq entry should have a valid fd by the time we attempt
to map the segment. However, in case it doesn't, we're leaking fd,
so fix it.

Coverity issue: 272570

Fixes: 2a04139f66b4 ("eal: add single file segments option")
Cc: anatoly.burakov@intel.com

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

Notes:
    v4:
    - Unconditionally close fd on remove
    - Clarify what happens if file is not removed

 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 5ea6dd3..d366f0a 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -522,13 +522,14 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 		resize_hugefile(fd, map_offset, alloc_sz, false);
 		if (is_zero_length(fd)) {
 			struct msl_entry *te = get_msl_entry_by_idx(list_idx);
-			if (te != NULL && te->fd >= 0) {
-				close(te->fd);
+			/* te->fd is equivalent to fd */
+			if (te != NULL && te->fd >= 0)
 				te->fd = -1;
-			}
 			/* ignore errors, can't make it any worse */
 			unlink(path);
+			close(fd);
 		}
+		/* if we're not removing the file, fd stays in the tailq */
 	} else {
 		close(fd);
 		unlink(path);
-- 
2.7.4

[PATCH v4 6/9] mem: fix comparing pointer to value

[Anatoly Burakov]

Previous code had an old rebase leftover from the time when
oldpolicy was an actual int, instead of a pointer. Fix it to
do comparison with dereferencing the pointer.

Coverity issue: 272589

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index d366f0a..604ce6d 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -146,7 +146,7 @@ resotre_numa(int *oldpolicy, struct bitmask *oldmask)
 {
 	RTE_LOG(DEBUG, EAL,
 		"Restoring previous memory policy: %d\n", *oldpolicy);
-	if (oldpolicy == MPOL_DEFAULT) {
+	if (*oldpolicy == MPOL_DEFAULT) {
 		numa_set_localalloc();
 	} else if (set_mempolicy(*oldpolicy, oldmask->maskp,
 				 oldmask->size + 1) < 0) {
-- 
2.7.4

[PATCH v4 7/9] mem: fix potential bad unmap

[Anatoly Burakov]

Previously, if mmap failed to map page address at requested
address, we were attempting to unmap the wrong address. Fix it
by unmapping our actual mapped address, and jump further to
avoid unmapping memory that is not allocated.

Coverity issue: 272602

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index 604ce6d..a40cfd3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -466,7 +466,8 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	}
 	if (va != addr) {
 		RTE_LOG(DEBUG, EAL, "%s(): wrong mmap() address\n", __func__);
-		goto mapped;
+		munmap(va, alloc_sz);
+		goto resized;
 	}
 
 	rte_iova_t iova = rte_mem_virt2iova(addr);
-- 
2.7.4

[PATCH v4 8/9] mem: fix statement having no effect

[Anatoly Burakov]

Coverity reports these lines as having no effect. Technically, we do
want for those lines to have no effect, however they would've likely
been optimized out. Add volatile qualifiers to ensure the code has
effects.

Coverity issue: 272608

Fixes: 582bed1e1d1d ("mem: support mapping hugepages at runtime")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_memalloc.c b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
index a40cfd3..672a1be 100644
--- a/lib/librte_eal/linuxapp/eal/eal_memalloc.c
+++ b/lib/librte_eal/linuxapp/eal/eal_memalloc.c
@@ -504,7 +504,12 @@ alloc_seg(struct rte_memseg *ms, void *addr, int socket_id,
 	if (!internal_config.single_file_segments)
 		close(fd);
 
-	*(int *)addr = *(int *)addr;
+	/* we need to trigger a write to the page to enforce page fault and
+	 * ensure that page is accessible to us, but we can't overwrite value
+	 * that is already there, so read the old value, and write itback.
+	 * kernel populates the page with zeroes initially.
+	 */
+	*(volatile int *)addr = *(volatile int *)addr;
 
 	ms->addr = addr;
 	ms->hugepage_sz = alloc_sz;
-- 
2.7.4

[PATCH v4 9/9] mem: fix possible use-after-free

[Anatoly Burakov]

If user has specified a flag to unmap the area right after mapping it,
we were passing an already-unmapped pointer to RTE_LOG. This is not an
issue since RTE_LOG doesn't actually dereference the pointer, but fix
it anyway by moving call to RTE_LOG to before unmap.

Coverity issue: 272584

Fixes: b7cc54187ea4 ("mem: move virtual area function in common directory")
Cc: anatoly.burakov@intel.com

Signed-off-by: Anatoly Burakov <anatoly.burakov@intel.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/librte_eal/common/eal_common_memory.c b/lib/librte_eal/common/eal_common_memory.c
index 24a9ed5..3e30c58 100644
--- a/lib/librte_eal/common/eal_common_memory.c
+++ b/lib/librte_eal/common/eal_common_memory.c
@@ -113,12 +113,12 @@ eal_get_virtual_area(void *requested_addr, size_t *size,
 		RTE_LOG(WARNING, EAL, "   This may cause issues with mapping memory into secondary processes\n");
 	}
 
-	if (unmap)
-		munmap(mapped_addr, map_sz);
-
 	RTE_LOG(DEBUG, EAL, "Virtual area found at %p (size = 0x%zx)\n",
 		aligned_addr, *size);
 
+	if (unmap)
+		munmap(mapped_addr, map_sz);
+
 	baseaddr_offset += *size;
 
 	return aligned_addr;
-- 
2.7.4

Re: [PATCH v4 0/9] Coverity fixes for EAL memory

[Thomas Monjalon]

> Anatoly Burakov (9):
>   mem: use strlcpy instead of snprintf
>   mem: fix resource leak
>   mem: fix potential double close
>   mem: fix potential resource leak
>   mem: fix potential resource leak
>   mem: fix comparing pointer to value
>   mem: fix potential bad unmap
>   mem: fix statement having no effect
>   mem: fix possible use-after-free

Applied (with improved titles), thanks