Re: [PATCH net-next 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 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

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()
  (drivers/net/ethernet/meta/mpnic/mpnic_txrx.c) never checks that the…
- [Medium] eth: mpnic: stale inner transport offset used in
  mpnic_tx_fill_hdrs()
- [Low] The commit message does not mention that this patch turns on
  hardware TX checksum offload for all encapsulated traffic.

--

eth: mpnic: IPv4 and IPv6 in IPv6 tunnel segmentation

This commit adds 4-in-6 and 6-in-6 TSO support to mpnic. It advertises
NETIF_F_GSO_IPXIP6, sets hw_enc_features, and fills the outer L3 header
length and the new V6V6/V6V4 L3 types in the TX descriptor. A new helper,
mpnic_features_check_encap_gso(), checks the outer IPv6 extension headers
before it allows hardware segmentation.

> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> index 83be45031a30b..e391ca6483e09 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_netdev.c

[ ... ]

> @@ -336,6 +337,7 @@ struct net_device *mpnic_netdev_alloc(struct mpnic_dev *mpd)
>  
>  	netdev->hw_features |= netdev->features;
>  	netdev->vlan_features |= netdev->features;
> +	netdev->hw_enc_features |= netdev->features;

[Severity: Low]
This line in mpnic_netdev_alloc() copies the whole feature set into
hw_enc_features, not only NETIF_F_GSO_IPXIP6. Was it meant to also turn on
NETIF_F_HW_CSUM, TSO, TSO6, TSO_ECN and GSO_UDP_L4 for all encapsulated
traffic?

Before this patch the driver did not set hw_enc_features. The only bits came
from register_netdevice():

    dev->hw_enc_features |= NETIF_F_SG | NETIF_F_GSO_PARTIAL;

netif_skb_features() masks encapsulated skbs with hw_enc_features. So until
now, CHECKSUM_PARTIAL tunnel packets were checksummed in software. That
covers VXLAN, GENEVE, GRE, IPIP/SIT, and the segments that software GSO
produces from them.

With this change those packets reach mpnic_tx_offloads() with
MPNIC_TWD_FLAG_REQ_CSO set. The non-GSO path in mpnic_tx_fill_hdrs() then
sets L3_IHLEN to cover everything from the outer IP header up to the inner
L4 header. Because TSO is now in hw_enc_features, register_netdevice() also
adds NETIF_F_TSO_MANGLEID there.

mpnic_features_check() still enforces the alignment and header length
limits, so this looks functionally ok. The commit message, though, only
describes 4-in-6 and 6-in-6 segmentation.

Could the commit message mention the tunnel checksum offload? If the wider
change isn't intended, could hw_enc_features be limited to the bits IPXIP6
needs?

>  
>  	netdev->min_mtu = IPV6_MIN_MTU;
>  	netdev->max_mtu = MPNIC_MAX_JUMBO_FRAME_SIZE - ETH_HLEN;
> diff --git a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> index 9dd61b6d374c7..c349e54a35470 100644
> --- a/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c
> +++ b/drivers/net/ethernet/meta/mpnic/mpnic_txrx.c

[ ... ]

> @@ -363,6 +376,32 @@ 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);
> +	nexthdr = ip6h->nexthdr;
> +	start = skb_network_offset(skb) + sizeof(*ip6h);
> +	start = ipv6_skip_exthdr(skb, start, &nexthdr, &frag_off);

[Severity: Medium]
Should mpnic_ipxip6_gso_ok() check that ip6h->version == 6 before it
parses this header as IPv6?

A set SKB_GSO_IPXIP6 bit doesn't guarantee that the outermost header is
IPv6. iptunnel_handle_offloads() only resets the inner headers on the first
encapsulation, and it ORs in the gso_type bits.

Take a 4-in-6 ip6_tnl packet that is then sent over a sit tunnel. It ends
up with an outer IPv4 header and gso_type TCPV4 | IPXIP6 | IPXIP4.
inner_network_header still points at the innermost IPv4 header.

mpnic_tx_fill_hdrs() labels any encapsulated GSO skb as outer IPv6:

    hdrs->l3_type = inner_ip_hdr(skb)->version == 4 ?
            MPNIC_TWD_L3_TYPE_V6V4 : MPNIC_TWD_L3_TYPE_V6V6;

The fbnic gate this code appears to follow rejects this case in
fbnic_features_check_encap_gso():

    if (ip6_hdr->version != 6)
        return features & ~FBNIC_TUN_GSO_FEATURES;

With only this patch applied, the nested case still falls back to software
GSO, because IPXIP4 is not supported yet. The gate is still the same at the
end of the series, though.

After "eth: mpnic: add GSO_PARTIAL for other tunnels", a true return from
mpnic_ipxip6_gso_ok() makes mpnic_features_check_encap_gso() return early.
That skips the outer IPv4 check:

    if (iph->version == 4 && !(iph->frag_off & htons(IP_DF)))
        return features & ~NETIF_F_GSO_MASK;

It also skips the TSO_MANGLEID requirement.

With a sit tunnel using nopmtudisc, the outer DF bit is clear and byte 6 of
the IPv4 header is 0, which is read as NEXTHDR_HOP. ipv6_skip_exthdr() then
treats bytes of the middle IPv6 addresses as extension headers.

With addresses such as saddr[12..13] = 0x33,0x00 and daddr[4..5] =
0x04,0x01, the walk ends exactly at inner_network_offset with
nexthdr == IPPROTO_IPIP, and the gate passes.

Could that send every GSO_PARTIAL segment with the same outer IPv4 ID while
DF is clear? That is the case the later DF check is meant to prevent.

SKB_GSO_DODGY packets built by BPF (bpf_skb_adjust_room() with
BPF_F_ADJ_ROOM_ENCAP_L3_IPV6), or GRO packets with a bogus version nibble,
could also reach the device labelled V6V6/V6V4 when the outer version is
not 6. Software GSO doesn't check the version either, so the effect on the
hardware in that case is unclear.

Would a check like the one fbnic has be enough here?

    if (ip6h->version != 6)
        return false;

The inner version could optionally be checked against the nexthdr value
accepted here as well.

> +
> +	return !frag_off && start == skb_inner_network_offset(skb) &&
> +	       (nexthdr == IPPROTO_IPV6 || nexthdr == IPPROTO_IPIP);
> +}
> +
> +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;
> +}

[ ... ]

> @@ -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_inner_transport_offset(skb) - inner_off;
> +		hdrs->l3_type = inner_ip_hdr(skb)->version == 4 ?
> +				MPNIC_TWD_L3_TYPE_V6V4 : MPNIC_TWD_L3_TYPE_V6V6;
> +	}
>  }
>  
>  static bool

[Severity: Medium]
Can skb_inner_transport_offset() be relied on here? In the
encapsulated GSO branch, mpnic_tx_fill_hdrs() takes the inner L3
length from inner_transport_header. The rest of the TX path finds the
L4 header through skb_checksum_start():

  - mpnic_features_check() validates i3len that way.
  - mpnic_tx_lso() seeds the L4 checksum at skb_checksum_start(skb).
  - mpnic_tx_lso() also reads the L4 header length from there.

If the two offsets differ, MPNIC_TWD_L3_IHLEN no longer agrees with
where the driver says the L4 header is.

They can differ. Take an IPv6 GSO packet with an inner extension
header, such as destination options or a routing header.
ip6_rcv_core() sets transport_header to network_header +
sizeof(struct ipv6hdr), which points at the first extension header
and not at TCP/UDP.

Suppose the packet is then encapsulated by one of these:

  - bpf_lwt_push_ip_encap()
  - bpf_skb_adjust_room()
  - seg6_do_srh()
  - the IPVS tunnel xmit
  - the nf_flow_table tunnel push

In each case inner_transport_header is copied from that unadvanced
transport_header.

validate_xmit_skb() only calls skb_set_inner_transport_header() with
skb_checksum_start_offset() when netif_needs_gso() is false.
SKB_GSO_DODGY can be set by virtio_net_hdr_to_skb(), and
bpf_lwt_push_ip_encap() and bpf_skb_net_grow() add it
unconditionally. Without NETIF_F_GSO_ROBUST, netif_needs_gso() is
then true and __skb_gso_segment() runs.

For a dodgy skb, ip6ip6_gso_segment() -> ipv6_gso_segment() ->
tcp_gso_segment() only validates the packet, updates gso_segs and
returns NULL segs. The inner ipv6_gso_segment() then jumps to out
before skb_reset_inner_headers(). So when mpnic_xmit_frame() gets
the skb, inner_transport_header still points at the inner extension
header.

In that case hdrs->i3len is short by the length of the inner
extension headers. The descriptor tells the device that L4 starts
somewhere other than where mpnic_tx_lso() seeded the pseudo-header
checksum. That would corrupt the hardware-segmented frames on the
wire, and a guest or a BPF program can trigger it.

Would it be better to compute this as
skb_checksum_start_offset(skb) - inner_off, which matches
mpnic_features_check(), mpnic_tx_lso() and fbnic_tx_lso()? The
GSO_PARTIAL branch added later in the series also uses
skb_inner_transport_offset() and may need the same change.

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