|SUCCESS| [v5] 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/164053

_AI Code Review_

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

Reviewing patch: net/tap: use offsets provided by rte_net_get_ptype

commit: 20260518082659.13036-2-rjarry at redhat.com
Author: Robin Jarry <rjarry at redhat.com>
Subject: [PATCH dpdk v5] net/tap: use offsets provided by rte_net_get_ptype

This patch modifies the tap driver to use header offsets from
rte_net_get_ptype() instead of manually calculating them, adding 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..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)))

Does this check validate that hlen is properly populated? If
rte_net_get_ptype() fails to populate hlen (for example, if it returns
an error or encounters malformed packets), could hlen->l2_len and
hlen->l3_len both be zero, potentially bypassing this bounds check and
causing the code to access invalid memory regions below?

>  		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;
>  
> -		cksum = ~rte_raw_cksum(iph, l3_len);
> +		cksum = ~rte_raw_cksum(iph, hlen->l3_len);

The IPv4 checksum calculation in rte_raw_cksum() expects exactly the IP
header size. Does this code correctly handle IPv4 packets with options?
The hlen->l3_len should include options, but can you confirm
rte_net_get_ptype() properly accounts for IP options when setting
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) {
>  		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;

For IPv6 with extension headers, does this bounds check work correctly?
The iph->payload_len includes extension headers, and hlen->l3_len also
includes them. Should this be using hlen->l3_len instead of sizeof(*iph)
to avoid double-counting extension headers?

[ ... ]

> @@ -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;
>  
> -		l4_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l2_len + l3_len);
>  		/* Don't verify checksum for multi-segment packets. */
>  		if (mbuf->nb_segs > 1)
>  			return;
> +
> +		l4_hdr = rte_pktmbuf_mtod_offset(mbuf, void *, l4_off);
>  		if (l3 == RTE_PTYPE_L3_IPV4 || l3 == RTE_PTYPE_L3_IPV4_EXT) {
>  			if (l4 == RTE_PTYPE_L4_UDP) {
> -				udp_hdr = (struct rte_udp_hdr *)l4_hdr;
> +				struct rte_udp_hdr *udp_hdr = l4_hdr;
>  				if (udp_hdr->dgram_cksum == 0) {

[ ... ]

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

According to DPDK coding style guidelines, variable declarations should
be at the beginning of blocks, not mixed with code. Can this declaration
be moved to the top of the function scope?

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