|SUCCESS| [v4] 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/163971

_AI Code Review_

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

git sha: 20260512151611.186577-2-rjarry at redhat.com
Author: Robin Jarry <rjarry at redhat.com>
Subject: [PATCH dpdk v4] net/tap: use offsets provided by rte_net_get_ptype

This patch modifies the tap_verify_csum function to use the header
lengths provided by rte_net_get_ptype instead of manually calculating
them. This change enables support for stacked VLAN/QinQ tags and IPv6
extensions.

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

[ ... ]

> @@ -327,79 +327,66 @@ 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;

Does this change in logic correctly handle all packet types? The old code
checked whether the L2 header plus an IPv4 header would fit in the first
segment. Now it checks if l4_off (L2+L3) fits. For IPv6 packets where
hlen->l3_len includes extension headers, does rte_net_get_ptype guarantee
that hlen->l3_len is populated before this function is called?

Also, should this still check for sizeof(struct rte_ipv4_hdr) or
sizeof(struct rte_ipv6_hdr) in addition to l4_off to ensure the L3 header
is accessible?

>  
> -	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;
>  
> -		cksum = ~rte_raw_cksum(iph, l3_len);
> +		cksum = ~rte_raw_cksum(iph, hlen->l3_len);
>  		mbuf->ol_flags |= cksum ?
>  			RTE_MBUF_F_RX_IP_CKSUM_BAD :
>  			RTE_MBUF_F_RX_IP_CKSUM_GOOD;
> -	} else if (l3 == RTE_PTYPE_L3_IPV6) {
> +	} else if (l3 == RTE_PTYPE_L3_IPV6 || l3 == RTE_PTYPE_L3_IPV6_EXT) {
>  		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;

Is this length check correct for IPv6 with extension headers? The
payload_len field in the IPv6 header includes extension headers. Does
using sizeof(*iph) here instead of hlen->l3_len cause the check to
under-account for extension header space, potentially allowing processing
of truncated packets?

>  	} else {
>  		/* - RTE_PTYPE_L3_IPV4_EXT_UNKNOWN cannot happen because
>  		 *   mbuf->packet_type is filled by rte_net_get_ptype() which
>  		 *   never returns this value.
> -		 * - IPv6 extensions are not supported.
>  		 */
>  		return;
>  	}
>  
>  	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 accurately represent the minimum L4 header size needed
for checksum verification? For TCP, does rte_net_get_ptype set l4_len to
the full TCP header length including options, and is this necessary for
the checksum functions below?

[ ... ]

> @@ -561,10 +548,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);
>  
>  		/* account for the receive frame */
>  		bufs[num_rx++] = mbuf;