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

[Jiri Olsa <olsajiri@gmail.com>]

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@gmail.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@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.
> > >
> 
> 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

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);
 }