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

[netdev-bot+sashiko@kernel.org]

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.

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110-mpnic-lso-v2-0-ccd37b051d08%40gmail.com