Paolo Abeni wrote:

On 6/20/25 4:40 PM, Willem de Bruijn wrote:

quoted

Paolo Abeni wrote:

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 };
+	struct virtio_net_hdr_v1_hash_tunnel hdr;

Not for this series.

But one day virtio will need a policy on how multiple optional
features can be composed, and simple APIs to get to those optional
headers. Perhaps something like skb extensions.

Now, each new extention means adding yet another struct and updating
all sites that access it.

A minimal rule may be that options can be entirely independent, but
if they exist at least their headers are always in a fixed order.
Which is already implied by the current extensions, i.e., hash comes
before tunnel if present.

If I read correctly, you are suggesting that negotiating tunnel and not
hash would yield this layout:

< basic vnet hdr> <tnl fields>

with no gaps/data between the basic header fields and the tunnel-related
one. Am I correct?

This has been discussed in the previous revisions, and a recent
specification update explicitly states differently: with tunnel support
and without hash report the only possible layout is:

< basic vnet hdr> <hash report field (unused)> <tnl fields>

Indeed. Long-term it seems odd to just keep extending the header with
every optional feature, even when disabled.

Since it's in the spec it's too late to change it, unless we add yet
another feature for that. I'm gladly leaving that joy and fun to someone
else:)

Agreed!

FTR the initial revisions of this series, before I stumbled upon the
mentioned spec change, followed the schema you mentioned.

quoted

quoted

@@ -1721,7 +1733,12 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
 	if (tun->flags & IFF_VNET_HDR) {
 		int vnet_hdr_sz = READ_ONCE(tun->vnet_hdr_sz);
 
-		hdr_len = tun_vnet_hdr_get(vnet_hdr_sz, tun->flags, from, &gso);
+		if (vnet_hdr_sz >= TUN_VNET_TNL_SIZE)
+			features = NETIF_F_GSO_UDP_TUNNEL |
+				   NETIF_F_GSO_UDP_TUNNEL_CSUM;

Maybe a helper virtio_net_has_opt_tunnel(), to encapsulate whatever
conditions have to be met. As those conditions are not obvious.

Especially if needed in multiple locations. Not sure if that is the
case here, I have not checked that.

Yep, as an outcome of Ajihiko's review I'm encapsulation the above in
a new helper - tun_vnet_hdr_guest_features() to be more generic.

Saw that. Awesome.
 

quoted

quoted

@@ -2812,6 +2849,8 @@ static void tun_get_iff(struct tun_struct *tun, struct ifreq *ifr)
 
 }
 
+#define PLAIN_GSO (NETIF_F_GSO_UDP_L4 | NETIF_F_TSO | NETIF_F_TSO6)
+

Minor/subjective: prefer const unsigned int at function scope over untyped
file scope macros.

Unless it's blocking I would keep the current code here.

Ack.

quoted

quoted

+static inline int
+tun_vnet_hdr_tnl_to_skb(unsigned int flags, netdev_features_t features,
+			struct sk_buff *skb,
+			const struct virtio_net_hdr_v1_hash_tunnel *hdr)
+{
+	return virtio_net_hdr_tnl_to_skb(skb, hdr,
+					 !!(features & NETIF_F_GSO_UDP_TUNNEL),
+					 !!(features & NETIF_F_GSO_UDP_TUNNEL_CSUM),

Double exclamation points not needed. Compiler does the right thing
when arguments are of type bool.

Will drop in the next revision.

Thanks!

Paolo