netdev - Re: [PATCH net-next 7/8] tun: enable gso over UDP tunnel support.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <68620cd9-923e-49df-ad39-482c3fa22be4@redhat.com>
Date: Mon, 26 May 2025 13:20:21 +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/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.

>> @@ -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 and 1) will require additional care when
adding hash report support.

>> 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.

Note that tun_vnet_parse_size() and  tun_vnet_tnl_offset() should be
adjusted accordingly after that HASH_REPORT support is introduced.

>> +                          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

/P