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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

Paolo Abeni wrote:
> On 6/20/25 4:40 PM, Willem de Bruijn wrote:
> > Paolo Abeni wrote:
> >> @@ -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.
> 
> >> @@ -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.
 
> >> @@ -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.

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