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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Dec 2022 15:46:55 -0800
From:   Stanislav Fomichev <sdf@...gle.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>
Cc:     bpf@...r.kernel.org, ast@...nel.org, daniel@...earbox.net,
        andrii@...nel.org, martin.lau@...ux.dev, song@...nel.org,
        yhs@...com, john.fastabend@...il.com, kpsingh@...nel.org,
        haoluo@...gle.com, jolsa@...nel.org,
        David Ahern <dsahern@...il.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Willem de Bruijn <willemb@...gle.com>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Anatoly Burakov <anatoly.burakov@...el.com>,
        Alexander Lobakin <alexandr.lobakin@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        Maryam Tahhan <mtahhan@...hat.com>, xdp-hints@...-project.net,
        netdev@...r.kernel.org
Subject: Re: [xdp-hints] [PATCH bpf-next v3 03/12] bpf: XDP metadata RX kfuncs

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