[RFC PATCH] mempool: Fix some Coverity defects

[RFC PATCH] mempool: Fix some Coverity defects

[Morten Brørup]

Coverity reports some issues regarding the mempool library.
I have tried fixing them.
How to test if Coverity is satisfied with these fixes?

Function rte_mem_page_size() returns a negative number.
This seems to be the root cause of the following mempool defects.
And since the function is used elsewhere, it may also be the root cause
of other defects.
Coverity issue: 448869
Coverity issue: 360531

Function get_min_page_size(mp->socket_id) returns a negative number.
Coverity issue: 360532

Truncation due to cast operation on mem_size from 64 to 32 bits:
ret is assigned from mem_size.
Coverity issue: 442155

Calling rte_mempool_ops_dequeue_bulk without checking return value.
Coverity issue: 363744

And an unrelated drive-by fix in eal_mem_set_dump():
When madvise() failed, an incorrect reason was logged.

Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/unix/eal_unix_memory.c |  9 +++++++--
 lib/mempool/rte_mempool.c      | 14 ++++++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/lib/eal/unix/eal_unix_memory.c b/lib/eal/unix/eal_unix_memory.c
index c540f1e838..a796a28899 100644
--- a/lib/eal/unix/eal_unix_memory.c
+++ b/lib/eal/unix/eal_unix_memory.c
@@ -85,7 +85,7 @@ eal_mem_set_dump(void *virt, size_t size, bool dump)
 	int ret = madvise(virt, size, flags);
 	if (ret) {
 		EAL_LOG(DEBUG, "madvise(%p, %#zx, %d) failed: %s",
-				virt, size, flags, strerror(rte_errno));
+				virt, size, flags, strerror(errno));
 		rte_errno = errno;
 	}
 	return ret;
@@ -141,8 +141,13 @@ rte_mem_page_size(void)
 {
 	static size_t page_size;
 
-	if (!page_size)
+	if (page_size == 0) {
+		errno = 0; /* man sysconf(3) */
 		page_size = sysconf(_SC_PAGESIZE);
+		if ((ssize_t)page_size < 0)
+			rte_panic("sysconf(_SC_PAGESIZE) failed: %s",
+					errno == 0 ? "Indeterminate" : strerror(errno));
+	}
 
 	return page_size;
 }
diff --git a/lib/mempool/rte_mempool.c b/lib/mempool/rte_mempool.c
index 1021ede0c2..d12b5e8a79 100644
--- a/lib/mempool/rte_mempool.c
+++ b/lib/mempool/rte_mempool.c
@@ -157,7 +157,7 @@ get_min_page_size(int socket_id)
 
 	rte_memseg_list_walk(find_min_pagesz, &wa);
 
-	return wa.min == SIZE_MAX ? (size_t) rte_mem_page_size() : wa.min;
+	return wa.min == SIZE_MAX ? rte_mem_page_size() : wa.min;
 }
 
 
@@ -238,6 +238,7 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
 #endif
 
 	/* element size is 8 bytes-aligned at least */
+	RTE_VERIFY(elt_size <= UINT32_MAX - sizeof(uint64_t));
 	sz->elt_size = RTE_ALIGN_CEIL(elt_size, sizeof(uint64_t));
 
 	/* expand trailer to next cache line */
@@ -257,10 +258,13 @@ rte_mempool_calc_obj_size(uint32_t elt_size, uint32_t flags,
 		unsigned new_size;
 		new_size = arch_mem_object_align
 			    (sz->header_size + sz->elt_size + sz->trailer_size);
+		RTE_VERIFY(new_size >=
+				(uint64_t)sz->header_size + sz->elt_size + sz->trailer_size);
 		sz->trailer_size = new_size - sz->header_size - sz->elt_size;
 	}
 
 	/* this is the size of an object, including header and trailer */
+	RTE_VERIFY((uint64_t)sz->header_size + sz->elt_size + sz->trailer_size <= UINT32_MAX);
 	sz->total_size = sz->header_size + sz->elt_size + sz->trailer_size;
 
 	return sz->total_size;
@@ -280,11 +284,13 @@ static void
 rte_mempool_free_memchunks(struct rte_mempool *mp)
 {
 	struct rte_mempool_memhdr *memhdr;
+	int ret;
 	void *elt;
 
 	while (!STAILQ_EMPTY(&mp->elt_list)) {
-		rte_mempool_ops_dequeue_bulk(mp, &elt, 1);
-		(void)elt;
+		ret = rte_mempool_ops_dequeue_bulk(mp, &elt, 1);
+		RTE_SET_USED(ret);
+		RTE_SET_USED(elt);
 		STAILQ_REMOVE_HEAD(&mp->elt_list, next);
 		mp->populated_size--;
 	}
@@ -565,7 +571,7 @@ rte_mempool_populate_default(struct rte_mempool *mp)
 			mp, n, pg_shift, &min_chunk_size, &align);
 
 		if (mem_size < 0) {
-			ret = mem_size;
+			ret = (int)mem_size;
 			goto fail;
 		}
 
-- 
2.43.0

Re: [RFC PATCH] mempool: Fix some Coverity defects

[Stephen Hemminger]

On Mon,  9 Jun 2025 14:42:26 +0000
Morten Brørup <mb@smartsharesystems.com> wrote:

> @@ -141,8 +141,13 @@ rte_mem_page_size(void)
>  {
>  	static size_t page_size;
>  
> -	if (!page_size)
> +	if (page_size == 0) {
> +		errno = 0; /* man sysconf(3) */
>  		page_size = sysconf(_SC_PAGESIZE);
> +		if ((ssize_t)page_size < 0)
> +			rte_panic("sysconf(_SC_PAGESIZE) failed: %s",
> +					errno == 0 ? "Indeterminate" : strerror(errno));
> +	}

Why not make page_size a ssize_t or long?
The return value of sysconf is long.
Not sure if setting errno to zero is required, sysconf only touchs it if return value is negative.

RE: [RFC PATCH] mempool: Fix some Coverity defects

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Monday, 9 June 2025 17.26
> 
> On Mon,  9 Jun 2025 14:42:26 +0000
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > @@ -141,8 +141,13 @@ rte_mem_page_size(void)
> >  {
> >  	static size_t page_size;
> >
> > -	if (!page_size)
> > +	if (page_size == 0) {
> > +		errno = 0; /* man sysconf(3) */
> >  		page_size = sysconf(_SC_PAGESIZE);
> > +		if ((ssize_t)page_size < 0)
> > +			rte_panic("sysconf(_SC_PAGESIZE) failed: %s",
> > +					errno == 0 ? "Indeterminate" :
> strerror(errno));
> > +	}
> 
> Why not make page_size a ssize_t or long?
> The return value of sysconf is long.

I guess it's a matter of preference...
This function returns size_t, i.e. an unsigned type. So I kept page_size (the cached return value) the same type, and put the type cast in the unlikely code path.
The alternative, changing page_size to a signed type (ssize_t or long), would require casting to an unsigned type in the return statement.

> Not sure if setting errno to zero is required, sysconf only touchs it if
> return value is negative.

The Linux man sysconf(3) page recommends setting errno to zero before calling sysconf(), so errno can be used to discriminate between actual errors and an indeterminate value when sysconf() returns -1.
Hence the "man sysconf(3)" comment.
BSD sysconf(3) behaves the same way, but the BSD man page is less explicit about setting errno to zero.