Some functions we use now for bpf prologue generation are
going to be deprecated, so reworking the current code not
to use them.
We need to replace following functions/struct:
bpf_program__set_prep
bpf_program__nth_fd
struct bpf_prog_prep_result
Current code uses bpf_program__set_prep to hook perf callback
before the program is loaded and provide new instructions with
the prologue.
We workaround this by using objects's 'unloaded' programs instructions
for that specific program and load new ebpf programs with prologue
using separate bpf_prog_load calls.
We keep new ebpf program instances descriptors in bpf programs
private struct.
Suggested-by: Andrii Nakryiko <[email protected]>
Signed-off-by: Jiri Olsa <[email protected]>
---
tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
1 file changed, 104 insertions(+), 18 deletions(-)
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 6f87729817ad..03f917002c00 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -48,6 +48,7 @@ struct bpf_prog_priv {
struct bpf_insn *insns_buf;
int nr_types;
int *type_mapping;
+ int *proglogue_fds;
};
struct bpf_perf_object {
@@ -55,6 +56,11 @@ struct bpf_perf_object {
struct bpf_object *obj;
};
+struct bpf_preproc_result {
+ struct bpf_insn *new_insn_ptr;
+ int new_insn_cnt;
+};
+
static LIST_HEAD(bpf_objects_list);
static struct hashmap *bpf_program_hash;
static struct hashmap *bpf_map_hash;
@@ -176,14 +182,31 @@ struct bpf_object *bpf__prepare_load(const char *filename, bool source)
return obj;
}
+static void close_prologue_programs(struct bpf_prog_priv *priv)
+{
+ struct perf_probe_event *pev;
+ int i, fd;
+
+ if (!priv->need_prologue)
+ return;
+ pev = &priv->pev;
+ for (i = 0; i < pev->ntevs; i++) {
+ fd = close(priv->proglogue_fds[i]);
+ if (fd != -1)
+ close(fd);
+ }
+}
+
static void
clear_prog_priv(const struct bpf_program *prog __maybe_unused,
void *_priv)
{
struct bpf_prog_priv *priv = _priv;
+ close_prologue_programs(priv);
cleanup_perf_probe_events(&priv->pev, 1);
zfree(&priv->insns_buf);
+ zfree(&priv->proglogue_fds);
zfree(&priv->type_mapping);
zfree(&priv->sys_name);
zfree(&priv->evt_name);
@@ -539,8 +562,8 @@ static int bpf__prepare_probe(void)
static int
preproc_gen_prologue(struct bpf_program *prog, int n,
- struct bpf_insn *orig_insns, int orig_insns_cnt,
- struct bpf_prog_prep_result *res)
+ const struct bpf_insn *orig_insns, int orig_insns_cnt,
+ struct bpf_preproc_result *res)
{
struct bpf_prog_priv *priv = program_priv(prog);
struct probe_trace_event *tev;
@@ -588,7 +611,6 @@ preproc_gen_prologue(struct bpf_program *prog, int n,
res->new_insn_ptr = buf;
res->new_insn_cnt = prologue_cnt + orig_insns_cnt;
- res->pfd = NULL;
return 0;
errout:
@@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
struct bpf_prog_priv *priv = program_priv(prog);
struct perf_probe_event *pev;
bool need_prologue = false;
- int err, i;
+ int i;
if (IS_ERR_OR_NULL(priv)) {
pr_debug("Internal error when hook preprocessor\n");
@@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
return 0;
}
+ /*
+ * Do not load programs that need prologue, because we need
+ * to add prologue first, check bpf_object__load_prologue.
+ */
+ bpf_program__set_autoload(prog, false);
+
priv->need_prologue = true;
priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
if (!priv->insns_buf) {
@@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
return -ENOMEM;
}
+ priv->proglogue_fds = malloc(sizeof(int) * pev->ntevs);
+ if (!priv->proglogue_fds) {
+ pr_debug("Not enough memory: alloc prologue fds failed\n");
+ return -ENOMEM;
+ }
+ memset(priv->proglogue_fds, -1, sizeof(int) * pev->ntevs);
+
priv->type_mapping = malloc(sizeof(int) * pev->ntevs);
if (!priv->type_mapping) {
pr_debug("Not enough memory: alloc type_mapping failed\n");
@@ -742,13 +777,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
memset(priv->type_mapping, -1,
sizeof(int) * pev->ntevs);
- err = map_prologue(pev, priv->type_mapping, &priv->nr_types);
- if (err)
- return err;
-
- err = bpf_program__set_prep(prog, priv->nr_types,
- preproc_gen_prologue);
- return err;
+ return map_prologue(pev, priv->type_mapping, &priv->nr_types);
}
int bpf__probe(struct bpf_object *obj)
@@ -855,6 +884,66 @@ int bpf__unprobe(struct bpf_object *obj)
return ret;
}
+static int bpf_object__load_prologue(struct bpf_object *obj)
+{
+ const struct bpf_insn *orig_insns;
+ struct bpf_preproc_result res;
+ struct perf_probe_event *pev;
+ struct bpf_program *prog;
+ int orig_insns_cnt;
+
+ bpf_object__for_each_program(prog, obj) {
+ struct bpf_prog_priv *priv = program_priv(prog);
+ int err, i, fd;
+
+ if (IS_ERR_OR_NULL(priv)) {
+ pr_debug("bpf: failed to get private field\n");
+ return -BPF_LOADER_ERRNO__INTERNAL;
+ }
+
+ if (!priv->need_prologue)
+ continue;
+
+ /*
+ * For each program that needs prologue we do following:
+ *
+ * - take its current instructions and use them
+ * to generate the new code with prologue
+ *
+ * - load new instructions with bpf_prog_load
+ * and keep the fd in proglogue_fds
+ *
+ * - new fd will be used bpf__foreach_event
+ * to connect this program with perf evsel
+ */
+ orig_insns = bpf_program__insns(prog);
+ orig_insns_cnt = bpf_program__insn_cnt(prog);
+
+ pev = &priv->pev;
+ for (i = 0; i < pev->ntevs; i++) {
+ err = preproc_gen_prologue(prog, i, orig_insns,
+ orig_insns_cnt, &res);
+ if (err)
+ return err;
+
+ fd = bpf_prog_load(bpf_program__get_type(prog),
+ bpf_program__name(prog), "GPL",
+ res.new_insn_ptr,
+ res.new_insn_cnt, NULL);
+ if (fd < 0) {
+ char bf[128];
+
+ libbpf_strerror(-errno, bf, sizeof(bf));
+ pr_debug("bpf: load objects with prologue failed: err=%d: (%s)\n",
+ -errno, bf);
+ return -errno;
+ }
+ priv->proglogue_fds[i] = fd;
+ }
+ }
+ return 0;
+}
+
int bpf__load(struct bpf_object *obj)
{
int err;
@@ -866,7 +955,7 @@ int bpf__load(struct bpf_object *obj)
pr_debug("bpf: load objects failed: err=%d: (%s)\n", err, bf);
return err;
}
- return 0;
+ return bpf_object__load_prologue(obj);
}
int bpf__foreach_event(struct bpf_object *obj,
@@ -901,13 +990,10 @@ int bpf__foreach_event(struct bpf_object *obj,
for (i = 0; i < pev->ntevs; i++) {
tev = &pev->tevs[i];
- if (priv->need_prologue) {
- int type = priv->type_mapping[i];
-
- fd = bpf_program__nth_fd(prog, type);
- } else {
+ if (priv->need_prologue)
+ fd = priv->proglogue_fds[i];
+ else
fd = bpf_program__fd(prog);
- }
if (fd < 0) {
pr_debug("bpf: failed to get file descriptor\n");
--
2.35.1
On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <[email protected]> wrote:
>
> Some functions we use now for bpf prologue generation are
> going to be deprecated, so reworking the current code not
> to use them.
>
> We need to replace following functions/struct:
> bpf_program__set_prep
> bpf_program__nth_fd
> struct bpf_prog_prep_result
>
> Current code uses bpf_program__set_prep to hook perf callback
> before the program is loaded and provide new instructions with
> the prologue.
>
> We workaround this by using objects's 'unloaded' programs instructions
> for that specific program and load new ebpf programs with prologue
> using separate bpf_prog_load calls.
>
> We keep new ebpf program instances descriptors in bpf programs
> private struct.
>
> Suggested-by: Andrii Nakryiko <[email protected]>
> Signed-off-by: Jiri Olsa <[email protected]>
> ---
> tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> 1 file changed, 104 insertions(+), 18 deletions(-)
>
[...]
> errout:
> @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> struct bpf_prog_priv *priv = program_priv(prog);
> struct perf_probe_event *pev;
> bool need_prologue = false;
> - int err, i;
> + int i;
>
> if (IS_ERR_OR_NULL(priv)) {
> pr_debug("Internal error when hook preprocessor\n");
> @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> return 0;
> }
>
> + /*
> + * Do not load programs that need prologue, because we need
> + * to add prologue first, check bpf_object__load_prologue.
> + */
> + bpf_program__set_autoload(prog, false);
if you set autoload to false, program instructions might be invalid in
the end. Libbpf doesn't apply some (all?) relocations to such
programs, doesn't resolve CO-RE, etc, etc. You have to let
"prototypal" BPF program to be loaded before you can grab final
instructions. It's not great, but in your case it should work, right?
> +
> priv->need_prologue = true;
> priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> if (!priv->insns_buf) {
> @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> return -ENOMEM;
> }
>
[...]
> + /*
> + * For each program that needs prologue we do following:
> + *
> + * - take its current instructions and use them
> + * to generate the new code with prologue
> + *
> + * - load new instructions with bpf_prog_load
> + * and keep the fd in proglogue_fds
> + *
> + * - new fd will be used bpf__foreach_event
> + * to connect this program with perf evsel
> + */
> + orig_insns = bpf_program__insns(prog);
> + orig_insns_cnt = bpf_program__insn_cnt(prog);
> +
> + pev = &priv->pev;
> + for (i = 0; i < pev->ntevs; i++) {
> + err = preproc_gen_prologue(prog, i, orig_insns,
> + orig_insns_cnt, &res);
> + if (err)
> + return err;
> +
> + fd = bpf_prog_load(bpf_program__get_type(prog),
nit: bpf_program__type() is preferred (we are deprecating/discouraging
"get_" prefixed getters in libbpf 1.0)
> + bpf_program__name(prog), "GPL",
would it make sense to give each clone a distinct name?
> + res.new_insn_ptr,
> + res.new_insn_cnt, NULL);
> + if (fd < 0) {
> + char bf[128];
> +
[...]
On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <[email protected]> wrote:
> >
> > Some functions we use now for bpf prologue generation are
> > going to be deprecated, so reworking the current code not
> > to use them.
> >
> > We need to replace following functions/struct:
> > bpf_program__set_prep
> > bpf_program__nth_fd
> > struct bpf_prog_prep_result
> >
> > Current code uses bpf_program__set_prep to hook perf callback
> > before the program is loaded and provide new instructions with
> > the prologue.
> >
> > We workaround this by using objects's 'unloaded' programs instructions
> > for that specific program and load new ebpf programs with prologue
> > using separate bpf_prog_load calls.
> >
> > We keep new ebpf program instances descriptors in bpf programs
> > private struct.
> >
> > Suggested-by: Andrii Nakryiko <[email protected]>
> > Signed-off-by: Jiri Olsa <[email protected]>
> > ---
> > tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > 1 file changed, 104 insertions(+), 18 deletions(-)
> >
>
> [...]
>
> > errout:
> > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > struct bpf_prog_priv *priv = program_priv(prog);
> > struct perf_probe_event *pev;
> > bool need_prologue = false;
> > - int err, i;
> > + int i;
> >
> > if (IS_ERR_OR_NULL(priv)) {
> > pr_debug("Internal error when hook preprocessor\n");
> > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > return 0;
> > }
> >
> > + /*
> > + * Do not load programs that need prologue, because we need
> > + * to add prologue first, check bpf_object__load_prologue.
> > + */
> > + bpf_program__set_autoload(prog, false);
>
> if you set autoload to false, program instructions might be invalid in
> the end. Libbpf doesn't apply some (all?) relocations to such
> programs, doesn't resolve CO-RE, etc, etc. You have to let
> "prototypal" BPF program to be loaded before you can grab final
> instructions. It's not great, but in your case it should work, right?
hum, do we care? it should all be done when the 'new' program with
the prologue is loaded, right?
I switched it off because the verifier failed to load the program
without the prologue.. because in the originaal program there's no
code to grab the arguments that the rest of the code depends on,
so the verifier sees invalid access
>
> > +
> > priv->need_prologue = true;
> > priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> > if (!priv->insns_buf) {
> > @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > return -ENOMEM;
> > }
> >
>
> [...]
>
> > + /*
> > + * For each program that needs prologue we do following:
> > + *
> > + * - take its current instructions and use them
> > + * to generate the new code with prologue
> > + *
> > + * - load new instructions with bpf_prog_load
> > + * and keep the fd in proglogue_fds
> > + *
> > + * - new fd will be used bpf__foreach_event
> > + * to connect this program with perf evsel
> > + */
> > + orig_insns = bpf_program__insns(prog);
> > + orig_insns_cnt = bpf_program__insn_cnt(prog);
> > +
> > + pev = &priv->pev;
> > + for (i = 0; i < pev->ntevs; i++) {
> > + err = preproc_gen_prologue(prog, i, orig_insns,
> > + orig_insns_cnt, &res);
> > + if (err)
> > + return err;
> > +
> > + fd = bpf_prog_load(bpf_program__get_type(prog),
>
> nit: bpf_program__type() is preferred (we are deprecating/discouraging
> "get_" prefixed getters in libbpf 1.0)
ok, will change
>
> > + bpf_program__name(prog), "GPL",
>
> would it make sense to give each clone a distinct name?
AFAICS the original code uses same prog name for instances,
so I'd rather keep it that way
thanks,
jirka
>
> > + res.new_insn_ptr,
> > + res.new_insn_cnt, NULL);
> > + if (fd < 0) {
> > + char bf[128];
> > +
>
> [...]
On Fri, Feb 18, 2022 at 02:03:28PM +0100, Jiri Olsa wrote:
> On Fri, Feb 18, 2022 at 10:01:45AM +0100, Jiri Olsa wrote:
> > On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <[email protected]> wrote:
> > > >
> > > > Some functions we use now for bpf prologue generation are
> > > > going to be deprecated, so reworking the current code not
> > > > to use them.
> > > >
> > > > We need to replace following functions/struct:
> > > > bpf_program__set_prep
> > > > bpf_program__nth_fd
> > > > struct bpf_prog_prep_result
> > > >
> > > > Current code uses bpf_program__set_prep to hook perf callback
> > > > before the program is loaded and provide new instructions with
> > > > the prologue.
> > > >
> > > > We workaround this by using objects's 'unloaded' programs instructions
> > > > for that specific program and load new ebpf programs with prologue
> > > > using separate bpf_prog_load calls.
> > > >
> > > > We keep new ebpf program instances descriptors in bpf programs
> > > > private struct.
> > > >
> > > > Suggested-by: Andrii Nakryiko <[email protected]>
> > > > Signed-off-by: Jiri Olsa <[email protected]>
> > > > ---
> > > > tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > > 1 file changed, 104 insertions(+), 18 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > > errout:
> > > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > struct bpf_prog_priv *priv = program_priv(prog);
> > > > struct perf_probe_event *pev;
> > > > bool need_prologue = false;
> > > > - int err, i;
> > > > + int i;
> > > >
> > > > if (IS_ERR_OR_NULL(priv)) {
> > > > pr_debug("Internal error when hook preprocessor\n");
> > > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > return 0;
> > > > }
> > > >
> > > > + /*
> > > > + * Do not load programs that need prologue, because we need
> > > > + * to add prologue first, check bpf_object__load_prologue.
> > > > + */
> > > > + bpf_program__set_autoload(prog, false);
> > >
> > > if you set autoload to false, program instructions might be invalid in
> > > the end. Libbpf doesn't apply some (all?) relocations to such
> > > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > > "prototypal" BPF program to be loaded before you can grab final
> > > instructions. It's not great, but in your case it should work, right?
> >
> > hum, do we care? it should all be done when the 'new' program with
> > the prologue is loaded, right?
> >
> > I switched it off because the verifier failed to load the program
> > without the prologue.. because in the originaal program there's no
> > code to grab the arguments that the rest of the code depends on,
> > so the verifier sees invalid access
> >
> > >
> > > > +
> > > > priv->need_prologue = true;
> > > > priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> > > > if (!priv->insns_buf) {
> > > > @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > return -ENOMEM;
> > > > }
> > > >
> > >
> > > [...]
> > >
> > > > + /*
> > > > + * For each program that needs prologue we do following:
> > > > + *
> > > > + * - take its current instructions and use them
> > > > + * to generate the new code with prologue
> > > > + *
> > > > + * - load new instructions with bpf_prog_load
> > > > + * and keep the fd in proglogue_fds
> > > > + *
> > > > + * - new fd will be used bpf__foreach_event
> > > > + * to connect this program with perf evsel
> > > > + */
> > > > + orig_insns = bpf_program__insns(prog);
> > > > + orig_insns_cnt = bpf_program__insn_cnt(prog);
> > > > +
> > > > + pev = &priv->pev;
> > > > + for (i = 0; i < pev->ntevs; i++) {
> > > > + err = preproc_gen_prologue(prog, i, orig_insns,
> > > > + orig_insns_cnt, &res);
> > > > + if (err)
> > > > + return err;
> > > > +
> > > > + fd = bpf_prog_load(bpf_program__get_type(prog),
> > >
> > > nit: bpf_program__type() is preferred (we are deprecating/discouraging
> > > "get_" prefixed getters in libbpf 1.0)
> >
> > ok, will change
>
> hum, I can't see bpf_program__type.. what do I miss?
nah I was on top of perf/core.. I see it now ;-)
jirka
On Fri, Feb 18, 2022 at 10:01:45AM +0100, Jiri Olsa wrote:
> On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <[email protected]> wrote:
> > >
> > > Some functions we use now for bpf prologue generation are
> > > going to be deprecated, so reworking the current code not
> > > to use them.
> > >
> > > We need to replace following functions/struct:
> > > bpf_program__set_prep
> > > bpf_program__nth_fd
> > > struct bpf_prog_prep_result
> > >
> > > Current code uses bpf_program__set_prep to hook perf callback
> > > before the program is loaded and provide new instructions with
> > > the prologue.
> > >
> > > We workaround this by using objects's 'unloaded' programs instructions
> > > for that specific program and load new ebpf programs with prologue
> > > using separate bpf_prog_load calls.
> > >
> > > We keep new ebpf program instances descriptors in bpf programs
> > > private struct.
> > >
> > > Suggested-by: Andrii Nakryiko <[email protected]>
> > > Signed-off-by: Jiri Olsa <[email protected]>
> > > ---
> > > tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > 1 file changed, 104 insertions(+), 18 deletions(-)
> > >
> >
> > [...]
> >
> > > errout:
> > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > struct bpf_prog_priv *priv = program_priv(prog);
> > > struct perf_probe_event *pev;
> > > bool need_prologue = false;
> > > - int err, i;
> > > + int i;
> > >
> > > if (IS_ERR_OR_NULL(priv)) {
> > > pr_debug("Internal error when hook preprocessor\n");
> > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > return 0;
> > > }
> > >
> > > + /*
> > > + * Do not load programs that need prologue, because we need
> > > + * to add prologue first, check bpf_object__load_prologue.
> > > + */
> > > + bpf_program__set_autoload(prog, false);
> >
> > if you set autoload to false, program instructions might be invalid in
> > the end. Libbpf doesn't apply some (all?) relocations to such
> > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > "prototypal" BPF program to be loaded before you can grab final
> > instructions. It's not great, but in your case it should work, right?
>
> hum, do we care? it should all be done when the 'new' program with
> the prologue is loaded, right?
>
> I switched it off because the verifier failed to load the program
> without the prologue.. because in the originaal program there's no
> code to grab the arguments that the rest of the code depends on,
> so the verifier sees invalid access
>
> >
> > > +
> > > priv->need_prologue = true;
> > > priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> > > if (!priv->insns_buf) {
> > > @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > return -ENOMEM;
> > > }
> > >
> >
> > [...]
> >
> > > + /*
> > > + * For each program that needs prologue we do following:
> > > + *
> > > + * - take its current instructions and use them
> > > + * to generate the new code with prologue
> > > + *
> > > + * - load new instructions with bpf_prog_load
> > > + * and keep the fd in proglogue_fds
> > > + *
> > > + * - new fd will be used bpf__foreach_event
> > > + * to connect this program with perf evsel
> > > + */
> > > + orig_insns = bpf_program__insns(prog);
> > > + orig_insns_cnt = bpf_program__insn_cnt(prog);
> > > +
> > > + pev = &priv->pev;
> > > + for (i = 0; i < pev->ntevs; i++) {
> > > + err = preproc_gen_prologue(prog, i, orig_insns,
> > > + orig_insns_cnt, &res);
> > > + if (err)
> > > + return err;
> > > +
> > > + fd = bpf_prog_load(bpf_program__get_type(prog),
> >
> > nit: bpf_program__type() is preferred (we are deprecating/discouraging
> > "get_" prefixed getters in libbpf 1.0)
>
> ok, will change
hum, I can't see bpf_program__type.. what do I miss?
jirka
On Fri, Feb 18, 2022 at 1:01 AM Jiri Olsa <[email protected]> wrote:
>
> On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <[email protected]> wrote:
> > >
> > > Some functions we use now for bpf prologue generation are
> > > going to be deprecated, so reworking the current code not
> > > to use them.
> > >
> > > We need to replace following functions/struct:
> > > bpf_program__set_prep
> > > bpf_program__nth_fd
> > > struct bpf_prog_prep_result
> > >
> > > Current code uses bpf_program__set_prep to hook perf callback
> > > before the program is loaded and provide new instructions with
> > > the prologue.
> > >
> > > We workaround this by using objects's 'unloaded' programs instructions
> > > for that specific program and load new ebpf programs with prologue
> > > using separate bpf_prog_load calls.
> > >
> > > We keep new ebpf program instances descriptors in bpf programs
> > > private struct.
> > >
> > > Suggested-by: Andrii Nakryiko <[email protected]>
> > > Signed-off-by: Jiri Olsa <[email protected]>
> > > ---
> > > tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > 1 file changed, 104 insertions(+), 18 deletions(-)
> > >
> >
> > [...]
> >
> > > errout:
> > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > struct bpf_prog_priv *priv = program_priv(prog);
> > > struct perf_probe_event *pev;
> > > bool need_prologue = false;
> > > - int err, i;
> > > + int i;
> > >
> > > if (IS_ERR_OR_NULL(priv)) {
> > > pr_debug("Internal error when hook preprocessor\n");
> > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > return 0;
> > > }
> > >
> > > + /*
> > > + * Do not load programs that need prologue, because we need
> > > + * to add prologue first, check bpf_object__load_prologue.
> > > + */
> > > + bpf_program__set_autoload(prog, false);
> >
> > if you set autoload to false, program instructions might be invalid in
> > the end. Libbpf doesn't apply some (all?) relocations to such
> > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > "prototypal" BPF program to be loaded before you can grab final
> > instructions. It's not great, but in your case it should work, right?
>
> hum, do we care? it should all be done when the 'new' program with
> the prologue is loaded, right?
yeah, you should care. If there is any BPF map involved, it is
properly resolved to correct FD (which is put into ldimm64 instruction
in BPF program code) during the load. If program is not autoloaded,
this is skipped. Same for any global variable or subprog call (if it's
not always inlined). So you very much should care for any non-trivial
program.
>
> I switched it off because the verifier failed to load the program
> without the prologue.. because in the original program there's no
> code to grab the arguments that the rest of the code depends on,
> so the verifier sees invalid access
Do you have an example of C code and corresponding BPF instructions
before/after prologue generation? Just curious to see in details how
this is done.
>
> >
> > > +
> > > priv->need_prologue = true;
> > > priv->insns_buf = malloc(sizeof(struct bpf_insn) * BPF_MAXINSNS);
> > > if (!priv->insns_buf) {
> > > @@ -734,6 +762,13 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > return -ENOMEM;
> > > }
> > >
> >
> > [...]
> >
> > > + /*
> > > + * For each program that needs prologue we do following:
> > > + *
> > > + * - take its current instructions and use them
> > > + * to generate the new code with prologue
> > > + *
> > > + * - load new instructions with bpf_prog_load
> > > + * and keep the fd in proglogue_fds
> > > + *
> > > + * - new fd will be used bpf__foreach_event
> > > + * to connect this program with perf evsel
> > > + */
> > > + orig_insns = bpf_program__insns(prog);
> > > + orig_insns_cnt = bpf_program__insn_cnt(prog);
> > > +
> > > + pev = &priv->pev;
> > > + for (i = 0; i < pev->ntevs; i++) {
> > > + err = preproc_gen_prologue(prog, i, orig_insns,
> > > + orig_insns_cnt, &res);
> > > + if (err)
> > > + return err;
> > > +
> > > + fd = bpf_prog_load(bpf_program__get_type(prog),
> >
> > nit: bpf_program__type() is preferred (we are deprecating/discouraging
> > "get_" prefixed getters in libbpf 1.0)
>
> ok, will change
It's been added in v0.7, yeah.
>
> >
> > > + bpf_program__name(prog), "GPL",
> >
> > would it make sense to give each clone a distinct name?
>
> AFAICS the original code uses same prog name for instances,
> so I'd rather keep it that way
>
sure, np
> thanks,
> jirka
>
> >
> > > + res.new_insn_ptr,
> > > + res.new_insn_cnt, NULL);
> > > + if (fd < 0) {
> > > + char bf[128];
> > > +
> >
> > [...]
On Sun, Feb 20, 2022 at 10:43:42AM -0800, Yonghong Song wrote:
>
>
> On 2/20/22 5:44 AM, Jiri Olsa wrote:
> > On Fri, Feb 18, 2022 at 11:55:16AM -0800, Andrii Nakryiko wrote:
> > > On Fri, Feb 18, 2022 at 1:01 AM Jiri Olsa <[email protected]> wrote:
> > > >
> > > > On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > > > > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <[email protected]> wrote:
> > > > > >
> > > > > > Some functions we use now for bpf prologue generation are
> > > > > > going to be deprecated, so reworking the current code not
> > > > > > to use them.
> > > > > >
> > > > > > We need to replace following functions/struct:
> > > > > > bpf_program__set_prep
> > > > > > bpf_program__nth_fd
> > > > > > struct bpf_prog_prep_result
> > > > > >
> > > > > > Current code uses bpf_program__set_prep to hook perf callback
> > > > > > before the program is loaded and provide new instructions with
> > > > > > the prologue.
> > > > > >
> > > > > > We workaround this by using objects's 'unloaded' programs instructions
> > > > > > for that specific program and load new ebpf programs with prologue
> > > > > > using separate bpf_prog_load calls.
> > > > > >
> > > > > > We keep new ebpf program instances descriptors in bpf programs
> > > > > > private struct.
> > > > > >
> > > > > > Suggested-by: Andrii Nakryiko <[email protected]>
> > > > > > Signed-off-by: Jiri Olsa <[email protected]>
> > > > > > ---
> > > > > > tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > > > > 1 file changed, 104 insertions(+), 18 deletions(-)
> > > > > >
> > > > >
> > > > > [...]
> > > > >
> > > > > > errout:
> > > > > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > > > struct bpf_prog_priv *priv = program_priv(prog);
> > > > > > struct perf_probe_event *pev;
> > > > > > bool need_prologue = false;
> > > > > > - int err, i;
> > > > > > + int i;
> > > > > >
> > > > > > if (IS_ERR_OR_NULL(priv)) {
> > > > > > pr_debug("Internal error when hook preprocessor\n");
> > > > > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > + /*
> > > > > > + * Do not load programs that need prologue, because we need
> > > > > > + * to add prologue first, check bpf_object__load_prologue.
> > > > > > + */
> > > > > > + bpf_program__set_autoload(prog, false);
> > > > >
> > > > > if you set autoload to false, program instructions might be invalid in
> > > > > the end. Libbpf doesn't apply some (all?) relocations to such
> > > > > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > > > > "prototypal" BPF program to be loaded before you can grab final
> > > > > instructions. It's not great, but in your case it should work, right?
> > > >
> > > > hum, do we care? it should all be done when the 'new' program with
> > > > the prologue is loaded, right?
> > >
> > > yeah, you should care. If there is any BPF map involved, it is
> > > properly resolved to correct FD (which is put into ldimm64 instruction
> > > in BPF program code) during the load. If program is not autoloaded,
> > > this is skipped. Same for any global variable or subprog call (if it's
> > > not always inlined). So you very much should care for any non-trivial
> > > program.
> >
> > ah too bad.. all that is in the load path, ok
> >
> > >
> > > >
> > > > I switched it off because the verifier failed to load the program
> > > > without the prologue.. because in the original program there's no
> > > > code to grab the arguments that the rest of the code depends on,
> > > > so the verifier sees invalid access
> > >
> > > Do you have an example of C code and corresponding BPF instructions
> > > before/after prologue generation? Just curious to see in details how
> > > this is done.
> >
> > so with following example:
> >
> > SEC("func=do_sched_setscheduler param->sched_priority@user")
> > int bpf_func__setscheduler(void *ctx, int err, int param)
> > {
> > char fmt[] = "prio: %ld";
> > bpf_trace_printk(fmt, sizeof(fmt), param);
> > return 1;
> > }
> >
> > perf will attach the code to do_sched_setscheduler function,
> > and read 'param->sched_priority' into 'param' argument
> >
> > so the resulting clang object expects 'param' to be in R3
> >
> > 0000000000000000 <bpf_func__setscheduler>:
> > 0: b7 01 00 00 64 00 00 00 r1 = 100
> > 1: 6b 1a f8 ff 00 00 00 00 *(u16 *)(r10 - 8) = r1
> > 2: 18 01 00 00 70 72 69 6f 00 00 00 00 3a 20 25 6c r1 = 77926701655
> > 4: 7b 1a f0 ff 00 00 00 00 *(u64 *)(r10 - 16) = r1
> > 5: bf a1 00 00 00 00 00 00 r1 = r10
> > 6: 07 01 00 00 f0 ff ff ff r1 += -16
> > 7: b7 02 00 00 0a 00 00 00 r2 = 10
> > 8: 85 00 00 00 06 00 00 00 call 6
> > 9: b7 00 00 00 01 00 00 00 r0 = 1
> > 10: 95 00 00 00 00 00 00 00 exit
> >
> > and R3 is loaded in the prologue code (first 15 instructions)
> > and it also sets 'err' (R2) with the result of the reading:
> >
> > 0: (bf) r6 = r1
> > 1: (79) r3 = *(u64 *)(r6 +96)
> > 2: (bf) r7 = r10
> > 3: (07) r7 += -8
> > 4: (7b) *(u64 *)(r10 -8) = r3
> > 5: (b7) r2 = 8
> > 6: (bf) r1 = r7
> > 7: (85) call bpf_probe_read_user#-60848
> > 8: (55) if r0 != 0x0 goto pc+2
> > 9: (61) r3 = *(u32 *)(r10 -8)
> > 10: (05) goto pc+3
> > 11: (b7) r2 = 1
> > 12: (b7) r3 = 0
> > 13: (05) goto pc+1
> > 14: (b7) r2 = 0
> > 15: (bf) r1 = r6
> >
> > 16: (b7) r1 = 100
> > 17: (6b) *(u16 *)(r10 -8) = r1
> > 18: (18) r1 = 0x6c25203a6f697270
> > 20: (7b) *(u64 *)(r10 -16) = r1
> > 21: (bf) r1 = r10
> > 22: (07) r1 += -16
> > 23: (b7) r2 = 10
> > 24: (85) call bpf_trace_printk#-54848
> > 25: (b7) r0 = 1
> > 26: (95) exit
>
> Just curious. Is the prologue code generated through C code or through
> asm code? Is it possible prologue code can be generated through C
it's C code in perf generating bpf instructions:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/util/bpf-prologue.c?h=perf/core
> code with similar mechanism like BPF_PROG macro? Or this is already
> an API which cannot be changed?
do you mean to have some stub like:
int bpf_func__setscheduler_stub(void *ctx)
{
return bpf_func__setscheduler(ctx, 0, 0)
}
int bpf_func__setscheduler(void *ctx, int err, int param)
{
char fmt[] = "prio: %ld";
bpf_trace_printk(fmt, sizeof(fmt), param);
return 1;
}
to make verifier happy
then we'd need instructions for bpf_func__setscheduler
it looks like subprogram instructions are appended and we should
be able to locate bpf_func__setscheduler start in instructions
returned in bpf_program__insns ? anyway does not look nice ;-)
jirka
On Fri, Feb 18, 2022 at 11:55:16AM -0800, Andrii Nakryiko wrote:
> On Fri, Feb 18, 2022 at 1:01 AM Jiri Olsa <[email protected]> wrote:
> >
> > On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <[email protected]> wrote:
> > > >
> > > > Some functions we use now for bpf prologue generation are
> > > > going to be deprecated, so reworking the current code not
> > > > to use them.
> > > >
> > > > We need to replace following functions/struct:
> > > > bpf_program__set_prep
> > > > bpf_program__nth_fd
> > > > struct bpf_prog_prep_result
> > > >
> > > > Current code uses bpf_program__set_prep to hook perf callback
> > > > before the program is loaded and provide new instructions with
> > > > the prologue.
> > > >
> > > > We workaround this by using objects's 'unloaded' programs instructions
> > > > for that specific program and load new ebpf programs with prologue
> > > > using separate bpf_prog_load calls.
> > > >
> > > > We keep new ebpf program instances descriptors in bpf programs
> > > > private struct.
> > > >
> > > > Suggested-by: Andrii Nakryiko <[email protected]>
> > > > Signed-off-by: Jiri Olsa <[email protected]>
> > > > ---
> > > > tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > > 1 file changed, 104 insertions(+), 18 deletions(-)
> > > >
> > >
> > > [...]
> > >
> > > > errout:
> > > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > struct bpf_prog_priv *priv = program_priv(prog);
> > > > struct perf_probe_event *pev;
> > > > bool need_prologue = false;
> > > > - int err, i;
> > > > + int i;
> > > >
> > > > if (IS_ERR_OR_NULL(priv)) {
> > > > pr_debug("Internal error when hook preprocessor\n");
> > > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > return 0;
> > > > }
> > > >
> > > > + /*
> > > > + * Do not load programs that need prologue, because we need
> > > > + * to add prologue first, check bpf_object__load_prologue.
> > > > + */
> > > > + bpf_program__set_autoload(prog, false);
> > >
> > > if you set autoload to false, program instructions might be invalid in
> > > the end. Libbpf doesn't apply some (all?) relocations to such
> > > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > > "prototypal" BPF program to be loaded before you can grab final
> > > instructions. It's not great, but in your case it should work, right?
> >
> > hum, do we care? it should all be done when the 'new' program with
> > the prologue is loaded, right?
>
> yeah, you should care. If there is any BPF map involved, it is
> properly resolved to correct FD (which is put into ldimm64 instruction
> in BPF program code) during the load. If program is not autoloaded,
> this is skipped. Same for any global variable or subprog call (if it's
> not always inlined). So you very much should care for any non-trivial
> program.
ah too bad.. all that is in the load path, ok
>
> >
> > I switched it off because the verifier failed to load the program
> > without the prologue.. because in the original program there's no
> > code to grab the arguments that the rest of the code depends on,
> > so the verifier sees invalid access
>
> Do you have an example of C code and corresponding BPF instructions
> before/after prologue generation? Just curious to see in details how
> this is done.
so with following example:
SEC("func=do_sched_setscheduler param->sched_priority@user")
int bpf_func__setscheduler(void *ctx, int err, int param)
{
char fmt[] = "prio: %ld";
bpf_trace_printk(fmt, sizeof(fmt), param);
return 1;
}
perf will attach the code to do_sched_setscheduler function,
and read 'param->sched_priority' into 'param' argument
so the resulting clang object expects 'param' to be in R3
0000000000000000 <bpf_func__setscheduler>:
0: b7 01 00 00 64 00 00 00 r1 = 100
1: 6b 1a f8 ff 00 00 00 00 *(u16 *)(r10 - 8) = r1
2: 18 01 00 00 70 72 69 6f 00 00 00 00 3a 20 25 6c r1 = 77926701655
4: 7b 1a f0 ff 00 00 00 00 *(u64 *)(r10 - 16) = r1
5: bf a1 00 00 00 00 00 00 r1 = r10
6: 07 01 00 00 f0 ff ff ff r1 += -16
7: b7 02 00 00 0a 00 00 00 r2 = 10
8: 85 00 00 00 06 00 00 00 call 6
9: b7 00 00 00 01 00 00 00 r0 = 1
10: 95 00 00 00 00 00 00 00 exit
and R3 is loaded in the prologue code (first 15 instructions)
and it also sets 'err' (R2) with the result of the reading:
0: (bf) r6 = r1
1: (79) r3 = *(u64 *)(r6 +96)
2: (bf) r7 = r10
3: (07) r7 += -8
4: (7b) *(u64 *)(r10 -8) = r3
5: (b7) r2 = 8
6: (bf) r1 = r7
7: (85) call bpf_probe_read_user#-60848
8: (55) if r0 != 0x0 goto pc+2
9: (61) r3 = *(u32 *)(r10 -8)
10: (05) goto pc+3
11: (b7) r2 = 1
12: (b7) r3 = 0
13: (05) goto pc+1
14: (b7) r2 = 0
15: (bf) r1 = r6
16: (b7) r1 = 100
17: (6b) *(u16 *)(r10 -8) = r1
18: (18) r1 = 0x6c25203a6f697270
20: (7b) *(u64 *)(r10 -16) = r1
21: (bf) r1 = r10
22: (07) r1 += -16
23: (b7) r2 = 10
24: (85) call bpf_trace_printk#-54848
25: (b7) r0 = 1
26: (95) exit
I'm still scratching my head how to workaround this.. we do want maps
and all the other updates to the code, but verifier won't let it pass
without the prologue code
jirka
On Sun, Feb 20, 2022 at 5:44 AM Jiri Olsa <[email protected]> wrote:
>
> On Fri, Feb 18, 2022 at 11:55:16AM -0800, Andrii Nakryiko wrote:
> > On Fri, Feb 18, 2022 at 1:01 AM Jiri Olsa <[email protected]> wrote:
> > >
> > > On Thu, Feb 17, 2022 at 01:53:16PM -0800, Andrii Nakryiko wrote:
> > > > On Thu, Feb 17, 2022 at 5:19 AM Jiri Olsa <[email protected]> wrote:
> > > > >
> > > > > Some functions we use now for bpf prologue generation are
> > > > > going to be deprecated, so reworking the current code not
> > > > > to use them.
> > > > >
> > > > > We need to replace following functions/struct:
> > > > > bpf_program__set_prep
> > > > > bpf_program__nth_fd
> > > > > struct bpf_prog_prep_result
> > > > >
> > > > > Current code uses bpf_program__set_prep to hook perf callback
> > > > > before the program is loaded and provide new instructions with
> > > > > the prologue.
> > > > >
> > > > > We workaround this by using objects's 'unloaded' programs instructions
> > > > > for that specific program and load new ebpf programs with prologue
> > > > > using separate bpf_prog_load calls.
> > > > >
> > > > > We keep new ebpf program instances descriptors in bpf programs
> > > > > private struct.
> > > > >
> > > > > Suggested-by: Andrii Nakryiko <[email protected]>
> > > > > Signed-off-by: Jiri Olsa <[email protected]>
> > > > > ---
> > > > > tools/perf/util/bpf-loader.c | 122 +++++++++++++++++++++++++++++------
> > > > > 1 file changed, 104 insertions(+), 18 deletions(-)
> > > > >
> > > >
> > > > [...]
> > > >
> > > > > errout:
> > > > > @@ -696,7 +718,7 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > > struct bpf_prog_priv *priv = program_priv(prog);
> > > > > struct perf_probe_event *pev;
> > > > > bool need_prologue = false;
> > > > > - int err, i;
> > > > > + int i;
> > > > >
> > > > > if (IS_ERR_OR_NULL(priv)) {
> > > > > pr_debug("Internal error when hook preprocessor\n");
> > > > > @@ -727,6 +749,12 @@ static int hook_load_preprocessor(struct bpf_program *prog)
> > > > > return 0;
> > > > > }
> > > > >
> > > > > + /*
> > > > > + * Do not load programs that need prologue, because we need
> > > > > + * to add prologue first, check bpf_object__load_prologue.
> > > > > + */
> > > > > + bpf_program__set_autoload(prog, false);
> > > >
> > > > if you set autoload to false, program instructions might be invalid in
> > > > the end. Libbpf doesn't apply some (all?) relocations to such
> > > > programs, doesn't resolve CO-RE, etc, etc. You have to let
> > > > "prototypal" BPF program to be loaded before you can grab final
> > > > instructions. It's not great, but in your case it should work, right?
> > >
> > > hum, do we care? it should all be done when the 'new' program with
> > > the prologue is loaded, right?
> >
> > yeah, you should care. If there is any BPF map involved, it is
> > properly resolved to correct FD (which is put into ldimm64 instruction
> > in BPF program code) during the load. If program is not autoloaded,
> > this is skipped. Same for any global variable or subprog call (if it's
> > not always inlined). So you very much should care for any non-trivial
> > program.
>
> ah too bad.. all that is in the load path, ok
>
> >
> > >
> > > I switched it off because the verifier failed to load the program
> > > without the prologue.. because in the original program there's no
> > > code to grab the arguments that the rest of the code depends on,
> > > so the verifier sees invalid access
> >
> > Do you have an example of C code and corresponding BPF instructions
> > before/after prologue generation? Just curious to see in details how
> > this is done.
>
> so with following example:
>
> SEC("func=do_sched_setscheduler param->sched_priority@user")
> int bpf_func__setscheduler(void *ctx, int err, int param)
> {
> char fmt[] = "prio: %ld";
> bpf_trace_printk(fmt, sizeof(fmt), param);
> return 1;
> }
>
> perf will attach the code to do_sched_setscheduler function,
> and read 'param->sched_priority' into 'param' argument
>
> so the resulting clang object expects 'param' to be in R3
>
> 0000000000000000 <bpf_func__setscheduler>:
> 0: b7 01 00 00 64 00 00 00 r1 = 100
> 1: 6b 1a f8 ff 00 00 00 00 *(u16 *)(r10 - 8) = r1
> 2: 18 01 00 00 70 72 69 6f 00 00 00 00 3a 20 25 6c r1 = 77926701655
> 4: 7b 1a f0 ff 00 00 00 00 *(u64 *)(r10 - 16) = r1
> 5: bf a1 00 00 00 00 00 00 r1 = r10
> 6: 07 01 00 00 f0 ff ff ff r1 += -16
> 7: b7 02 00 00 0a 00 00 00 r2 = 10
> 8: 85 00 00 00 06 00 00 00 call 6
> 9: b7 00 00 00 01 00 00 00 r0 = 1
> 10: 95 00 00 00 00 00 00 00 exit
>
> and R3 is loaded in the prologue code (first 15 instructions)
> and it also sets 'err' (R2) with the result of the reading:
>
> 0: (bf) r6 = r1
> 1: (79) r3 = *(u64 *)(r6 +96)
> 2: (bf) r7 = r10
> 3: (07) r7 += -8
> 4: (7b) *(u64 *)(r10 -8) = r3
> 5: (b7) r2 = 8
> 6: (bf) r1 = r7
> 7: (85) call bpf_probe_read_user#-60848
> 8: (55) if r0 != 0x0 goto pc+2
> 9: (61) r3 = *(u32 *)(r10 -8)
> 10: (05) goto pc+3
> 11: (b7) r2 = 1
> 12: (b7) r3 = 0
> 13: (05) goto pc+1
> 14: (b7) r2 = 0
> 15: (bf) r1 = r6
>
> 16: (b7) r1 = 100
> 17: (6b) *(u16 *)(r10 -8) = r1
> 18: (18) r1 = 0x6c25203a6f697270
> 20: (7b) *(u64 *)(r10 -16) = r1
> 21: (bf) r1 = r10
> 22: (07) r1 += -16
> 23: (b7) r2 = 10
> 24: (85) call bpf_trace_printk#-54848
> 25: (b7) r0 = 1
> 26: (95) exit
>
>
> I'm still scratching my head how to workaround this.. we do want maps
> and all the other updates to the code, but verifier won't let it pass
> without the prologue code
ugh, perf cornered itself into supporting this crazy scheme and now
there is no good solution. I'm still questioning the value of
supporting this going forward. Is there an evidence that anyone is
using this functionality at all? Is it worth it trying to carry it on
just because we have some example that exercises this feature?
Anyways, one way to solve this is to add bpf_program__set_insns() that
could be called from prog_init_fn callback (which I just realized
hasn't landed yet, I'll send v4 today) to prepend a simple preamble
like this:
r1 = 0;
r2 = 0;
r3 = 0;
f4 = 0;
r5 = 0; /* how many input arguments we support? */
This will make all input arguments initialized, libbpf will be able to
adjust all the relocations and stuff. Once this "prototype program" is
loaded, perf can grab final instructions and replace first X
instructions with desired preamble.
But... ugliness and horror, yeah :(
>
> jirka
On Wed, Feb 23, 2022 at 02:29:56PM -0800, Andrii Nakryiko wrote:
SNIP
> > and R3 is loaded in the prologue code (first 15 instructions)
> > and it also sets 'err' (R2) with the result of the reading:
> >
> > 0: (bf) r6 = r1
> > 1: (79) r3 = *(u64 *)(r6 +96)
> > 2: (bf) r7 = r10
> > 3: (07) r7 += -8
> > 4: (7b) *(u64 *)(r10 -8) = r3
> > 5: (b7) r2 = 8
> > 6: (bf) r1 = r7
> > 7: (85) call bpf_probe_read_user#-60848
> > 8: (55) if r0 != 0x0 goto pc+2
> > 9: (61) r3 = *(u32 *)(r10 -8)
> > 10: (05) goto pc+3
> > 11: (b7) r2 = 1
> > 12: (b7) r3 = 0
> > 13: (05) goto pc+1
> > 14: (b7) r2 = 0
> > 15: (bf) r1 = r6
> >
> > 16: (b7) r1 = 100
> > 17: (6b) *(u16 *)(r10 -8) = r1
> > 18: (18) r1 = 0x6c25203a6f697270
> > 20: (7b) *(u64 *)(r10 -16) = r1
> > 21: (bf) r1 = r10
> > 22: (07) r1 += -16
> > 23: (b7) r2 = 10
> > 24: (85) call bpf_trace_printk#-54848
> > 25: (b7) r0 = 1
> > 26: (95) exit
> >
> >
> > I'm still scratching my head how to workaround this.. we do want maps
> > and all the other updates to the code, but verifier won't let it pass
> > without the prologue code
>
> ugh, perf cornered itself into supporting this crazy scheme and now
well, it just used the interface that was provided at the time
> there is no good solution. I'm still questioning the value of
> supporting this going forward. Is there an evidence that anyone is
> using this functionality at all? Is it worth it trying to carry it on
> just because we have some example that exercises this feature?
yea we discussed this again and I think we can somehow mark this
feature in perf as deprecated and remove it after some time,
because even with the workaround below it'll be pita ;-)
or people will come and scream and we will find some other solution
I already sent the rest of the changes (prog/map priv) separately
and will send some RFC for the deprecation
thanks,
jirka
>
> Anyways, one way to solve this is to add bpf_program__set_insns() that
> could be called from prog_init_fn callback (which I just realized
> hasn't landed yet, I'll send v4 today) to prepend a simple preamble
> like this:
>
> r1 = 0;
> r2 = 0;
> r3 = 0;
> f4 = 0;
> r5 = 0; /* how many input arguments we support? */
>
> This will make all input arguments initialized, libbpf will be able to
> adjust all the relocations and stuff. Once this "prototype program" is
> loaded, perf can grab final instructions and replace first X
> instructions with desired preamble.
>
> But... ugliness and horror, yeah :(
>
>
> >
> > jirka
Em Fri, Feb 25, 2022 at 01:14:49PM +0100, Jiri Olsa escreveu:
> On Wed, Feb 23, 2022 at 02:29:56PM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > and R3 is loaded in the prologue code (first 15 instructions)
> > > and it also sets 'err' (R2) with the result of the reading:
> > >
> > > 0: (bf) r6 = r1
> > > 1: (79) r3 = *(u64 *)(r6 +96)
> > > 2: (bf) r7 = r10
> > > 3: (07) r7 += -8
> > > 4: (7b) *(u64 *)(r10 -8) = r3
> > > 5: (b7) r2 = 8
> > > 6: (bf) r1 = r7
> > > 7: (85) call bpf_probe_read_user#-60848
> > > 8: (55) if r0 != 0x0 goto pc+2
> > > 9: (61) r3 = *(u32 *)(r10 -8)
> > > 10: (05) goto pc+3
> > > 11: (b7) r2 = 1
> > > 12: (b7) r3 = 0
> > > 13: (05) goto pc+1
> > > 14: (b7) r2 = 0
> > > 15: (bf) r1 = r6
> > >
> > > 16: (b7) r1 = 100
> > > 17: (6b) *(u16 *)(r10 -8) = r1
> > > 18: (18) r1 = 0x6c25203a6f697270
> > > 20: (7b) *(u64 *)(r10 -16) = r1
> > > 21: (bf) r1 = r10
> > > 22: (07) r1 += -16
> > > 23: (b7) r2 = 10
> > > 24: (85) call bpf_trace_printk#-54848
> > > 25: (b7) r0 = 1
> > > 26: (95) exit
> > >
> > >
> > > I'm still scratching my head how to workaround this.. we do want maps
> > > and all the other updates to the code, but verifier won't let it pass
> > > without the prologue code
> >
> > ugh, perf cornered itself into supporting this crazy scheme and now
> well, it just used the interface that was provided at the time
At the time it was where experimentation was done with tooling for eBPF,
Wangnan tried to provide a compact way to give access to parameters.
The problem now is for libbpf to remove something that is used and that
was documented to some extent in the perf tools examples so there _may_
be some usage of it, we just can't know.
Its like Linux removing some syscall that is "crazy" and wait for
somebody to complain of the breakage caused when they update to a new
version.
> > there is no good solution. I'm still questioning the value of
> > supporting this going forward. Is there an evidence that anyone is
> > using this functionality at all? Is it worth it trying to carry it on
> > just because we have some example that exercises this feature?
> yea we discussed this again and I think we can somehow mark this
> feature in perf as deprecated and remove it after some time,
> because even with the workaround below it'll be pita ;-)
>
> or people will come and scream and we will find some other solution
:-\ if you have some "ugly" way to keep the feature, can't we go with
it?
> I already sent the rest of the changes (prog/map priv) separately
> and will send some RFC for the deprecation
I'll look at it now.
Thanks for your work on this, Jiri.
- Araldo
> thanks,
> jirka
>
> >
> > Anyways, one way to solve this is to add bpf_program__set_insns() that
> > could be called from prog_init_fn callback (which I just realized
> > hasn't landed yet, I'll send v4 today) to prepend a simple preamble
> > like this:
> >
> > r1 = 0;
> > r2 = 0;
> > r3 = 0;
> > f4 = 0;
> > r5 = 0; /* how many input arguments we support? */
> >
> > This will make all input arguments initialized, libbpf will be able to
> > adjust all the relocations and stuff. Once this "prototype program" is
> > loaded, perf can grab final instructions and replace first X
> > instructions with desired preamble.
> >
> > But... ugliness and horror, yeah :(
> >
> >
> > >
> > > jirka
--
- Arnaldo