|SUCCESS| [v2] net: fix L2 ptype assignment in VLAN loop

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/163233

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-04-22

>From patchwork Wed Apr 22 10:38:38 2026
Content-Type: text/plain; charset="utf-8"
Subject: [PATCH dpdk v2] net: fix L2 ptype assignment in VLAN loop
Date: Wed, 22 Apr 2026 12:38:38 +0200
From: Robin Jarry <rjarry at redhat.com>

This patch fixes a bug where single VLAN frames are misidentified as
QinQ due to incorrect bitwise OR operation when setting the L2 packet
type.

> diff --git a/app/test/test_net_ptype.c b/app/test/test_net_ptype.c
> new file mode 100644
> index 000000000000..9619c1d54a8e
> --- /dev/null
> +++ b/app/test/test_net_ptype.c

[ ... ]

> +static int
> +test_get_ptype(struct rte_mempool *pool, const char *pktdata, size_t len,
> +	       uint32_t expected_l2, uint8_t expected_l2_len)
> +{
> +	struct rte_net_hdr_lens hdr_lens;
> +	struct rte_mbuf *m;
> +	uint32_t ptype;
> +	uint32_t l2;
> +	char *data;
> +
> +	m = rte_pktmbuf_alloc(pool);
> +	RTE_TEST_ASSERT_NOT_NULL(m, "cannot allocate mbuf");
> +
> +	data = rte_pktmbuf_append(m, len);
> +	if (data == NULL) {
> +		rte_pktmbuf_free(m);
> +		RTE_TEST_ASSERT_NOT_NULL(data, "cannot append data");
> +	}

Does this code leak m if RTE_TEST_ASSERT_NOT_NULL fails?  Looking at the
implementation in lib/eal/include/rte_test.h, the RTE_TEST_ASSERT_*
macros use "do { if (!cond) return -1; } while (0)" pattern.  When data
is NULL, rte_pktmbuf_free(m) is called, but then RTE_TEST_ASSERT_NOT_NULL
immediately returns, preventing the subsequent rte_pktmbuf_free(m) at the
end of the function from executing.  However, the mbuf was already freed
in the error path, so this is actually correct.  The pattern is a bit
unusual but not a leak.

> +
> +	memcpy(data, pktdata, len);
> +
> +	ptype = rte_net_get_ptype(m, &hdr_lens, RTE_PTYPE_ALL_MASK);
> +	l2 = ptype & RTE_PTYPE_L2_MASK;
> +
> +	rte_pktmbuf_free(m);
> +
> +	RTE_TEST_ASSERT_EQUAL(l2, expected_l2,
> +		"unexpected L2 ptype: got 0x%x, expected 0x%x",
> +		l2, expected_l2);
> +	RTE_TEST_ASSERT_EQUAL(hdr_lens.l2_len, expected_l2_len,
> +		"unexpected l2_len: got %u, expected %u",
> +		hdr_lens.l2_len, expected_l2_len);
> +
> +	return 0;
> +}

Does this code leak m when either of the RTE_TEST_ASSERT_EQUAL checks
fail?  The mbuf is freed before the assertions, so there is no leak here.

> +static int
> +test_net_ptype(void)
> +{
> +	struct rte_mempool *pool;
> +
> +	pool = rte_pktmbuf_pool_create("test_ptype_mbuf_pool",
> +			NB_MBUF, MEMPOOL_CACHE_SIZE, 0, MBUF_DATA_SIZE,
> +			SOCKET_ID_ANY);
> +	RTE_TEST_ASSERT_NOT_NULL(pool, "cannot allocate mbuf pool");
> +
> +	if (test_get_ptype(pool, pkt_ether_ipv4_udp,
> +			   sizeof(pkt_ether_ipv4_udp),
> +			   RTE_PTYPE_L2_ETHER, 14))
> +		goto fail;
> +
> +	if (test_get_ptype(pool, pkt_vlan_ipv4_udp,
> +			   sizeof(pkt_vlan_ipv4_udp),
> +			   RTE_PTYPE_L2_ETHER_VLAN, 18))
> +		goto fail;
> +
> +	if (test_get_ptype(pool, pkt_qinq_ipv4_udp,
> +			   sizeof(pkt_qinq_ipv4_udp),
> +			   RTE_PTYPE_L2_ETHER_QINQ, 22))
> +		goto fail;
> +
> +	rte_mempool_free(pool);
> +	return 0;
> +
> +fail:
> +	rte_mempool_free(pool);
> +	return -1;
> +}

Does this code leak pool when RTE_TEST_ASSERT_NOT_NULL fails after pool
creation?  Yes.  In test_net_ptype(), if the pool allocation succeeds but
the assertion fails, RTE_TEST_ASSERT_NOT_NULL returns -1 immediately
without freeing the pool.

> diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
> index 458b4814a9c9..ea5ba7019089 100644
> --- a/lib/net/rte_net.c
> +++ b/lib/net/rte_net.c
> @@ -359,7 +359,7 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  
>  		if (++vlan_depth > RTE_NET_VLAN_MAX_DEPTH)
>  			return 0;
> -		pkt_type |=
> +		pkt_type =
>  			proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ?
>  				 RTE_PTYPE_L2_ETHER_VLAN :
>  				 RTE_PTYPE_L2_ETHER_QINQ;

The fix correctly changes from |= to = to prevent the issue where single
VLAN frames were being misidentified as QinQ.