Message-ID: <CACGkMEsuRSOY3xe9=9ONMM3ZBGdyz=5cbTZ0sUp38cYrgtE07w@mail.gmail.com>
Date: Mon, 16 Jun 2025 12:53:59 +0800
From: Jason Wang <jasowang@...hat.com>
To: Paolo Abeni <pabeni@...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 Thu, Jun 12, 2025 at 7:03 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
> 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.

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

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

Powered by blists - more mailing lists