On Mon, Apr 25, 2022 at 9:22 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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
> >
> > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > ---
> > 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.
> > +}
> > +
> > 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.
> > +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.
>
> > /**
> > * @brief **bpf_program__insn_cnt()** returns number of `struct bpf_insn`'s
> > * that form specified BPF program.
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index dd35ee58bfaa..afa10d24ab41 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -444,7 +444,8 @@ LIBBPF_0.8.0 {
> > global:
> > bpf_object__destroy_subskeleton;
> > bpf_object__open_subskeleton;
> > + bpf_program__attach_kprobe_multi_opts;
> > + bpf_program__set_insns;
> > libbpf_register_prog_handler;
> > libbpf_unregister_prog_handler;
> > - bpf_program__attach_kprobe_multi_opts;
> > } LIBBPF_0.7.0;
> >
>
