Re: [PATCH v9 1/2] virtio-net: define UDP tunnel segmentation offload feature

[Paolo Abeni <pabeni@redhat.com>]

On 10/22/24 09:42, Jason Wang wrote:
> So my point is considering we are designing virtio instead of trying
> to make existing GSO capable devices to support tunnel GSO (or can we
> do that for virtio?). If some new fields can make things easier, we
> probably need to go that way.
> 
> Let me summarize my understanding for your proposal so far:
> 
> 1) csum_start/csum_offset were for the inner and so the outer checksum
> can be done through LCO or offload via patch 2
> 2) if VIRTIO_NET_HDR_GSO_UDP_TUNNEL_{IPV4|IPV6} is set the rest bits
> of the gso_type were for inner
> 3) hdr_len is for inner (this needs to be clarified in the spec), so
> outer hdr_len could be deduced from inner
> 
> It should work but for 1) I worry about the possible inconsistency:
> 
> 1) It might have some conflict with
> 
> "
> If the VIRTIO_NET_F_GUEST_CSUM feature was negotiated, the
> VIRTIO_NET_HDR_F_DATA_VALID bit in flags can be set: if so, device has
> validated the packet checksum. In case of multiple encapsulated
> protocols, one level of checksums has been validated.
> "

I proposed the following:

For GSO over UDP tunnel VIRTIO_NET_HDR_F_DATA_VALID is allowed if and
only even  VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM is set in flags.

That means that the device supports csum offload for both the inner and
the outer and both csums are valid. The corresponding skb will have
CHECKSUM_UNNECESSARY and csum_level == 1.

For GSO over UDP tunnel VIRTIO_NET_HDR_F_DATA_VALID with a single csum
offloaded, VIRTIO_NET_HDR_F_DATA_VALID can't be used.

Note that we may want to avoid completely VIRTIO_NET_HDR_F_DATA_VALID
for GSO over UDP tunnel packets, see below[1]. Also that option would
avoid all the conflicts.

> 2) csum_start/csum_offset were for outer for non GSO tunnel packets.
> hdr_len, gso_type are fine.
> 
> I think those can be addressed for sure with more statements in the
> spec. Just not sure if it's worth it. I'm fine if you stick to that or
> we can hear from others.

I'm unsure I read this point correctly. A rephrase could help.

> Down this road, I have other questions:
> 
> 1) We need to clarify if there's an N level nesting tunnel, is
> csum_start/offset/hdr_len for the innermost or not. Linux seems to use
> the innermost semantic. If yes, for DATA_VALID, as Willem mentioned,
> we need a way to report csum_level.

If there are multiple nested tunnels, there will be no GSO over UDP
tunnels packets. The existing specs will apply, only one csum will be
offloaded.

> 2) In your POC[1], hdr->csum_start/offset seems still point to the outer header

That would be a bug...

> https://github.com/pabeni/linux-devel/blob/virtio_tunnel_gso/include/linux/virtio_net.h#L229

... but the above code shows that hdr->csum_start points to the inner,
because skb->csum_start/skb->csum_offset refers to the inner header in
Linux.

There is actually another point which I'm quite scared to mention
because it caused v9 and implicitly all this discussion.

I want to prevent the driver receiving from the device GSO over UDP
tunnel packets without csum_start/csum_offset, because the header
probing code looks fragile and bug-prone and will be even more complex
in case of tunnels.

[1] AFAICS the easiest way to prevent such thing is explicitly requiring
no DATA_VALID for GSO over UDP tunnel packets.

Cheers,

Paolo