Re: [PATCH] net/gve: fix redundant comparison of qpl_bufs with null

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 25 Sep 2026 16:16:41 +0300
Konyukhov Aleksandr <Alexander.Konyukhov@kaspersky.com> wrote:

> Memory allocation for qpl->mz and qpl->qpl_bufs occurs in the
> gve_alloc_queue_page_list() function under the condition
> if(is_rx) qpl_bufs = rte_zmalloc else mz = gve_alloc_using_mz.
> Accordingly, if qpl->mz == NULL, then memory allocation
> for qpl->qpl_bufs definitely occurred. That is, in line
> gve_ethdev.c:127, an additional check if (qpl->qpl_bufs)
> is not required.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Fixes: 9873a135bfba ("net/gve: allocate Rx QPL pages using malloc")
> Cc: pkaligineedi@google.com
> 
> Signed-off-by: Konyukhov Aleksandr <Alexander.Konyukhov@kaspersky.com>
> ---

NAK

AI review with better tooling finds this patch is bogus.

Review: [PATCH] net/gve: fix redundant comparison of qpl_bufs with null
Patchwork 170040

Error
-----

The premise of the patch is wrong. mz and qpl_bufs are members of an
anonymous union in struct gve_queue_page_list (gve_ethdev.h:76):

	union {
		const struct rte_memzone *mz; /* memzone allocated for TX queue */
		void **qpl_bufs; /* RX qpl-buffer list allocated using malloc*/
	};

They are the same word. That is what SVACE is reporting: in the else
branch qpl->mz == NULL already means qpl->qpl_bufs == NULL, so the
test is always false. The commit message has this backwards; the
allocation logic in gve_alloc_queue_page_list() is not what makes the
comparison redundant.

After the patch:

	} else {
		uint32_t i;

		for (i = 0; i < qpl->num_entries; i++)
			rte_free(qpl->qpl_bufs[i]);

qpl->qpl_bufs is NULL in that branch, so every iteration dereferences
NULL. The branch is unreachable today (a successful alloc always leaves
the union non-NULL), so runtime behaviour is unchanged, but the patch
codifies a wrong reading of the struct and leaves the real bug in place
(see Info). Not a fix. Should be replaced by the union removal below.

Warning
-------

Fixes: 9873a135bfba does not exist in the upstream tree. The commit
"net/gve: allocate Rx QPL pages using malloc" is a71168a775e6 (v25.03).
Cc: stable@dpdk.org is on the mail but not in the commit body.

Info (pre-existing, introduced by a71168a775e6, not by this patch)
------------------------------------------------------------------

Because of the union, gve_free_queue_page_list() never frees Rx pages.
For an Rx QPL, qpl_bufs is non-NULL, so qpl->mz reads non-NULL and the
first branch runs:

	if (qpl->mz) {
		rte_memzone_free(qpl->mz);
		qpl->mz = NULL;

rte_memzone_free() is handed the rte_zmalloc'd pointer array;
rte_fbarray_find_idx() rejects it, the call returns -EINVAL (ignored),
and qpl->mz = NULL also clears qpl_bufs. The following
if (qpl->qpl_bufs) is then false. Every 4K page from
gve_alloc_using_malloc() and the qpl_bufs array itself leak on each Rx
queue release / teardown, and the per-page free loop is dead code.

Fix is to drop the union so the two pointers are independent:

	dma_addr_t *page_buses; /* the dma addrs of the pages */
	const struct rte_memzone *mz; /* Tx: memzone backing the pages */
	void **qpl_bufs;              /* Rx: per-page buffers from rte_malloc */

and simplify the free path:

	if (qpl->mz) {
		rte_memzone_free(qpl->mz);
		qpl->mz = NULL;
	}
	if (qpl->qpl_bufs) {
		for (i = 0; i < qpl->num_entries; i++)
			rte_free(qpl->qpl_bufs[i]);
		rte_free(qpl->qpl_bufs);
		qpl->qpl_bufs = NULL;
	}

Costs 8 bytes per QPL. That fix is the one that should carry the
Fixes: a71168a775e6 tag and Cc: stable@dpdk.org, since v25.03 and later
leak Rx QPL memory.

Review-Result: ERROR