Re: [PATCH bpf-next v2 2/6] net: tun: enable transfer of XDP metadata to skb

Marcus Wichelmann wrote:

Am 18.02.25 um 02:47 schrieb Willem de Bruijn:

quoted

Marcus Wichelmann wrote:

quoted

[...]
+	metasize = max(xdp->data - xdp->data_meta, 0);

Can xdp->data_meta ever be greater than xdp->data?

When an xdp_buff has no metadata support, then this is marked by setting
xdp->data_meta to xdp->data + 1. See xdp_prepare_buff or
xdp_set_data_meta_invalid.

In the case of tun_xdp_one, the xdp_buff is externally created by another
driver and passed to the tun driver using sendmsg and TUN_MSG_PTR. For
now, the vhost_net driver is the only driver doing that, and
xdp->data_meta is set to xdp->data there, marking support for metadata.

So knowing that vhost_net is currently the only driver passing xdp_buffs
to tun_sendmsg, the check is not strictly necessary. But other drivers
may use this API as well in the future. That's why I'd like to not make
the assumption that other drivers always create the xdp_buffs with
metadata support, when they pass them to tun_sendmsg.

Or am I just to careful about this? What do you think?

I agree.
 

quoted

This is pointer comparison, which is tricky wrt type. It likely is
ptrdiff_t and thus signed. But may want to use max_t(long int, ..) to
make this explicit.

Ah, I see, good point.

So like that?

	metasize = max_t(long int, xdp->data - xdp->data_meta, 0);
	if (metasize)
		skb_metadata_set(skb, metasize);

Or just this? Also ensures the test uses signed int.

    int metasize;

    ...


    metasize = xdp->data - xdp->data_meta;
    if (metasize > 0)
            skb_metadata_set(skb, metasize);

Alternatively, there is also xdp_data_meta_unsupported(xdp_buff) which
could be used to make this check very explicit, but I don't see it being
used in network drivers elsewhere. Not sure why.

quoted

quoted

+	if (metasize)
+		skb_metadata_set(skb, metasize);
+

Not strictly needed. As skb_metadata_clear is just
skb_metadata_set(skb, 0). But also not wrong, so fine to keep.

Oh, haven't seen that.
I'm following a common pattern here that I've seen in many other network
drivers (grep for "skb_metadata_set"):

	unsigned int metasize = xdp->data - xdp->data_meta;
	[...]
	if (metasize)
		skb_metadata_set(skb, metasize);

Thanks for that context. Sounds good.