[xdp-hints] Re: [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs

["Toke Høiland-Jørgensen" <toke@redhat.com>]

Stanislav Fomichev <sdf@google.com> writes:

> There is an ndo handler per kfunc, the verifier replaces a call to the
> generic kfunc with a call to the per-device one.
>
> For XDP, we define a new kfunc set (xdp_metadata_kfunc_ids) which
> implements all possible metatada kfuncs. Not all devices have to
> implement them. If kfunc is not supported by the target device,
> the default implementation is called instead.

So one unfortunate consequence of this "fallback to the default
implementation" is that it's really easy to get a step wrong and end up
with something that doesn't work. Specifically, if you load an XDP
program that calls the metadata kfuncs, but don't set the ifindex and
flag on load, the kfunc resolution will work just fine, but you'll end
up calling the default kfunc implementations (and get no data). I ran
into this multiple times just now when playing around with it and
implementing the freplace support.

So I really think it would be a better user experience if we completely
block (with a nice error message!) the calling of the metadata kfuncs if
the program is not device-bound...

Another UX thing I ran into is that libbpf will bail out if it can't
find the kfunc in the kernel vmlinux, even if the code calling the
function is behind an always-false if statement (which would be
eliminated as dead code from the verifier). This makes it a bit hard to
conditionally use them. Should libbpf just allow the load without
performing the relocation (and let the verifier worry about it), or
should we have a bpf_core_kfunc_exists() macro to use for checking?
Maybe both?

> Upon loading, if BPF_F_XDP_HAS_METADATA is passed via prog_flags,
> we treat prog_index as target device for kfunc resolution.

[...]

> -	if (!bpf_prog_map_compatible(map, prog)) {
> -		bpf_prog_put(prog);
> -		return ERR_PTR(-EINVAL);
> -	}
> +	/* When tail-calling from a non-dev-bound program to a dev-bound one,
> +	 * XDP metadata helpers should be disabled. Until it's implemented,
> +	 * prohibit adding dev-bound programs to tail-call maps.
> +	 */
> +	if (bpf_prog_is_dev_bound(prog->aux))
> +		goto err;
> +
> +	if (!bpf_prog_map_compatible(map, prog))
> +		goto err;

I think it's better to move the new check into bpf_prog_map_compatible()
itself; that way it'll cover cpumaps and devmaps as well :)

-Toke