Coverity fixes for EAL memory
cover Coverity fixes for EAL memory
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(-)
Signature · 1 line
2.7.4
PATCH 1/10 mem: use strlcpy instead of snprintf
---
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(-)
lib/librte_eal/bsdapp/eal/eal_hugepage_info.c +1 -1
@@ -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;
lib/librte_eal/common/eal_common_memalloc.c +3 -2
@@ -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 ;
lib/librte_eal/linuxapp/eal/eal_memory.c +2 -2
@@ -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]);
Signature · 1 line
2.7.4
PATCH 2/10 mem: fix resource leak
In reply to Anatoly BurakovCoverity issue: 272601
--- lib/librte_eal/linuxapp/eal/eal_memory.c | 1 + 1 file changed, 1 insertion(+)
lib/librte_eal/linuxapp/eal/eal_memory.c +1 -0
@@ -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; }
Signature · 1 line
2.7.4
PATCH 3/10 mem: fix potential double close
In reply to Anatoly BurakovWe 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +3 -3
@@ -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 ) {
Signature · 1 line
2.7.4
PATCH 4/10 mem: fix potential resource leak
In reply to Anatoly BurakovWe 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -0
@@ -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); }
Signature · 1 line
2.7.4
PATCH 5/10 mem: fix potential resource leak
In reply to Anatoly BurakovNormally, 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -0
@@ -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);
Signature · 1 line
2.7.4
PATCH 6/10 mem: fix comparing pointer to value
In reply to Anatoly BurakovPrevious 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +1 -1
@@ -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 ) {
Signature · 1 line
2.7.4
PATCH 7/10 mem: fix potential bad unmap
In reply to Anatoly BurakovPreviously, 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -1
@@ -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)
Signature · 1 line
2.7.4
PATCH 8/10 mem: fix statement having no effect
In reply to Anatoly BurakovCoverity 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +6 -1
@@ -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;
Signature · 1 line
2.7.4
PATCH 9/10 mem: fix negative return value
In reply to Anatoly BurakovAlthough 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
--- lib/librte_eal/common/eal_common_memory.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
lib/librte_eal/common/eal_common_memory.c +14 -3
@@ -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 ; }
Signature · 1 line
2.7.4
PATCH 10/10 mem: fix possible use-after-free
In reply to Anatoly BurakovIf 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
--- lib/librte_eal/common/eal_common_memory.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/librte_eal/common/eal_common_memory.c +3 -3
@@ -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;
Signature · 1 line
2.7.4
17/04/2018 17:50, Anatoly Burakov:
Quoted 14 lines
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 ;)
Quoted 16 lines · Thomas Monjalon
17/04/2018 17:50, Anatoly Burakov:
Quoted 14 lines
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
Quoted 3 lines
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.
Signature · 2 lines
Thanks, Anatoly
17/04/2018 18:09, Burakov, Anatoly:
Quoted 17 lines · Thomas Monjalon
Quoted 16 lines
17/04/2018 17:50, Anatoly Burakov:
Quoted 14 lines
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
Quoted 6 lines
Quoted 2 lines
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.
v2 cover Coverity fixes for EAL memory
In reply to Anatoly BurakovThis 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(-)
Signature · 1 line
2.7.4
PATCH v2 1/10 mem: use strlcpy instead of snprintf
---
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(-)
lib/librte_eal/common/eal_common_memalloc.c +3 -2
@@ -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 ;
Signature · 1 line
2.7.4
PATCH v2 2/10 mem: fix resource leak
In reply to Anatoly BurakovCoverity issue: 272601
--- lib/librte_eal/linuxapp/eal/eal_memory.c | 1 + 1 file changed, 1 insertion(+)
lib/librte_eal/linuxapp/eal/eal_memory.c +1 -0
@@ -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; }
Signature · 1 line
2.7.4
PATCH v2 3/10 mem: fix potential double close
In reply to Anatoly BurakovWe 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +3 -3
@@ -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 ) {
Signature · 1 line
2.7.4
PATCH v2 4/10 mem: fix potential resource leak
In reply to Anatoly BurakovWe 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -0
@@ -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); }
Signature · 1 line
2.7.4
PATCH v2 5/10 mem: fix potential resource leak
In reply to Anatoly BurakovNormally, 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -0
@@ -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);
Signature · 1 line
2.7.4
PATCH v2 6/10 mem: fix comparing pointer to value
In reply to Anatoly BurakovPrevious 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +1 -1
@@ -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 ) {
Signature · 1 line
2.7.4
PATCH v2 7/10 mem: fix potential bad unmap
In reply to Anatoly BurakovPreviously, 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -1
@@ -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)
Signature · 1 line
2.7.4
PATCH v2 8/10 mem: fix statement having no effect
In reply to Anatoly BurakovCoverity 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +6 -1
@@ -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;
Signature · 1 line
2.7.4
PATCH v2 9/10 mem: fix negative return value
In reply to Anatoly BurakovAlthough 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
--- lib/librte_eal/common/eal_common_memory.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-)
lib/librte_eal/common/eal_common_memory.c +14 -3
@@ -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 ; }
Signature · 1 line
2.7.4
PATCH v2 10/10 mem: fix possible use-after-free
In reply to Anatoly BurakovIf 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
--- lib/librte_eal/common/eal_common_memory.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/librte_eal/common/eal_common_memory.c +3 -3
@@ -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;
Signature · 1 line
2.7.4
v3 cover Coverity fixes for EAL memory
In reply to Anatoly BurakovThis 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(-)
Signature · 1 line
2.7.4
PATCH v3 1/9 mem: use strlcpy instead of snprintf
---
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(-)
lib/librte_eal/common/eal_common_memalloc.c +3 -2
@@ -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 ;
Signature · 1 line
2.7.4
Re: [PATCH v3 1/9] mem: use strlcpy instead of snprintf
Quoted 3 lines · Anatoly Burakov
Signed-off-by Anatoly Burakov <[email protected]>---
PATCH v3 2/9 mem: fix resource leak
In reply to Anatoly BurakovCoverity issue: 272601
--- lib/librte_eal/linuxapp/eal/eal_memory.c | 1 + 1 file changed, 1 insertion(+)
lib/librte_eal/linuxapp/eal/eal_memory.c +1 -0
@@ -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; }
Signature · 1 line
2.7.4
Re: [PATCH v3 2/9] mem: fix resource leak
Quoted 7 lines · Anatoly Burakov
Coverity issue: 272601
Fixes 66cc45e293ed ("mem: replace memseg with memseg lists")Signed-off-by Anatoly Burakov <[email protected]>---
PATCH v3 3/9 mem: fix potential double close
In reply to Anatoly BurakovWe 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +3 -3
@@ -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 ) {
Signature · 1 line
2.7.4
Re: [PATCH v3 3/9] mem: fix potential double close
Quoted 5 lines · 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.
Quoted 7 lines
Coverity issue: 272560
Fixes 2a04139f66b4 ("eal: add single file segments option")Signed-off-by Anatoly Burakov <[email protected]>---
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
Quoted 8 lines · Bruce Richardson
Quoted 7 lines · 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
Quoted 8 lines
Quoted 5 lines
Fixes 2a04139f66b4 ("eal: add single file segments option")Signed-off-by Anatoly Burakov <[email protected]>---
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 :)
Signature · 2 lines
Thanks, Anatoly
PATCH v3 4/9 mem: fix potential resource leak
In reply to Anatoly BurakovWe 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -0
@@ -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); }
Signature · 1 line
2.7.4
Re: [PATCH v3 4/9] mem: fix potential resource leak
Quoted 9 lines · 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
This coverity issue indicates two resource leaks, while I think this patch only closes one of them.
/Bruce
Quoted 23 lines
Fixes 2a04139f66b4 ("eal: add single file segments option")Signed-off-by Anatoly Burakov <[email protected]>--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)lib/librte_eal/linuxapp/eal/eal_memalloc.c +2
-0@@ -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); }Signature · 1 line
2.7.4
Re: [PATCH v3 4/9] mem: fix potential resource leak
Quoted 8 lines · Bruce Richardson
Quoted 7 lines · 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.
Quoted 6 lines
Quoted 2 lines
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"...
Signature · 2 lines
Thanks, Anatoly
Re: [PATCH v3 4/9] mem: fix potential resource leak
In reply to Bruce RichardsonQuoted 8 lines · Bruce Richardson
Quoted 7 lines · 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.
Quoted 5 lines
Quoted 2 lines
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.
Signature · 2 lines
Thanks, Anatoly
Bruce Richardson
Acked-byRe: [PATCH v3 4/9] mem: fix potential resource leak
Quoted 12 lines · Anatoly
Quoted 11 lines · Bruce Richardson
Quoted 9 lines · 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
Quoted 8 lines
Quoted 2 lines
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:
PATCH v3 5/9 mem: fix potential resource leak
In reply to Anatoly BurakovNormally, 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -0
@@ -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);
Signature · 1 line
2.7.4
Re: [PATCH v3 5/9] mem: fix potential resource leak
Quoted 19 lines · 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")Signed-off-by Anatoly Burakov <[email protected]>--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)lib/librte_eal/linuxapp/eal/eal_memalloc.c +0
-0
lib/librte_eal/linuxapp/eal/eal_memalloc.c alloc_seg(struct rte_memseg *ms, void *addr, int socket_id, · line 526
Quoted 3 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c +0
-0@@ -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?
Quoted 7 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2
-0+ }else { +close (fd); }/* ignore errors, can't make it any worse */ unlink (path);Signature · 1 line
2.7.4
Re: [PATCH v3 5/9] mem: fix potential resource leak
Quoted 15 lines · Bruce Richardson
Quoted 14 lines · 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")Signed-off-by Anatoly Burakov <[email protected]>--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)
Quoted 11 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c alloc_seg(struct rte_memseg *ms, void *addr, int socket_id, · line 526
Quoted 8 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c +0
-0@@ -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.
Signature · 2 lines
Thanks, Anatoly
Re: [PATCH v3 5/9] mem: fix potential resource leak
Quoted 28 lines · Anatoly
Quoted 15 lines · Bruce Richardson
Quoted 14 lines · 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")Signed-off-by Anatoly Burakov <[email protected]>--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)Quoted 11 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c alloc_seg(struct rte_memseg *ms, void *addr, int socket_id, · line 526
Quoted 8 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c +0
-0@@ -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?
Quoted 4 lines
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
In reply to Bruce RichardsonQuoted 15 lines · Bruce Richardson
Quoted 14 lines · 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")Signed-off-by Anatoly Burakov <[email protected]>--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)
Quoted 11 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c alloc_seg(struct rte_memseg *ms, void *addr, int socket_id, · line 526
Quoted 8 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c +0
-0@@ -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.
Quoted 9 lines
Quoted 8 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2
-0+ }else { +close (fd); }/* ignore errors, can't make it any worse */ unlink (path);Signature · 1 line
2.7.4
Signature · 2 lines
Thanks, Anatoly
Re: [PATCH v3 5/9] mem: fix potential resource leak
Quoted 28 lines · Anatoly
Quoted 15 lines · Bruce Richardson
Quoted 14 lines · 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")Signed-off-by Anatoly Burakov <[email protected]>--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)Quoted 11 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c alloc_seg(struct rte_memseg *ms, void *addr, int socket_id, · line 526
Quoted 8 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c +0
-0@@ -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?
Quoted 2 lines
Perhaps i should clarify -
te->fdand fd are the same fd.
Can you clarify that to coverity somehow?
Re: [PATCH v3 5/9] mem: fix potential resource leak
Quoted 29 lines · Bruce Richardson
Quoted 28 lines · Anatoly
Quoted 26 lines · Bruce Richardson
Quoted 19 lines · 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")Signed-off-by Anatoly Burakov <[email protected]>--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 ++ 1 file changed, 2 insertions(+)lib/librte_eal/linuxapp/eal/eal_memalloc.c +0
-0lib/librte_eal/linuxapp/eal/eal_memalloc.c alloc_seg(struct rte_memseg *ms, void *addr, int socket_id, · line 526
Quoted 3 lines
lib/librte_eal/linuxapp/eal/eal_memalloc.c +0
-0@@ -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?
Quoted 4 lines
Quoted 2 lines
Perhaps i should clarify -
te->fdand 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.
Signature · 2 lines
Thanks, Anatoly
PATCH v3 6/9 mem: fix comparing pointer to value
In reply to Anatoly BurakovPrevious 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +1 -1
@@ -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 ) {
Signature · 1 line
2.7.4
Re: [PATCH v3 6/9] mem: fix comparing pointer to value
Quoted 11 lines · 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")Signed-off-by Anatoly Burakov <[email protected]>---
PATCH v3 7/9 mem: fix potential bad unmap
In reply to Anatoly BurakovPreviously, 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -1
@@ -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)
Signature · 1 line
2.7.4
Re: [PATCH v3 7/9] mem: fix potential bad unmap
Quoted 5 lines · 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.
Quoted 7 lines
Coverity issue: 272602
Fixes 582bed1e1d1d ("mem: support mapping hugepages at runtime")Signed-off-by Anatoly Burakov <[email protected]>---
PATCH v3 8/9 mem: fix statement having no effect
In reply to Anatoly BurakovCoverity 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +6 -1
@@ -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;
Signature · 1 line
2.7.4
Re: [PATCH v3 8/9] mem: fix statement having no effect
Quoted 11 lines · 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")Signed-off-by Anatoly Burakov <[email protected]>
PATCH v3 9/9 mem: fix possible use-after-free
In reply to Anatoly BurakovIf 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
--- lib/librte_eal/common/eal_common_memory.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/librte_eal/common/eal_common_memory.c +3 -3
@@ -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;
Signature · 1 line
2.7.4
Re: [PATCH v3 9/9] mem: fix possible use-after-free
Quoted 25 lines · 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 sinceRTE_LOGdoesn't actually dereference the pointer, but fix it anyway by moving call toRTE_LOGto before unmap.Coverity issue: 272584
Fixes b7cc54187ea4 ("mem: move virtual area function in common directory")Signed-off-by Anatoly Burakov <[email protected]>--- lib/librte_eal/common/eal_common_memory.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)lib/librte_eal/common/eal_common_memory.c +0
-2@@ -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);
lib/librte_eal/common/eal_common_memory.c eal_get_virtual_area(void *requested_addr, size_t *size, · line 124
Quoted 10 lines
lib/librte_eal/common/eal_common_memory.c +3
-1-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;
v4 cover Coverity fixes for EAL memory
In reply to Anatoly BurakovThis 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(-)
Signature · 1 line
2.7.4
PATCH v4 1/9 mem: use strlcpy instead of snprintf
---
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(-)
lib/librte_eal/common/eal_common_memalloc.c +3 -2
@@ -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 ;
Signature · 1 line
2.7.4
PATCH v4 2/9 mem: fix resource leak
In reply to Anatoly BurakovCoverity issue: 272601
--- lib/librte_eal/linuxapp/eal/eal_memory.c | 1 + 1 file changed, 1 insertion(+)
lib/librte_eal/linuxapp/eal/eal_memory.c +1 -0
@@ -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; }
Signature · 1 line
2.7.4
PATCH v4 3/9 mem: fix potential double close
In reply to Anatoly BurakovWe 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
---
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(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +4 -3
@@ -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;
Signature · 1 line
2.7.4
Re: [PATCH v4 3/9] mem: fix potential double close
Quoted 10 lines · 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")
Quoted 7 lines
Signed-off-by Anatoly Burakov <[email protected]>---
Notes:
v4:
- Moved fd close to until after all error checks are done
PATCH v4 4/9 mem: fix potential resource leak
In reply to Anatoly BurakovWe 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
---
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(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +4 -3
@@ -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
Signature · 1 line
2.7.4
PATCH v4 5/9 mem: fix potential resource leak
In reply to Anatoly BurakovNormally, 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
---
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(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +4 -3
@@ -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);
Signature · 1 line
2.7.4
PATCH v4 6/9 mem: fix comparing pointer to value
In reply to Anatoly BurakovPrevious 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +1 -1
@@ -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 ) {
Signature · 1 line
2.7.4
PATCH v4 7/9 mem: fix potential bad unmap
In reply to Anatoly BurakovPreviously, 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +2 -1
@@ -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);
Signature · 1 line
2.7.4
PATCH v4 8/9 mem: fix statement having no effect
In reply to Anatoly BurakovCoverity 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
--- lib/librte_eal/linuxapp/eal/eal_memalloc.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)
lib/librte_eal/linuxapp/eal/eal_memalloc.c +6 -1
@@ -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;
Signature · 1 line
2.7.4
PATCH v4 9/9 mem: fix possible use-after-free
In reply to Anatoly BurakovIf 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
--- lib/librte_eal/common/eal_common_memory.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
lib/librte_eal/common/eal_common_memory.c +3 -3
@@ -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;
Signature · 1 line
2.7.4
Quoted 10 lines
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