Re: [PATCH v8 0/2] virtio-net: define UDP tunnel offload

[Paolo Abeni <pabeni@redhat.com>]

On 10/2/24 14:29, Willem de Bruijn wrote:
> On Wed, Oct 2, 2024 at 4:35 AM Paolo Abeni <pabeni@redhat.com> wrote:
>>
>> On 10/1/24 21:15, Willem de Bruijn wrote:
>>> On Tue, Oct 1, 2024 at 10:02 AM Paolo Abeni <pabeni@redhat.com> wrote:
>>>> On 10/1/24 15:09, Willem de Bruijn wrote:
>>>>> On Mon, Sep 30, 2024 at 3:36 AM Paolo Abeni <pabeni@redhat.com> wrote:
>>>>>> On 9/9/24 10:05, Paolo Abeni wrote:
>>>>>> I'm wondering if we could revisit the spec change requiring that both
>>>>>> the driver and the device, for GSO over UDP tunnel packets only, will
>>>>>> fill the csum_start field when either the NEEDS_CSUM or the DATA_VALID
>>>>>> bits are set.
>>>>>
>>>>> This SGTM.
>>>>>
>>>>>> Equivalently, for GSO over UDP tunnel packets only, the
>>>>>> driver and the device, to specify CHECKSUM_UNNECESSARY packets, could
>>>>>> set both the NEEDS_CSUM and DATA_VALID, with the latter flag 'taking
>>>>>> precedence' WRT the actual checksum type.
>>>>>
>>>>> Why do both need to be set to signal CHECKSUM_UNNECESSARY, rather
>>>>> than only DATA_VALID?
>>>>
>>>> I think it's mostly a matter of preferences, and 'allowed delta' WRT to
>>>> the existing offload.
>>>>
>>>> AFAICS, technically the plain GSO spec don't mandate that NEEDS_CSUM and
>>>> DATA_VALID as mutually exclusive. Instead the spec require the
>>>> NEEDS_CSUM flag to use csum_* fields: the first option would create a
>>>> more significant difference WRT plain GSO.
>>>
>>> True. The existing API is very permissive.
>>>
>>> As you probably know, I'm strongly in favor of being more strict in
>>> new additions, to avoid allowing weird unexpected packets that we then
>>> have to detect and work around later.
>>>
>>>   From that PoV, explicitly specifying whether the bits are mutually
>>> exclusive or, if not, what having both sets means, would be great.
>>> Even if only for the new tunnel case, as we cannot do it
>>> retroactively.
>>
>> I must admit it's not clear to me if the above is a sort of endorsement
>> for both options, assuming each of them will be strict about the flags
>> definition.
>>
>> If even the 2nd option is ok, I would go with that, as
> 
> Here option 1 is "requiring that both the driver and the device, for
> GSO over UDP tunnel packets only, will fill the csum_start field when
> either the NEEDS_CSUM or the DATA_VALID bits are set."
> 
> And option 2 is "Equivalently, for GSO over UDP tunnel packets only,
> the driver and the device, to specify CHECKSUM_UNNECESSARY packets,
> could set both the NEEDS_CSUM and DATA_VALID, with the latter flag
> 'taking precedence' WRT the actual checksum type."
> 
> Right?

Yes!

> Option 1 intuitively is simpler to follow: for tunnel offload,
> csum_start must always be set.
> 
>> it sounds preferable to me for the mentioned reasons.
> 
> It's a bit unfortunate for implementation to dictate interface,
> especially if it makes it less straightforward. But if the
> implementation is significantly simpler with option 2, no strong
> objections from me. This is not a huge design point, either way.

Arguably the most relevant sell-point for option 2 is that it allows 
enforcing simpler/stricter checks in virtio net hdr parsing (if 
UDP_TUNNEL is set, NEEDS_CSUM and plain gso must be set, too).

Somewhat related: I just noted that the 'TX sides' of the spec (both in 
device->driver and in the driver -> device direction) is already strict 
WRT csum and gso dependency, but the corresponding 'RX sides' 
({device,driver}normative/Processing of Incoming Packets) lack 
completely any MUST NOT accept/MUST drop statement for bad/invalid inputs.

My understanding is that most/all of the trouble we have on virtio net 
hdr parsing and validation steam from such lack. I'm adding explicit 
'MUST NOT accept' statements for all the UDP-tunnel-related invalid 
flags/gso_type permutation I can think of.

@Jason: do you know if there is a specific reason for the mentioned lack 
  validation in the current specs?

Cheers,

Paolo