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

[Paolo Abeni <pabeni@redhat.com>]

On 10/17/24 08:47, Jason Wang wrote:
>> AFAICS the main argument against the layout I proposed is the
>> inconsistent usage of csum_start that will refer to the inner header for
>> GSO over udp-tunnel packets and to the outer header in when the device
>> does not expose the GSO over UDP tunnel support.
>>
>> I argue that dubbing such behavior as inconsistent is a matter of
>> interpretation: in both case the csum_start field refers to the
>> innermost header that the device is able to parse.
> 
> I was convinced by this argument until I see this
> 
> """
> 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.
> """
> 
> The "one level" seems to be ambiguous. But this reminds me that the
> innermost assumption seems to be impractical:
> 
> The checksum of non GSO tunnels has been supported.

I argue the above statement is again a matter of interpretation: the 
existing specs support checksum offload of UDP packets. That UDP packet 
may (or may not) carry a tunnel and some other headers, but such info 
are actually irrelevant to the device: lacking the GSO over UDP tunnel 
support, the device still parse the packet up to the (only known) 
transport (UDP) header and offload the related checksum.

Also note that I already agreed the specification for DATA_VALID with 
GSO over UDP tunnel packets must be changed/corrected and I proposed 2 
options to avoid confusion there:

https://lore.kernel.org/virtio-comment/CACGkMEvmhdYMHf5HKX37XqAYO2uHPq1=eWXfBPoS2gp8FJezDQ@mail.gmail.com/T/#m520efe2519200e878ab103f32598e5163cd01ec4

"""
1) forbid DATA_VALID with GSO over UDP tunnel

or

2) forbid DATA_VALID with GSO over UDP tunnel without UDP tunnel
checksum offload. That means that DATA_VALID for GSO over UDP tunnel
packets could only be set when both the inner and outher header are
offloaded.
"""

[...]
> Such inconsistency a lot of extra statements to describe the
> difference semantics in the spec and if/else in the code (as
> demonstrated in your POC which uses a completely different helper to
> do the header conversion between vnet header and skb metadata).

I think you have misread the PoC code. It actually re-uses the exiting 
helpers, verbatim, without the need to duplicate any existing checks.

Sure, there is some additional code in the GSO over UDP tunnel helpers, 
as the latter must process and check additional fields. The other 
additional conditionals you see are due to both the spec and helper 
being very strict about the admitted packets - we want to avoid the 
loopholes leading to nasty bugs, as already mentioned. Such code checks 
and spec statements should (/must) be there with any layout we chose for 
the virtio_net_hdr to support GSO over UDP tunnel.

Let me reiterate that, instead, using the csum_start/csum_offset fields 
to track the outer header information, the GSO over UDP tunnel could 
processing could not use 
virtio_net_hdr_to_skb()/virtio_net_hdr_from_skb() to process the 
existing fields. i.e. all the checks in

https://elixir.bootlin.com/linux/v6.12-rc2/source/include/linux/virtio_net.h#L97 
..L112

will be meaningless in such scenario and the GSO over UDP tunnel helper 
code must replicate them with the correct offsets.

IDK which extra statements in the spec you refers to, could you please 
be more specific?

> If we introduce new fields for inner, we can stick to the guidance and
> normatives for the new fields instead of dealing with the different
> semantics between GSO and non-GSO tunnels.
> 
> 1) a patch to clarify the existing fields in vnet header were for
> outer in there's a tunnel

Would such patch require the sort of extra statements you don't like?!?

> 2) adding new fields and explain that they were used for inner, we
> don't need to worry any possible conflict when trying to reuse fields
> with outer (as demonstrated by DATA_VALID).

I must admit the 6m (or 2y?) step backward in time does sound scaring to me.

As mentioned the above will lead to a poorer implementation, are we ok 
with that?

What about hdr_len field? for GSO over UDP packets should include the 
inner hdr len or not? If not, it would be useless, otherwise would be 
inconsistent.

/P