|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

commit a1b2c3d4e5f6
Author: Robin Jarry <rjarry at redhat.com>

net/tap: use offsets provided by rte_net_get_ptype

A brief summary of the commit:

The patch modifies the tap driver's checksum verification to use the
rte_net_hdr_lens structure returned by rte_net_get_ptype() instead of
manually calculating header offsets. This allows proper support for stacked
VLAN/QinQ tags and IPv6 extension headers.

>  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 check still provide meaningful packet validation? The old code
verified that at least IPv4 header length was available. With l4_off, the
code now requires L2+L3 headers to be present before even checking which
L3 protocol type is in use. Is there any validation path that ensures hlen
members are not zero when they should have values?

> +
> +	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);

Does this code correctly handle IPv4 options? The original l3_len was
computed via rte_ipv4_hdr_len() which accounts for the IHL field and
variable IPv4 options. Does rte_net_get_ptype() populate hlen->l3_len with
the IHL-derived length or just sizeof(struct rte_ipv4_hdr)?

>  		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;

Does this validation correctly account for IPv6 extension headers? The code
adds sizeof(*iph) which is the base IPv6 header, but IPv6 extension headers
are included in payload_len. Should this use hlen->l3_len instead of
sizeof(*iph) to properly validate packets with extensions?

>  	} 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 reliably contain the minimum L4 header length? The old
code explicitly selected minimum required lengths (UDP header vs TCP header).
Does rte_net_get_ptype() guarantee hlen->l4_len is set to at least these
minimums?

> @@ -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);

Minor style note: The declaration is at the start of a code block after
executable statements. DPDK coding standards typically require declarations
at the beginning of block scope. Should the struct rte_net_hdr_lens
declaration be moved to the top of the enclosing block?