|SUCCESS| [v6] net/tap: use offsets provided by rte_net_get_ptype

[dpdklab at iol.unh.edu]

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

_AI Code Review_

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

Patch SHA: 20260518205448.142884-2-rjarry at redhat.com
Author: Robin Jarry <rjarry at redhat.com>
Subject: [PATCH dpdk v6] net/tap: use offsets provided by rte_net_get_ptype

This patch modifies tap_verify_csum() to use header length information
from rte_net_get_ptype() instead of manually calculating offsets,
enabling support for stacked VLAN/QinQ tags.

> diff --git a/drivers/net/tap/rte_eth_tap.c b/drivers/net/tap/rte_eth_tap.c
> index a5d460a0b3cb..3b8e19afb7af 100644
> --- a/drivers/net/tap/rte_eth_tap.c
> +++ b/drivers/net/tap/rte_eth_tap.c

[ ... ]

> @@ -327,52 +327,41 @@ tun_alloc(struct pmd_internals *pmd, int is_keepalive, int persistent)
>  }
>  
>  static void
> -tap_verify_csum(struct rte_mbuf *mbuf)
> +tap_verify_csum(struct rte_mbuf *mbuf, const struct rte_net_hdr_lens *hlen)
>  {
> -	uint32_t l2 = mbuf->packet_type & RTE_PTYPE_L2_MASK;
>  	uint32_t l3 = mbuf->packet_type & RTE_PTYPE_L3_MASK;
>  	uint32_t l4 = mbuf->packet_type & RTE_PTYPE_L4_MASK;
> -	unsigned int l2_len = sizeof(struct rte_ether_hdr);
> -	unsigned int l3_len;
> +	uint32_t l4_off = hlen->l2_len + hlen->l3_len;
>  	uint16_t cksum = 0;
>  	void *l3_hdr;
>  	void *l4_hdr;
> -	struct rte_udp_hdr *udp_hdr;
>  
> -	if (l2 == RTE_PTYPE_L2_ETHER_VLAN)
> -		l2_len += 4;
> -	else if (l2 == RTE_PTYPE_L2_ETHER_QINQ)
> -		l2_len += 8;
>  	/* Don't verify checksum for packets with discontinuous L2 header */
> -	if (unlikely(l2_len + sizeof(struct rte_ipv4_hdr) >
> -		     rte_pktmbuf_data_len(mbuf)))
> +	if (unlikely(l4_off > rte_pktmbuf_data_len(mbuf)))
>  		return;
> -	l3_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len);
> +
> +	l3_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, hlen->l2_len);
>  	if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
>  		struct rte_ipv4_hdr *iph = l3_hdr;
>  
> -		l3_len = rte_ipv4_hdr_len(iph);
> -		if (unlikely(l2_len + l3_len > rte_pktmbuf_data_len(mbuf)))
> -			return;
>  		/* check that the total length reported by header is not
>  		 * greater than the total received size
>  		 */
> -		if (l2_len + rte_be_to_cpu_16(iph->total_length) >
> +		if (hlen->l2_len + rte_be_to_cpu_16(iph->total_length) >
>  				rte_pktmbuf_data_len(mbuf))
>  			return;

The removal of the check for "l2_len + l3_len > rte_pktmbuf_data_len"
may cause issues. Does this code correctly handle truncated IPv4 headers
now that we rely only on l4_off for validation? The original code
verified the L3 header was fully present before accessing the IPv4
header fields. Now we only check l4_off, but still access iph fields
(total_length). Can this access uninitialized or out-of-bounds memory
if the IPv4 header is truncated?

>  
> -		cksum = ~rte_raw_cksum(iph, l3_len);
> +		cksum = ~rte_raw_cksum(iph, hlen->l3_len);

Does rte_raw_cksum() safely handle cases where hlen->l3_len extends
beyond rte_pktmbuf_data_len(mbuf)? The earlier bounds check was removed.

[ ... ]

>  	} else if (l3 == RTE_PTYPE_L3_IPV6) {
>  		struct rte_ipv6_hdr *iph = l3_hdr;
>  
> -		l3_len = sizeof(struct rte_ipv6_hdr);
>  		/* check that the total length reported by header is not
>  		 * greater than the total received size
>  		 */
> -		if (l2_len + l3_len + rte_be_to_cpu_16(iph->payload_len) >
> +		if (hlen->l2_len + sizeof(*iph) + rte_be_to_cpu_16(iph->payload_len) >
>  				rte_pktmbuf_data_len(mbuf))
>  			return;

Similar concern here: the original code stored l3_len before accessing
iph fields. Does this code safely access iph->payload_len if the IPv6
header is truncated? The bounds check at l4_off may not catch a
truncated IPv6 base header.

[ ... ]

> @@ -386,20 +375,19 @@ tap_verify_csum(struct rte_mbuf *mbuf)
>  
>  	if (l4 == RTE_PTYPE_L4_UDP || l4 == RTE_PTYPE_L4_TCP) {
>  		int cksum_ok;
> -		const unsigned int l4_min_len = (l4 == RTE_PTYPE_L4_UDP)
> -			? sizeof(struct rte_udp_hdr) : sizeof(struct rte_tcp_hdr);
>  
>  		/* Don't verify checksum if L4 header is truncated */
> -		if (l2_len + l3_len + l4_min_len > rte_pktmbuf_data_len(mbuf))
> +		if (l4_off + hlen->l4_len > rte_pktmbuf_data_len(mbuf))
>  			return;

Does hlen->l4_len represent the actual L4 header length or something
else? The old code checked for minimum L4 header sizes (UDP/TCP header
length). If hlen->l4_len is the payload length instead, does this
incorrectly validate truncated L4 headers?

[ ... ]

> @@ -561,10 +549,11 @@ pmd_rx_burst(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>  			continue;
>  		}
>  
> -		mbuf->packet_type = rte_net_get_ptype(mbuf, NULL,
> +		struct rte_net_hdr_lens hlen = {0};
> +		mbuf->packet_type = rte_net_get_ptype(mbuf, &hlen,
>  						      RTE_PTYPE_ALL_MASK);
>  		if (rxq->rxmode->offloads & RTE_ETH_RX_OFFLOAD_CHECKSUM)
> -			tap_verify_csum(mbuf);
> +			tap_verify_csum(mbuf, &hlen);

Declaration placement: "struct rte_net_hdr_lens hlen = {0};" appears
mid-function after other statements. Does this violate C89/C90
requirements? According to the coding style guide, all declarations
should appear at the beginning of their scope.

>  
>  		/* account for the receive frame */
>  		bufs[num_rx++] = mbuf;