Message-ID: <CACGkMEuRrGyCi-kxjTgGpyCdcp=yVHCS_qDFEmvfeqxzhfoJwA@mail.gmail.com>
Date: Tue, 3 Jun 2025 10:11:32 +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 Fri, May 30, 2025 at 12:18 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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.

Ok I see.

>
> >>>> @@ -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;
> };

Is this used by any guest? It looks problematic:

\begin{lstlisting}
struct virtio_net_hdr {
#define VIRTIO_NET_HDR_F_NEEDS_CSUM    1
#define VIRTIO_NET_HDR_F_DATA_VALID    2
#define VIRTIO_NET_HDR_F_RSC_INFO      4
#define VIRTIO_NET_HDR_F_UDP_TUNNEL_CSUM 8
        u8 flags;
#define VIRTIO_NET_HDR_GSO_NONE        0
#define VIRTIO_NET_HDR_GSO_TCPV4       1
#define VIRTIO_NET_HDR_GSO_UDP         3
#define VIRTIO_NET_HDR_GSO_TCPV6       4
#define VIRTIO_NET_HDR_GSO_UDP_L4      5
#define VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV4 0x20
#define VIRTIO_NET_HDR_GSO_UDP_TUNNEL_IPV6 0x40
#define VIRTIO_NET_HDR_GSO_ECN      0x80
u8 gso_type;
        le16 hdr_len;
        le16 gso_size;
        le16 csum_start;
        le16 csum_offset;
        le16 num_buffers;
        le32 hash_value;        (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
        le16 hash_report;       (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
        le16 padding_reserved;  (Only if VIRTIO_NET_F_HASH_REPORT negotiated)
        le16 outer_th_offset    (Only if
VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO or VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO
negotiated)
        le16 inner_nh_offset;   (Only if
VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO or VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO
negotiated)
};
\end{lstlisting}

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

I'm not sure what's the problem here, we can just skip the hash part
and it would be easier for the hash reporting feature.

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

See above, if I was not wrong, virtio_net_hdr_v1_hash_tnl should be
sufficient for both tunnel offloading and hash reporting.

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

Note that TUNSET/GETVNETLE stuff is not what virtio should know.

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

I'm not sure I got here, it works for non GSO offload, anything makes
UDP tunnel different here?

Thanks

>
> /P
>
>
>

Powered by blists - more mailing lists