|SUCCESS| [v5, 5/5] net: add truncated packet tests for rte_net_get_ptype

[dpdklab at iol.unh.edu]

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

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-05-18

# Review of DPDK Patch Series v5

git sha: 1f250674085aeb4ffd15ac2519a68efc04faf7ac (reverted)
Author: Robin Jarry <rjarry at redhat.com>
Subject: [PATCH dpdk v5 1/5] Revert "net: fix packet type for stacked VLAN"

This series addresses a regression in rte_net_get_ptype() where single
VLAN frames are misidentified as QinQ due to incorrect use of bitwise OR
with packet type flags.

## Patch 1/5: Revert "net: fix packet type for stacked VLAN"

> diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
> index 458b4814a9c9..c70b57fdc0f8 100644
> --- a/lib/net/rte_net.c
> +++ b/lib/net/rte_net.c
> @@ -320,9 +320,6 @@ rte_net_skip_ip6_ext(uint16_t proto, const struct rte_mbuf *m, uint32_t *off,
>  	return -1;
>  }
>  
> -/* limit number of supported VLAN headers */
> -#define RTE_NET_VLAN_MAX_DEPTH 8
> -

Does removing this limit create a potential for excessive iterations in
the VLAN parsing loop?

[ ... ]

> @@ -352,26 +349,30 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  	if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_IPV4))
>  		goto l3; /* fast path if packet is IPv4 */
>  
> -	while (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ||
> -	       proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ)) {
> +	if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN)) {
>  		const struct rte_vlan_hdr *vh;
>  		struct rte_vlan_hdr vh_copy;
>  
> -		if (++vlan_depth > RTE_NET_VLAN_MAX_DEPTH)
> -			return 0;
> -		pkt_type |=
> -			proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ?
> -				 RTE_PTYPE_L2_ETHER_VLAN :
> -				 RTE_PTYPE_L2_ETHER_QINQ;
> +		pkt_type = RTE_PTYPE_L2_ETHER_VLAN;
>  		vh = rte_pktmbuf_read(m, off, sizeof(*vh), &vh_copy);
>  		if (unlikely(vh == NULL))
>  			return pkt_type;
>  		off += sizeof(*vh);
>  		hdr_lens->l2_len += sizeof(*vh);
>  		proto = vh->eth_proto;
> -	}
> +	} else if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ)) {
> +		const struct rte_vlan_hdr *vh;
> +		struct rte_vlan_hdr vh_copy;
>  
> -	if ((proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS)) ||
> +		pkt_type = RTE_PTYPE_L2_ETHER_QINQ;
> +		vh = rte_pktmbuf_read(m, off + sizeof(*vh), sizeof(*vh),
> +			&vh_copy);

Does this code handle the first VLAN header correctly? The offset skips
past the first tag without processing it, but we're reading the second
tag and using its eth_proto.

> +		if (unlikely(vh == NULL))
> +			return pkt_type;
> +		off += 2 * sizeof(*vh);
> +		hdr_lens->l2_len += 2 * sizeof(*vh);
> +		proto = vh->eth_proto;
> +	} else if ((proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS)) ||
>  		(proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLSM))) {

## Patch 2/5: net: support multiple stacked VLAN tags

> diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
> index c70b57fdc0f8..d3cded961fb5 100644
> --- a/lib/net/rte_net.c
> +++ b/lib/net/rte_net.c
> @@ -349,29 +349,26 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  	if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_IPV4))
>  		goto l3; /* fast path if packet is IPv4 */
>  
> -	if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN)) {
> +	if ((proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN)) ||
> +		(proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ))) {

Inconsistent indentation: the continuation line should align with the
opening parenthesis of the outer condition or be indented with an
additional tab.

>  		const struct rte_vlan_hdr *vh;
>  		struct rte_vlan_hdr vh_copy;
>  
> -		pkt_type = RTE_PTYPE_L2_ETHER_VLAN;
> -		vh = rte_pktmbuf_read(m, off, sizeof(*vh), &vh_copy);
> -		if (unlikely(vh == NULL))
> -			return pkt_type;
> -		off += sizeof(*vh);
> -		hdr_lens->l2_len += sizeof(*vh);
> -		proto = vh->eth_proto;
> -	} else if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ)) {
> -		const struct rte_vlan_hdr *vh;
> -		struct rte_vlan_hdr vh_copy;
> +		if (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN))
> +			pkt_type = RTE_PTYPE_L2_ETHER_VLAN;
> +		else
> +			pkt_type = RTE_PTYPE_L2_ETHER_QINQ;
> +
> +		do {
> +			vh = rte_pktmbuf_read(m, off, sizeof(*vh), &vh_copy);
> +			if (unlikely(vh == NULL))
> +				return pkt_type;
> +			off += sizeof(*vh);
> +			hdr_lens->l2_len += sizeof(*vh);
> +			proto = vh->eth_proto;
> +		} while (proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_VLAN) ||
> +			proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_QINQ));

Does this loop need bounds checking? Without a maximum depth limit, can
a malformed packet cause excessive iterations?

## Patch 4/5: net: parse L3 protocol after MPLS labels

> diff --git a/lib/net/rte_net.c b/lib/net/rte_net.c
> index d3cded961fb5..3bb5fbc16d43 100644
> --- a/lib/net/rte_net.c
> +++ b/lib/net/rte_net.c
> @@ -371,22 +371,36 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>  
>  	} else if ((proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLS)) ||
>  		(proto == rte_cpu_to_be_16(RTE_ETHER_TYPE_MPLSM))) {
> -		unsigned int i;
>  		const struct rte_mpls_hdr *mh;
>  		struct rte_mpls_hdr mh_copy;
> +		const uint8_t *nimble;
> +		uint8_t nimble_copy;

Should this variable be named "nibble" rather than "nimble"?

>  
> -#define MAX_MPLS_HDR 5
> -		for (i = 0; i < MAX_MPLS_HDR; i++) {
> -			mh = rte_pktmbuf_read(m, off + (i * sizeof(*mh)),
> -				sizeof(*mh), &mh_copy);
> +		pkt_type = RTE_PTYPE_L2_ETHER_MPLS;
> +
> +		/* consume all labels until bottom of stack is reached */
> +		do {
> +			mh = rte_pktmbuf_read(m, off, sizeof(*mh), &mh_copy);
>  			if (unlikely(mh == NULL))
>  				return pkt_type;
> -		}
> -		if (i == MAX_MPLS_HDR)
> +			off += sizeof(*mh);
> +			hdr_lens->l2_len += sizeof(*mh);
> +		} while (!mh->bs);

Does this loop require bounds checking? Can a packet with no bottom of
stack bit set cause infinite iteration?

> +
> +		/* try to guess what is the payload based on the first 4 bits */
> +		nimble = rte_pktmbuf_read(m, off, sizeof(*nimble), &nimble_copy);
> +		if (nimble == NULL)

Implicit comparison: should be "if (nimble == NULL)" is correct per
coding style, but you could also write "if (unlikely(nimble == NULL))".

>  			return pkt_type;
> -		pkt_type = RTE_PTYPE_L2_ETHER_MPLS;
> -		hdr_lens->l2_len += (sizeof(*mh) * i);
> -		return pkt_type;
> +		switch (*nimble & 0xf0) {
> +		case 0x40:
> +			proto = RTE_BE16(RTE_ETHER_TYPE_IPV4);
> +			break;
> +		case 0x60:
> +			proto = RTE_BE16(RTE_ETHER_TYPE_IPV6);
> +			break;
> +		default:
> +			return pkt_type;
> +		}
>  	}

## Patch 3/5 and 5/5: Unit tests

The test additions look reasonable and follow DPDK test patterns
correctly.