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

[Paolo Abeni <pabeni@redhat.com>]

On 10/21/24 08:54, Jason Wang wrote:
> On Fri, Oct 18, 2024 at 6:10 PM Paolo Abeni <pabeni@redhat.com> wrote:
>> On 10/18/24 06:26, Jason Wang wrote:
>>> On Thu, Oct 17, 2024 at 11:34 PM Paolo Abeni <pabeni@redhat.com> wrote:
>>>> 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.
>>>
>>> I don't understand here. It works like a charm for non GSO UDP tunnel,
>>> anything that makes it can't work for GSO?
>>
>> I we chose to use csum_start for the outer header, for GSO over UDP
>> tunnel packets:
>>
>> 1) the 'all headers in linear part' check:
>>
>> https://elixir.bootlin.com/linux/v6.12-rc3/source/include/linux/virtio_net.h#L111
>>
>> and all the need preeceeding algebra will be useless,
> 
> This part I don't understand. We still need a validation for the outer
> header and we've already done this for non GSO tunnels already.

For GSO over UDP tunnel packet, what we need to ensure is that all the
headers, comprising the inner ones, are available.

Checking that the outer header are there does not avoid the need for
checking that the inner headers are present.

Checking the inners only is instead sufficient: if the inner headers are
available any preceeding data will also be available, including the
outer headers.

>> 3) the csum_start/csum_offset assignment performed by:
>>
>> https://elixir.bootlin.com/linux/v6.12-rc3/source/include/linux/virtio_net.h#L104
>>
>> will yeld the wrong values - as they should point to the inner header
>> for GSO over UDP tunnel packets.
> 
> See above, it just works as a non GSO UDP tunnel.

Note that all this topic is about GSO over UDP tunnel packets, and which
layout is more consistent for them.

For GSO over UDP tunnel packets, if we set virtio_net_header csum_start
to the outer, we can't reuse a single line of the existing code, as the
tests will be unnecessary or will give the wrong results.

i.e.

-
https://elixir.bootlin.com/linux/v6.12-rc3/source/include/linux/virtio_net.h#L171

would flag as invalid good  packets, as it would over-estimante the
packet payload size

-
https://elixir.bootlin.com/linux/v6.12-rc3/source/include/linux/virtio_net.h#L179

would drop every TCP over UDP tunnel packet, as csum_offset will be 6
even when the inner header is TCP.

Instead if for GSO over UDP tunnel we set csum_start to the inner
header, all the existing test will yield valid result - mostly because
the Linux kernel, for such packets, expect csum_start pointing the inner
header.

And that is IMHO a clear sign that for such packet csum_offset should
point to the inner header even for virtio_net_hdr.

Cheers,

Paolo