On Fri, May 30, 2025 at 12:18 AM Paolo Abeni [off-list ref] wrote:

On 5/27/25 6:19 AM, Jason Wang wrote:

quoted

On Mon, May 26, 2025 at 7:20 PM Paolo Abeni [off-list ref] wrote:

quoted

On 5/26/25 6:40 AM, Jason Wang wrote:

quoted

On Wed, May 21, 2025 at 6:34 PM Paolo Abeni [off-list ref] wrote:

quoted

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.

quoted

quoted

quoted

quoted

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

quoted

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.

quoted

quoted

quoted

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

+                          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