Re: [PATCH net-next v2 5/6] eth: mpnic: IPv4 and IPv6 in IPv6 tunnel segmentation

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Thu Nov 12, 2026 at 5:08 AM EST, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [Medium] mpnic_ipxip6_gso_ok() only checks the outer IPv6 header chain.
>
> --
>
> eth: mpnic: IPv4 and IPv6 in IPv6 tunnel segmentation
>
> This patch turns on NETIF_F_GSO_IPXIP6 for mpnic. The device can segment
> 4-in-6 and 6-in-6 packets. The patch adds the outer L3 header length and
> two new L3 types to the Tx descriptor, and adds
> mpnic_features_check_encap_gso() to reject skbs the hardware cannot
> handle.
>
>> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> index 2a0560b7dd19e..839feb4716b60 100644
>> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
>
> [ ... ]
>
>> @@ -228,9 +232,17 @@ static void mpnic_tx_fill_hdrs(struct sk_buff *skb, struct mpnic_tx_hdrs *hdrs)
>>  	else
>>  		hdrs->l4_type = MPNIC_TWD_L4_TYPE_UDP;
>>  
>> -	hdrs->i3len = skb_transport_offset(skb) - skb_network_offset(skb);
>> -	hdrs->l3_type = ip_hdr(skb)->version == 4 ?
>> -			MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
>> +	if (!skb->encapsulation) {
>> +		hdrs->i3len = skb_transport_offset(skb) - net_off;
>> +		hdrs->l3_type = ip_hdr(skb)->version == 4 ?
>> +				MPNIC_TWD_L3_TYPE_IPV4 : MPNIC_TWD_L3_TYPE_IPV6;
>> +	} else {
>> +		inner_off = skb_inner_network_offset(skb);
>> +		hdrs->o3len = inner_off - net_off;
>> +		hdrs->i3len = skb_checksum_start_offset(skb) - inner_off;
>> +		hdrs->l3_type = inner_ip_hdr(skb)->version == 4 ?
>> +				MPNIC_TWD_L3_TYPE_V6V4 : MPNIC_TWD_L3_TYPE_V6V6;
>> +	}
>>  }
>
> [ ... ]
>
>> @@ -362,6 +375,35 @@ netdev_tx_t mpnic_xmit_frame(struct sk_buff *skb, struct net_device *dev)
>>  	return mpnic_xmit_frame_ring(skb, mpn->tx[skb_get_queue_mapping(skb)]);
>>  }
>>  
>> +static bool mpnic_ipxip6_gso_ok(struct sk_buff *skb)
>> +{
>> +	struct ipv6hdr *ip6h;
>> +	__be16 frag_off;
>> +	u8 nexthdr;
>> +	int start;
>> +
>> +	ip6h = ipv6_hdr(skb);
>> +	if (ip6h->version != 6)
>> +		return false;
>> +
>> +	nexthdr = ip6h->nexthdr;
>> +	start = skb_network_offset(skb) + sizeof(*ip6h);
>> +	start = ipv6_skip_exthdr(skb, start, &nexthdr, &frag_off);
>> +
>> +	return !frag_off && start == skb_inner_network_offset(skb) &&
>> +	       (nexthdr == IPPROTO_IPV6 || nexthdr == IPPROTO_IPIP);
>> +}
>
> [Severity: Medium]
> Can a nested tunnel still get past this check?
>
> mpnic_ipxip6_gso_ok() validates only the outer IPv6 header chain up to
> skb_inner_network_offset(). It never checks that the inner L3 header
> leads straight to the L4 header at csum_start.
>
> Some paths reset the inner headers on an skb that is already
> encapsulated. One is bpf_lwt_push_ip_encap() in net/core/lwt_bpf.c:
>
>     skb_reset_inner_headers(skb);
>     skb_reset_inner_mac_header(skb);  /* mac header is not yet set */
>     skb_set_inner_protocol(skb, skb->protocol);
>     skb->encapsulation = 1;
>
> For an IPv6 outer header with nexthdr IPV6, handle_gso_encap() then adds
> SKB_GSO_IPXIP6. BPF_LWT_ENCAP_SEG6 via bpf_push_seg6_encap() looks
> similar.
>
> One way to reach this is an ip6_tnl device whose underlay route has an
> LWT BPF xmit program that pushes another IPv6 header:
>
>     ip6_tnl xmit (encapsulation=1, SKB_GSO_IPXIP6)
>       ip6_finish_output2()->lwtunnel_xmit()->bpf_xmit()
>         bpf_lwt_push_ip_encap()   (inner headers reset)
>
> The packet then looks like this:
>
>     outer IPv6 -> middle IPv6 -> innermost IPv4/IPv6 -> TCP
>
> Here inner_network_header points at the middle header. So
> mpnic_ipxip6_gso_ok() sees an outer v6 header, nexthdr IPV6, and
> start == inner offset, and it returns true.
>
> The skb is DODGY, so validate_xmit_skb() runs software GSO validation. That
> recursion ends in tcp_gso_segment(), where skb_gso_ok() succeeds against
> hw_enc_features and NULL is returned. The skb then reaches
> mpnic_xmit_frame() unsegmented.
>
> mpnic_tx_fill_hdrs() would then program o3len = 40, i3len = 80 (or 60 for
> an IPv4 innermost header), and L3_TYPE taken from the middle header's
> version.
>
> The descriptor only carries OHLEN and IHLEN. Would the hardware then
> skip the per-segment fixups on the innermost header, i.e. IPv6
> payload_len, or IPv4 tot_len/ID/checksum? That would put malformed
> segments on the wire where software GSO would have produced correct
> ones.
>
> The v2 changelog says nested tunnels under an IPv4 outer are now
> rejected, but this layout still has an IPv6 outer.
>
> Should mpnic_ipxip6_gso_ok() also check that the inner L3 header ends
> exactly at skb_checksum_start_offset() with TCP or UDP as its protocol?
> For IPv4 that is ihl * 4. For IPv6 that is the header plus any extension
> headers.
>

This suggestion is insane. I thought I could count on the stack to not
reset the inner headers more than once and handle nested encap like
iptunnel_handle_offloads() does, but I wasn't aware of these two cases
with LWT and seg6 encap...

It would be helpful if we could establish a definition about what the
inner headers of an skb refer to, or otherwise put some other bit of
metadata on the skb so that drivers don't need to parse the skb for
these cases.

>> +
>> +static netdev_features_t
>> +mpnic_features_check_encap_gso(struct sk_buff *skb, netdev_features_t features)
>> +{
>> +	if (skb_shinfo(skb)->gso_type & SKB_GSO_IPXIP6 &&
>> +	    mpnic_ipxip6_gso_ok(skb))
>> +		return features;
>> +
>> +	return features & ~NETIF_F_GSO_IPXIP6;
>> +}