Re: [PATCH perf/core 1/5] libbpf: Add bpf_program__set_insns function

[Jiri Olsa <olsajiri@gmail.com>]

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@iogearbox.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@kernel.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.
> 
> 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