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

[Stanislav Fomichev <sdf@google.com>]

On Thu, Dec 8, 2022 at 2:39 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>
> 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...

Oh, right, that's a good point. Having defaults for dev-bound only
makes total sense.

> 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?

I'm not sure how libbpf can allow the load without performing the
relocation; maybe I'm missing something.
IIUC, libbpf uses the kfunc name (from the relocation?) and replaces
it with the kfunc id, right?

Having bpf_core_kfunc_exists would help, but this probably needs
compiler work first to preserve some of the kfunc traces in vmlinux.h?

So yeah, I don't have any good ideas/suggestions here on how to make
it all magically work :-(

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

Will do, thanks!

> -Toke
>