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

[Chris Arges]

On 2026-10-22 19:38:53, Michael S. Tsirkin wrote:
> 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 problem I saw used this case and setting inner_nh_offset incorrectly.
I'll dig more into this, and follow-up separately if this indeed is a
real 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
>     

Ok this is helpful, I can see why that doesn't work.
Thanks

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