Date:   Tue, 26 Apr 2022 08:58:12 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Jiri Olsa <olsajiri@...il.com>
Cc:     Daniel Borkmann <daniel@...earbox.net>,
        Jiri Olsa <jolsa@...nel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        "linux-perf-use." <linux-perf-users@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <a.p.zijlstra@...llo.nl>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Ian Rogers <irogers@...gle.com>
Subject: Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

On Mon, Apr 25, 2022 at 11:57 PM Jiri Olsa <olsajiri@...il.com> wrote:
>
> On Mon, Apr 25, 2022 at 11:19:09PM -0700, Andrii Nakryiko wrote:
> > On Mon, Apr 25, 2022 at 9:22 AM Daniel Borkmann <daniel@...earbox.net> wrote:
> > >
> > > On 4/22/22 12:00 PM, Jiri Olsa wrote:
> > > > Adding bpf_program__set_insns that allows to set new
> > > > instructions for program.
> > > >
> > > > Also moving bpf_program__attach_kprobe_multi_opts on
> > > > the proper name sorted place in map file.
> >
> > would make sense to fix it as a separate patch, it has nothing to do
> > with bpf_program__set_insns() API itself
>
> np
>
> >
> > > >
> > > > Signed-off-by: Jiri Olsa <jolsa@...nel.org>
> > > > ---
> > > >   tools/lib/bpf/libbpf.c   |  8 ++++++++
> > > >   tools/lib/bpf/libbpf.h   | 12 ++++++++++++
> > > >   tools/lib/bpf/libbpf.map |  3 ++-
> > > >   3 files changed, 22 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > > index 809fe209cdcc..284790d81c1b 100644
> > > > --- a/tools/lib/bpf/libbpf.c
> > > > +++ b/tools/lib/bpf/libbpf.c
> > > > @@ -8457,6 +8457,14 @@ size_t bpf_program__insn_cnt(const struct bpf_program *prog)
> > > >       return prog->insns_cnt;
> > > >   }
> > > >
> > > > +void bpf_program__set_insns(struct bpf_program *prog,
> > > > +                         struct bpf_insn *insns, size_t insns_cnt)
> > > > +{
> > > > +     free(prog->insns);
> > > > +     prog->insns = insns;
> > > > +     prog->insns_cnt = insns_cnt;
> >
> > let's not store user-provided pointer here. Please realloc prog->insns
> > as necessary and copy over insns into it.
> >
> > Also let's at least add the check for prog->loaded and return -EBUSY
> > in such a case. And of course this API should return int, not void.
>
> ok, will change
>
> >
> > > > +}
> > > > +
> > > >   int bpf_program__set_prep(struct bpf_program *prog, int nr_instances,
> > > >                         bpf_program_prep_t prep)
> > > >   {
> > > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > > index 05dde85e19a6..b31ad58d335f 100644
> > > > --- a/tools/lib/bpf/libbpf.h
> > > > +++ b/tools/lib/bpf/libbpf.h
> > > > @@ -323,6 +323,18 @@ struct bpf_insn;
> > > >    * different.
> > > >    */
> > > >   LIBBPF_API const struct bpf_insn *bpf_program__insns(const struct bpf_program *prog);
> > > > +
> > > > +/**
> > > > + * @brief **bpf_program__set_insns()** can set BPF program's underlying
> > > > + * BPF instructions.
> > > > + * @param prog BPF program for which to return instructions
> > > > + * @param insn a pointer to an array of BPF instructions
> > > > + * @param insns_cnt number of `struct bpf_insn`'s that form
> > > > + * specified BPF program
> > > > + */
> >
> > This API makes me want to cry... but I can't come up with anything
> > better for perf's use case.
> >

So thinking about this some more. If we make libbpf not to close maps
and prog FDs on BPF program load failure automatically and instead
doing it in bpf_object__close(), which would seem to be a totally fine
semantics and won't break any reasonable application as they always
have to call bpf_object__close() anyways to clean up all the
resources; we wouldn't need this horror of bpf_program__set_insns().
Your BPF program would fail to load, but you'll get its fully prepared
instructions with bpf_program__insns(), then you can just append
correct preamble. Meanwhile, all the maps will be created (they are
always created before the first program load), so all the FDs will be
correct.

This is certainly advanced knowledge of libbpf behavior, but the use
case you are trying to solve is also very unique and advanced (and I
wouldn't recommend anyone trying to do this anyways). WDYT? Would that
work?

> > But it can only more or less safely and sanely be used from the
> > prog_prepare_load_fn callback, so please add a big warning here saying
> > that this is a very advanced libbpf API and the user needs to know
> > what they are doing and this should be used from prog_prepare_load_fn
> > callback only. If bpf_program__set_insns() is called before
> > prog_prepare_load_fn any map/subprog/etc relocation will most probably
> > fail or corrupt BPF program code.
>
> will add the warnings
>
> >
> > > > +LIBBPF_API void bpf_program__set_insns(struct bpf_program *prog,
> > > > +                                    struct bpf_insn *insns, size_t insns_cnt);
> >
> > s/insns_cnt/insn_cnt/
> >
> > > > +
> > >
> > > Iiuc, patch 2 should be squashed into this one given they logically belong to the
> > > same change?
> > >
> > > Fwiw, I think the API description should be elaborated a bit more, in particular that
> > > the passed-in insns need to be from allocated dynamic memory which is later on passed
> > > to free(), and maybe also constraints at which point in time bpf_program__set_insns()
> > > may be called.. (as well as high-level description on potential use cases e.g. around
> > > patch 4).
> >
> > Yep, patch #1 is kind of broken without patch #2, so let's combine them.
>
> ok
>
> thanks,
> jirka

Powered by blists - more mailing lists