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