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

[Paolo Abeni <pabeni@redhat.com>]

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, as we will have 
later to do the same for inner header.

2) the 'thlen' as computed by the previous switch statement:

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

will be incorrect if we use csum_start to point the outer header, as 
VIRTIO_NET_HDR_GSO_{TCPV{4,6},UDP} refers to the inner header.

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.

4) the gso_size validation checks done under the if statement:

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

will be incorrect - we need to account for the inner

Basically all actions and checks performed by virtio_net_hdr_to_skb() 
will be wrong or irrelevant.

Instead, by using the virtio_net_hdr csum_start for the inner header, 
all the above mentioned checks are be correct and meaningful.

I wrote a longer reply, addressing every later point, but I fear it 
would distract from the main point using csum_offset for the outer is a 
poor and inconsistent choice.

Cheers,

Paolo

p.s. I think you got scared by the many checks that the tnl helpers 
implementation carry: such helpers are much more strict than the non tnl 
ones to avoid injecting into the stack packets with bad layout. A 
functional PoC could be implemented without most of them, but I think we 
actually want those additional checks no matter what layout we use for 
the virtio_net_hdr.