| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 27 Apr 2022 10:42:17 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Andrii Nakryiko <andrii.nakryiko@...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 Tue, Apr 26, 2022 at 08:58:12AM -0700, Andrii Nakryiko wrote:
> 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?
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);
}
Powered by blists - more mailing lists