On Thu, Jun 12, 2025 at 7:03 PM Paolo Abeni [off-list ref] wrote:

On 6/12/25 6:55 AM, Jason Wang wrote:

quoted

On Fri, Jun 6, 2025 at 7:46 PM Paolo Abeni [off-list ref] wrote:

quoted

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

Probably not? For example this is the way tun works with non tunnel GSO as well.

(And it allows the flexibility of debugging etc).

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?

See above and at least we can disallow the changing of UDP tunnel GSO
(but I don't see too much value).

quoted

quoted

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

I think it can be done in a way that works for non-tunnel gso.

The most complicated case is probably the case HOST_UDP_TUNNEL_X is
enabled but GUEST_UDP_TUNNEL_X is not. In this case tun can know this
by:

1) vnet_hdr_len is large enough
2) UDP tunnel GSO is not enabled in netdev->features

If HOST_UDP_TUNNEL_X is not enabled by GUEST_UDP_TUNNEL_X is enabled,
it can behave like existing non-tunnel GSO: still accept the UDP GSO
tunnel packet.

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.

I'm not sure I understand here, there's no bit in the virtio net
header that tells us if the packet contains the tunnel gso field. And
the check of:

READ_ONCE(tun->vnet_hdr_sz) >= TUN_VNET_TNL_SIZE

seems to be not buggy. As qemu already did:

static void virtio_net_set_mrg_rx_bufs(VirtIONet *n, int mergeable_rx_bufs,
                                       int version_1, int hash_report)
{
    int i;
    NetClientState *nc;

    n->mergeable_rx_bufs = mergeable_rx_bufs;

    if (version_1) {
        n->guest_hdr_len = hash_report ?
            sizeof(struct virtio_net_hdr_v1_hash) :
            sizeof(struct virtio_net_hdr_mrg_rxbuf);
        n->rss_data.populate_hash = !!hash_report;

...

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

quoted

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

Tun doesn't know V1, it can only see vnet_hdr_len, it is the userspace
(Qemu) that needs to configure all parties correctly. So I meant if we
get a XDP buff with less header, it should be more like a userspace
(Qemu) but. I'm not sure we need to workaround it by kernel.

Thanks

/P