Message-ID: <82b00219-73e8-4330-99b4-3a0a2fe86a50@redhat.com>
Date: Thu, 29 May 2025 18:17:27 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jason Wang <jasowang@...hat.com>
Cc: netdev@...r.kernel.org, Willem de Bruijn
 <willemdebruijn.kernel@...il.com>, Andrew Lunn <andrew+netdev@...n.ch>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, "Michael S. Tsirkin" <mst@...hat.com>,
 Xuan Zhuo <xuanzhuo@...ux.alibaba.com>, Eugenio Pérez
 <eperezma@...hat.com>
Subject: Re: [PATCH net-next 7/8] tun: enable gso over UDP tunnel support.

On 5/27/25 6:19 AM, Jason Wang wrote:
> On Mon, May 26, 2025 at 7:20 PM Paolo Abeni <pabeni@...hat.com> wrote:
>> On 5/26/25 6:40 AM, Jason Wang wrote:
>>> On Wed, May 21, 2025 at 6:34 PM Paolo Abeni <pabeni@...hat.com> wrote:
>>>>
>>>> Add new tun features to represent the newly introduced virtio
>>>> GSO over UDP tunnel offload. Allows detection and selection of
>>>> such features via the existing TUNSETOFFLOAD ioctl, store the
>>>> tunnel offload configuration in the highest bit of the tun flags
>>>> and compute the expected virtio header size and tunnel header
>>>> offset using such bits, so that we can plug almost seamless the
>>>> the newly introduced virtio helpers to serialize the extended
>>>> virtio header.
>>>>
>>>> As the tun features and the virtio hdr size are configured
>>>> separately, the data path need to cope with (hopefully transient)
>>>> inconsistent values.
>>>
>>> I'm not sure it's a good idea to deal with this inconsistency in this
>>> series as it is not specific to tunnel offloading. It could be a
>>> dependency for this patch or we can leave it for the future and just
>>> to make sure mis-configuration won't cause any kernel issues.
>>
>> The possible inconsistency is not due to a misconfiguration, but to the
>> facts that:
>> - configuring the virtio hdr len and the offload is not atomic
>> - successful GSO over udp tunnel parsing requires the relevant offloads
>> to be enabled and a suitable hdr len.
>>
>> Plain GSO don't have a similar problem because all the relevant fields
>> are always available for any sane virtio hdr length, but we need to deal
>> with them here.
> 
> Just to make sure we're on the same page.
> 
> I meant tun has TUNSETVNETHDRSZ, so user space can set it to any value
> at any time as long as it's not smaller than sizeof(struct
> virtio_net_hdr). Tun and vhost need to cope with this otherwise it
> should be a bug. This is allowed before the introduction of tunnel
> gso.

This code here is intended to support such scenario; but if the virtio
hdr size is configured to be lower than the minimum required for UDP
tunnel hdr fields, the related offload could not be used.

>>>> @@ -1698,7 +1700,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
>>>>         struct sk_buff *skb;
>>>>         size_t total_len = iov_iter_count(from);
>>>>         size_t len = total_len, align = tun->align, linear;
>>>> -       struct virtio_net_hdr gso = { 0 };
>>>> +       char buf[TUN_VNET_TNL_SIZE];
>>>
>>> I wonder why not simply
>>>
>>> 1) define the structure virtio_net_hdr_tnl_gso and use that
>>>
>>> or
>>>
>>> 2) stick the gso here and use iter advance to get
>>> virtio_net_hdr_tunnel when necessary?
>>
>> Code wise 2) looks more complex
> 
> I don't know how to define complex but we've already use a conatiner structure:
> 
> struct virtio_net_hdr_v1_hash {
>         struct virtio_net_hdr_v1 hdr;
>         __le32 hash_value;
> ...
>         __le16 hash_report;
>         __le16 padding;
> };
> 
>> and 1) will require additional care when
>> adding hash report support.
> 
> I don't understand here, you're doing:
> 
>         iov_iter_advance(from, sz - parsed_size);
> 
> in __tun_vnet_hdr_get(), so this logic needs to be extended for hash
> report as well.

Note that there are at least 2 different virtio net hdr binary layout
supporting UDP tunnel offload:

struct virtio_net_hdr_v1_tnl {
   struct virtio_net_hdr_v1 hdr;
   struct virtio_net_hdr_tunnel tnl;
};

and

struct virtio_net_hdr_v1_hash_tnl {
   struct virtio_net_hdr_v1_hash hdr;
   struct virtio_net_hdr_tunnel tnl;
};

depending on the negotiated features. Using directly a struct to
fill/fetch the tunnel fields is problematic.

With the current approach the binary layout differences are abstracted
by the tun_vnet_parse_size()/tun_vnet_tnl_offset() helpers. The
expectation is that enabling hash report will set a bit in `flags`, too,
 so that helpers could compute the correct offset accordingly.

No other change should be required.

>>>> diff --git a/drivers/net/tun_vnet.h b/drivers/net/tun_vnet.h
>>>> index 58b9ac7a5fc40..ab2d4396941ca 100644
>>>> --- a/drivers/net/tun_vnet.h
>>>> +++ b/drivers/net/tun_vnet.h
>>>> @@ -5,6 +5,12 @@
>>>>  /* High bits in flags field are unused. */
>>>>  #define TUN_VNET_LE     0x80000000
>>>>  #define TUN_VNET_BE     0x40000000
>>>> +#define TUN_VNET_TNL           0x20000000
>>>> +#define TUN_VNET_TNL_CSUM      0x10000000
>>>> +#define TUN_VNET_TNL_MASK      (TUN_VNET_TNL | TUN_VNET_TNL_CSUM)
>>>> +
>>>> +#define TUN_VNET_TNL_SIZE (sizeof(struct virtio_net_hdr_v1) + \
>>>
>>> Should this be virtio_net_hdr_v1_hash?
>>
>> If tun does not support HASH_REPORT, no: the GSO over UDP tunnels header
>> could be present regardless of the hash-related field presence. This has
>> been discussed extensively while crafting the specification.
> 
> Ok, so it excludes the hash report fields, more below.
> 
>>
>> Note that tun_vnet_parse_size() and  tun_vnet_tnl_offset() should be
>> adjusted accordingly after that HASH_REPORT support is introduced.
> 
> This is suboptimal as we know a hash report will be added so we can
> treat the field as anonymous one. See
> 
> https://patchwork.kernel.org/project/linux-kselftest/patch/20250307-rss-v9-3-df76624025eb@daynix.com/

I know hash support is in the work. The current design is intended to
minimize the conflicts with such feature. But I can't follow the
statement above. Could you please re-phrase it?

>>>> +                          sizeof(struct virtio_net_hdr_tunnel))
>>>>
>>>>  static inline bool tun_vnet_legacy_is_little_endian(unsigned int flags)
>>>>  {
>>>> @@ -45,6 +51,13 @@ static inline long tun_set_vnet_be(unsigned int *flags, int __user *argp)
>>>>         return 0;
>>>>  }
>>>>
>>>> +static inline void tun_set_vnet_tnl(unsigned int *flags, bool tnl, bool tnl_csum)
>>>> +{
>>>> +       *flags = (*flags & ~TUN_VNET_TNL_MASK) |
>>>> +                tnl * TUN_VNET_TNL |
>>>> +                tnl_csum * TUN_VNET_TNL_CSUM;
>>>
>>> We could refer to netdev via tun_struct, so I don't understand why we
>>> need to duplicate the features in tun->flags (we don't do that for
>>> other GSO/CSUM stuffs).
>>
>> Just to be consistent with commit 60df67b94804b1adca74854db502a72f7aeaa125
> 
> I don't see a connection here, the above commit just moves decouple
> vnet to make it reusable, it doesn't change the semantic of
> tun->flags.

You are right, I used a bad commit reference.

The goal here is to keep all the virtio-layout-related information in a
single place. tun->flags is already used for that (for little endian
flag), so I piggybacked there.

Ideally another bit there will be allocated used to mark the hash report
presence, too. That will allow the tun_vnet helpers to determine the
virtio net hdr layout using a single argument.

Note that we can't relay on the netdev->features to determine the virtio
net hdr binary layout because user-space could enable/disable GSO over
UDP tunnel support after ioctl(TUNSETOFFLOAD).

/P

Powered by blists - more mailing lists