Message-ID: <CACGkMEvpr1cqh2CaA6rP03T-dqzKcqkKV6cq+zCfCgAew=+CRw@mail.gmail.com>
Date: Tue, 27 May 2025 12:19:35 +0800
From: Jason Wang <jasowang@...hat.com>
To: Paolo Abeni <pabeni@...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 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.

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

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

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

Thanks

>
> /P
>

Powered by blists - more mailing lists