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

[Willem de Bruijn]

Marcus Wichelmann wrote:
> Am 18.02.25 um 02:47 schrieb Willem de Bruijn:
> > Marcus Wichelmann wrote:
> >> [...]
> >> +	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.
 
> > 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.
> 
> >> +	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.