On Mon, Jun 16, 2025 at 6:17 PM Paolo Abeni [off-list ref] wrote:

On 6/16/25 6:53 AM, Jason Wang wrote:

quoted

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

quoted

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

quoted

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

I'm sorry, but I don't understand what is the suggestion/request here.
Could you please phrase it?

I meant I don't see strong reasons to duplicate tunnel gso/csum in tun->flags:

1) extra complexity
2) non tunnel gso doesn't do this (and for macvtap, it tries to
emulate netdev->features)
3) we can find way to disallow toggling tunnel gso/csum via ethtool
(but I don't see too much value)

Also please allow me to re-state my main point. The current
implementation adds a very limited amount of code in the control path,
and makes the data path simpler and faster - requiring no new argument
to the tun_hdr_* helper instead of (at least) one as the other alternative.

It looks like tun_hdr_* argument list could grow with every new feature,
but I think we should try to avoid that.

See above:

1) for HOST_UDP_XXX we can assume it is enabled
2) for GUEST_UDP_XXX we can check netdev->features

So passing netdev->features seems to be sufficient, it avoids
introducing new argument when a new offload is supported in the
future.

quoted

quoted

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.

AFAICS the text above matches/describes quite accurately the
implementation proposed in this patch and quoted above. Which in turn
confuses me, because I don't see what you would prefer to see
implemented differently.

I meant those can be done without using tun->flags.

quoted

quoted

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;

...

Note that the qemu code quoted above does not include tunnel handling.

TUN_VNET_TNL_SIZE (== sizeof(struct virtio_net_hdr_v1_tunnel)) will be
too small when VIRTIO_NET_F_HASH_REPORT is enabled, too.

I did not handle that case here, due to the even greater overlapping with:

https://lore.kernel.org/netdev/20250530-rss-v12-0-95d8b348de91@daynix.com/ (local)

What I intended to do is:
- set another bit in `flags` according to the negotiated
VIRTIO_NET_F_HASH_REPORT value
- use such info in tun_vnet_parse_size() to computed the expected vnet
hdr len correctly.
- replace TUN_VNET_TNL_SIZE usage in tun.c with tun_vnet_parse_size() calls

I'm unsure if the above answer your question/doubt.

For hash reporting since we don't have a netdev feature for that, a
new argument for tun_hdr_XXX() is probably needed for that

Anyhow I now see that keeping the UDP GSO related fields offset constant
regardless of VIRTIO_NET_F_HASH_REPORT would remove some ambiguity from
the relevant control path.

Not a native speaker but I think the ambiguity mainly come from the
"Only if" that is something like

"Only if VIRTIO_NET_F_HASH_REPORT negotiated"

I think/hope we are still on time to update the specification clarifying
that, but I'm hesitant to take that path due to the additional
(hopefully small) overhead for the data path - and process overhead TBH.

On the flip (positive) side such action will decouple more this series
from the HASH_REPORT support.

Please advice, thanks!

/P

Thanks