Message-ID: <b7099b02-f0d4-492c-a15b-2a93a097d3f5@redhat.com>
Date: Thu, 12 Jun 2025 13:03:16 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jason Wang <jasowang@...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>, Yuri Benditovich <yuri.benditovich@...nix.com>,
 Akihiko Odaki <akihiko.odaki@...nix.com>
Subject: Re: [PATCH RFC v3 7/8] tun: enable gso over UDP tunnel support.

On 6/12/25 6:55 AM, Jason Wang wrote:
> On Fri, Jun 6, 2025 at 7:46 PM Paolo Abeni <pabeni@...hat.com> wrote:
>> @@ -1720,8 +1732,16 @@ 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);
>> +               int parsed_size;
>>
>> -               hdr_len = tun_vnet_hdr_get(vnet_hdr_sz, tun->flags, from, &gso);
>> +               if (vnet_hdr_sz < TUN_VNET_TNL_SIZE) {
> 
> I still don't understand why we need to duplicate netdev features in
> flags, and it seems to introduce unnecessary complexities. Can we
> simply check dev->features instead?
> 
> I think I've asked before, for example, we don't duplicate gso and
> csum for non tunnel packets.

My fear was that if
- the guest negotiated VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO
- tun stores the negotiated offload info netdev->features
- the tun netdev UDP tunnel feature is disabled via ethtool

tun may end-up sending to the guest packets without filling the tnl hdr,
which should be safe, as the driver should not use such info as no GSO
over UDP packets will go through, but is technically against the
specification.

The current implementation always zero the whole virtio net hdr space,
so there is no such an issue.

Still the additional complexity is ~5 lines and makes all the needed
information available on a single int, which is quite nice performance
wise. Do you have strong feeling against it?

>> @@ -2426,7 +2460,16 @@ static int tun_xdp_one(struct tun_struct *tun,
>>         if (metasize > 0)
>>                 skb_metadata_set(skb, metasize);
>>
>> -       if (tun_vnet_hdr_to_skb(tun->flags, skb, gso)) {
>> +       /* Assume tun offloads are enabled if the provided hdr is large
>> +        * enough.
>> +        */
>> +       if (READ_ONCE(tun->vnet_hdr_sz) >= TUN_VNET_TNL_SIZE &&
>> +           xdp->data - xdp->data_hard_start >= TUN_VNET_TNL_SIZE)
>> +               flags = tun->flags | TUN_VNET_TNL_MASK;
>> +       else
>> +               flags = tun->flags & ~TUN_VNET_TNL_MASK;
> 
> I'm not sure I get the point that we need dynamics of
> TUN_VNET_TNL_MASK here. We know if tunnel gso and its csum or enabled
> or not,

How does tun know about VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO or
VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO_CSUM?

The user-space does not tell the tun device about any of the host
offload features. Plain/baremetal GSO information are always available
in the basic virtio net header, so there is no size check, but the
overall behavior is similar - tun assumes the features have been
negotiated if the relevant bits are present in the header.

Here before checking the relevant bit we ensures we have enough vitio
net hdr data - that makes the follow-up test simpler.

> and we know the vnet_hdr_sz here, we can simply drop the
> packet with less header.

That looks prone migration or connectivity issue, and different from the
current general behavior of always accepting any well formed packet even
if shorter than what is actually negotiated (i.e. tun accepts packets
with just virtio_net_hdr header even when V1 has been negotiated).

/P

Powered by blists - more mailing lists