Re: [PATCH net v5 4/7] net: resolve uncommon partial checksums at ingress

[Michael S. Tsirkin]

On Thu, Oct 22, 2026 at 12:52:36PM -0500, Chris Arges wrote:
> > > @@ -335,9 +592,12 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >  	/* Let the basic parsing deal with plain GSO features. */
> > >  	ret = __virtio_net_hdr_to_skb(skb, hdr, true,
> > >  				      hdr->gso_type & ~gso_tunnel_type);
> > > -	if (ret)
> > > +	if (ret < 0)
> > >  		return ret;
> > >  
> 
> One other case I found in testing is that a producer can set gso_size
> larger than the remaining packet data. __virtio_net_hdr_to_skb() then
> succeeds, but intentionally does not install GSO state because the packet
> is too small to segment. NEEDS_CSUM has still left the skb with
> CHECKSUM_PARTIAL.

So why is this a problem? If checksum offset is correct we leave
packet as CHECKSUM_PARTIAL. That's intentional.
What does your test do that makes this a problem?

> The following check fixes my test case:
> 
> 	if (!virtio_net_hdr_validate_disabled(skb) &&
> 	    !skb_is_gso(skb))
> 		return -EINVAL;
> 
> A concern I have is this may reject valid packets, so maybe there's a better
> place for this.


Indeed, it's not uncommon to have small packets with !gso.

See also:
commit 7c6d2ecbda83150b2036a2b36b21381ad4667762
    net: be more gentle about silly gso requests coming from user
    


> 
> > > +	if (!virtio_net_tnl_offsets_ok(skb, outer_isv6, outer_th))
> > > +		return -EINVAL;
> > > +
> > >  	/* In case of USO, the inner protocol is still unknown and
> > >  	 * `inner_isv6` is just a guess, additional parsing is needed.
> > >  	 * The previous validation ensures that accessing an ipv4 inner
> > > @@ -353,7 +613,7 @@ virtio_net_hdr_tnl_to_skb(struct sk_buff *skb,
> > >  	}
> > >  
> > >  	skb_set_inner_protocol(skb, inner_isv6 ? htons(ETH_P_IPV6) :
> > > -						 htons(ETH_P_IP));
> > > +			    htons(ETH_P_IP));
> > >  	if (hdr->flags & VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM) {
> > >  		if (!tnl_csum_negotiated)
> > >  			return -EINVAL;
> > > @@ -368,7 +628,22 @@ 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.
> > > +	 */
> > > +	inner_proto = inner_isv6 ? htons(ETH_P_IPV6) : htons(ETH_P_IP);
> > 
> > on second thought, if having to respin, might as well fold the trivial
> > fix in patch 7 in here from the start.
> > 
> > > +	if (virtio_net_csum_partial_ok_at_offset(skb, gso_inner_type, inner_nh,
> > > +						 inner_proto))
> > > +		return 0;
> > > +
> > > +	/* A tunnel header implies GSO, so drop. */
> > 
> > no longer true for the above !skb_is_gso(skb) case
> >
> 
> virtio_net_csum_partial_ok_at_offset()

Sorry I don't get what you are saying.

>  
> > > +	return __virtio_net_resolve_csum_partial_mismatch(skb);
> > >  }
> > >  
> > >  /* Checksum-related fields validation for the driver */
> > > -- 
> > > MST
> > > 
> > 
> >