On Wed, Apr 27, 2022 at 1:42 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Tue, Apr 26, 2022 at 08:58:12AM -0700, Andrii Nakryiko wrote:
> > On Mon, Apr 25, 2022 at 11:57 PM Jiri Olsa <olsajiri@xxxxxxxxx> 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@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
> > >
> > > np
> > >
> > > >
> > > > > >
> > > > > > 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.
> > >
> > > 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?
>
> hm, so verifier will fail after all maps are set up during the walk
> of the program instructions.. I guess that could work, I'll give it
> a try, should be easy change in libbpf (like below) and also in perf
>
> but still the bpf_program__set_insns seems less horror to me
let's keep set_insns API then, but please do add a lot of warnings
into the description to make it very-very scary :)
>
> jirka
>
>
> ---
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c8df74e5f658..1eb75d4231ff 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -7577,19 +7577,6 @@ static int bpf_object_load(struct bpf_object *obj, int extra_log_level, const ch
> obj->btf_vmlinux = NULL;
>
> obj->loaded = true; /* doesn't matter if successfully or not */
> -
> - if (err)
> - goto out;
> -
> - return 0;
> -out:
> - /* unpin any maps that were auto-pinned during load */
> - for (i = 0; i < obj->nr_maps; i++)
> - if (obj->maps[i].pinned && !obj->maps[i].reused)
> - bpf_map__unpin(&obj->maps[i], NULL);
> -
> - bpf_object_unload(obj);
> - pr_warn("failed to load object '%s'\n", obj->path);
> return libbpf_err(err);
> }
>
