Re: [PATCH net v6 4/5] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Fri, Oct 23, 2026 at 03:37:48PM -0400, Willem de Bruijn wrote:
> Michael S. Tsirkin wrote:
> > As reported first by Paulos Yibelo <habte.yibelo@xxxxxxxxx> (after which
> > others, credited below, found more ways the bug can affect users) a
> > virtio net header can place CHECKSUM_PARTIAL at any in-packet offset.
> > If that state is retained, later checksum completion can modify the
> > packet after protocol processing has made decisions from it.
> > 
> > Let's fix that: let's validate csum_start and csum_offset and only keep
> > CHECKSUM_PARTIAL when csum_start and csum_offset match a parsed TCP or
> > UDP header, while handling VLAN headers, IPv4 options and IPv6 extension
> > headers.  Complete other non-GSO checksums immediately, before any
> > protocol processing runs.
> > 
> > We can't easily "complete the checksum" for GSO we don't recognize - GSO
> > assumes knowledge of the protocol. We could segment ignoring the
> > checksum then do whatever csum_start and csum_offset say, but that
> > seems like an overkill for something that shouldn't happen.
> > 
> > There's some risk here:
> > - non-gso packets which can't be parsed get checksum calculated in
> >   software, adding performance overhead
> > - gso packets which can't be parsed get dropped
> > 
> > Note that for zero copy, userspace can change the data after the
> > checksum has been calculated.  This is fundamental to zero copy by
> > design, and the only outcome is a bad checksum and a packet drop.
> > 
> > To mitigate the risks, the following patch will add a sysctl knob to
> > bypass the resolution. This is reasonable if the setup is such that
> > there is no protocol processing at all.
> > 
> > UDP tunnel headers get the same handling on the inner header.
> > 
> > Packets without VIRTIO_NET_HDR_F_NEEDS_CSUM are unaffected.
> > 
> > Reported-by: Paulos Yibelo <habte.yibelo@xxxxxxxxx>
> > Reported-by: Chris Arges <carges@xxxxxxxxxxxxxx>
> > Tested-by: Chris Arges <carges@xxxxxxxxxxxxxx>
> > Fixes: f43798c27684 ("tun: Allow GSO using virtio_net_hdr")
> > Suggested-by: Willem de Bruijn <willemdebruijn.kernel@xxxxxxxxx>
> > Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
> 
> A few comments inline. All but one cosmetic. Feel free to ignore
> those. Please do review the comment from an LLM about the tunnel.
> Aside from that LGTM.
> 

...

> > @@ -368,7 +637,20 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> >  	skb->inner_mac_header = inner_nh + skb_headroom(skb);
> >  	skb->transport_header = outer_th + skb_headroom(skb);
> >  	skb->encapsulation = 1;
> > -	return 0;
> > +
> > +	if (!skb_is_gso(skb))
> > +		skb_shinfo(skb)->gso_type &= ~(SKB_GSO_UDP_TUNNEL_CSUM |
> > +					       SKB_GSO_UDP_TUNNEL);
> > +
> > +	/*
> > +	 * We know VIRTIO_NET_HDR_F_NEEDS_CSUM was set so __virtio_net_hdr_to_skb
> > +	 * set CHECKSUM_PARTIAL.
> > +	 */
> > +	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > +						 inner_proto))
> > +		return 0;
> > +
> > +	return __virtio_net_resolve_csum_partial_mismatch(skb);
> 
>     Allowing `!skb_is_gso(skb)` packets through `virtio_net_hdr_tnl_to_skb()`
>     has a few issues:
> 
>     1. When `!skb_is_gso(skb)` and `virtio_net_csum_partial_ok_at_offset()`
>        returns false, `__virtio_net_resolve_csum_partial_mismatch()` completes
>        the checksum in software and lets the packet pass, but `skb->encapsulation`,
>        `skb->inner_protocol`, `skb->inner_transport_header`,
>        `skb->inner_network_header`, and `skb->inner_mac_header` have already
>        been populated with the unvalidated `inner_th` (`csum_start`) and
>        `inner_nh` offsets (unlike `virtio_net_hdr_to_skb()`, which restores
>        `skb->transport_header`).

Oh right. Hmm. I can just run the check first.


>     2. If `!skb_is_gso(skb)` and `VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM` is set,
>        clearing `SKB_GSO_UDP_TUNNEL_CSUM` drops the outer UDP checksum request,
>        since a non-GSO skb can only carry a single `CHECKSUM_PARTIAL` offset
>        (at `inner_th`).

Right. But it works! Because the checksum is 0 then. I'll ponder what is
best.


>     3. On RX, a non-GSO UDP packet with `CHECKSUM_PARTIAL` at `inner_th` causes
>        `skb_csum_unnecessary()` (`skb_checksum_start_offset(skb) >= 0`) to treat
>        the outer UDP checksum as already validated.
> 
>     Unlike plain GSO (commit 7c6d2ecbda83), UDP tunnel offload is modern-only and
>     explicitly requires GSO (`/* The UDP tunnel must carry a GSO packet, but no UFO. */`).
>     Why not reject `!skb_is_gso(skb)` right after `__virtio_net_hdr_to_skb()`, and
>     return `-EINVAL` directly if `!virtio_net_csum_partial_ok_at_offset()`?

This will break apps, I think. See 7c6d2ecbda83

-- 
MST