| Message ID | 20241218225246.3170300-1-yonghong.song@linux.dev (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,1/2] libbpf: Add unique_match option for multi kprobe | expand |
On Wed, Dec 18, 2024 at 5:53 PM Yonghong Song <yonghong.song@linux.dev> wrote: > > Jordan reported an issue in Meta production environment where func > try_to_wake_up() is renamed to try_to_wake_up.llvm.<hash>() by clang > compiler at lto mode. The original 'kprobe/try_to_wake_up' does not > work any more since try_to_wake_up() does not match the actual func > name in /proc/kallsyms. > > There are a couple of ways to resolve this issue. For example, in > attach_kprobe(), we could do lookup in /proc/kallsyms so try_to_wake_up() > can be replaced by try_to_wake_up.llvm.<hach>(). Or we can force users > to use bpf_program__attach_kprobe() where they need to lookup > /proc/kallsyms to find out try_to_wake_up.llvm.<hach>(). But these two > approaches requires extra work by either libbpf or user. > > Luckily, suggested by Andrii, multi kprobe already supports wildcard ('*') > for symbol matching. In the above example, 'try_to_wake_up*' can match > to try_to_wake_up() or try_to_wake_up.llvm.<hash>() and this allows > bpf prog works for different kernels as some kernels may have > try_to_wake_up() and some others may have try_to_wake_up.llvm.<hash>(). > > The original intention is to kprobe try_to_wake_up() only, so an optional > field unique_match is added to struct bpf_kprobe_multi_opts. If the > field is set to true, the number of matched functions must be one. > Otherwise, the attachment will fail. In the above case, multi kprobe > with 'try_to_wake_up*' and unique_match preserves user functionality. > > Reported-by: Jordan Rome <linux@jordanrome.com> > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > --- > tools/lib/bpf/libbpf.c | 10 +++++++++- > tools/lib/bpf/libbpf.h | 4 +++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 66173ddb5a2d..649c6e92972a 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -11522,7 +11522,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > struct bpf_link *link = NULL; > const unsigned long *addrs; > int err, link_fd, prog_fd; > - bool retprobe, session; > + bool retprobe, session, unique_match; > const __u64 *cookies; > const char **syms; > size_t cnt; > @@ -11558,6 +11558,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > err = libbpf_available_kallsyms_parse(&res); > if (err) > goto error; > + > + unique_match = OPTS_GET(opts, unique_match, false); > + if (unique_match && res.cnt != 1) { > + pr_warn("prog '%s': failed to find unique match: cnt %lu\n", > + prog->name, res.cnt); nit: "failed to find a unique match. Num matches: %lu\n" or "Failed to find a unique match for %s'. Num matches: %lu\n" > + return libbpf_err_ptr(-EINVAL); > + } > + > addrs = res.addrs; > cnt = res.cnt; > } > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index d45807103565..3020ee45303a 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -552,10 +552,12 @@ struct bpf_kprobe_multi_opts { > bool retprobe; > /* create session kprobes */ > bool session; > + /* enforce unique match */ > + bool unique_match; > size_t :0; > }; > > -#define bpf_kprobe_multi_opts__last_field session > +#define bpf_kprobe_multi_opts__last_field unique_match > > LIBBPF_API struct bpf_link * > bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > -- > 2.43.5 > Ack. Thanks for the quick work, Yonghong!
On Wed, Dec 18, 2024 at 2:53 PM Yonghong Song <yonghong.song@linux.dev> wrote: > > Jordan reported an issue in Meta production environment where func > try_to_wake_up() is renamed to try_to_wake_up.llvm.<hash>() by clang > compiler at lto mode. The original 'kprobe/try_to_wake_up' does not > work any more since try_to_wake_up() does not match the actual func > name in /proc/kallsyms. > > There are a couple of ways to resolve this issue. For example, in > attach_kprobe(), we could do lookup in /proc/kallsyms so try_to_wake_up() > can be replaced by try_to_wake_up.llvm.<hach>(). Or we can force users > to use bpf_program__attach_kprobe() where they need to lookup > /proc/kallsyms to find out try_to_wake_up.llvm.<hach>(). But these two > approaches requires extra work by either libbpf or user. > > Luckily, suggested by Andrii, multi kprobe already supports wildcard ('*') > for symbol matching. In the above example, 'try_to_wake_up*' can match > to try_to_wake_up() or try_to_wake_up.llvm.<hash>() and this allows > bpf prog works for different kernels as some kernels may have > try_to_wake_up() and some others may have try_to_wake_up.llvm.<hash>(). > > The original intention is to kprobe try_to_wake_up() only, so an optional > field unique_match is added to struct bpf_kprobe_multi_opts. If the > field is set to true, the number of matched functions must be one. > Otherwise, the attachment will fail. In the above case, multi kprobe > with 'try_to_wake_up*' and unique_match preserves user functionality. > > Reported-by: Jordan Rome <linux@jordanrome.com> > Suggested-by: Andrii Nakryiko <andrii@kernel.org> > Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > --- > tools/lib/bpf/libbpf.c | 10 +++++++++- > tools/lib/bpf/libbpf.h | 4 +++- > 2 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > index 66173ddb5a2d..649c6e92972a 100644 > --- a/tools/lib/bpf/libbpf.c > +++ b/tools/lib/bpf/libbpf.c > @@ -11522,7 +11522,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > struct bpf_link *link = NULL; > const unsigned long *addrs; > int err, link_fd, prog_fd; > - bool retprobe, session; > + bool retprobe, session, unique_match; > const __u64 *cookies; > const char **syms; > size_t cnt; > @@ -11558,6 +11558,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > err = libbpf_available_kallsyms_parse(&res); > if (err) > goto error; > + > + unique_match = OPTS_GET(opts, unique_match, false); > + if (unique_match && res.cnt != 1) { > + pr_warn("prog '%s': failed to find unique match: cnt %lu\n", > + prog->name, res.cnt); > + return libbpf_err_ptr(-EINVAL); goto error, leaking resources here we should also think about interaction of unique_match interaction for !pattern case, and either reject it (if it makes no sense), or enforce it (if it does, I haven't really thought about which case do we have) pw-bot: cr > + } > + > addrs = res.addrs; > cnt = res.cnt; > } > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > index d45807103565..3020ee45303a 100644 > --- a/tools/lib/bpf/libbpf.h > +++ b/tools/lib/bpf/libbpf.h > @@ -552,10 +552,12 @@ struct bpf_kprobe_multi_opts { > bool retprobe; > /* create session kprobes */ > bool session; > + /* enforce unique match */ > + bool unique_match; > size_t :0; > }; > > -#define bpf_kprobe_multi_opts__last_field session > +#define bpf_kprobe_multi_opts__last_field unique_match > > LIBBPF_API struct bpf_link * > bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > -- > 2.43.5 >
On 1/6/25 4:24 PM, Andrii Nakryiko wrote: > On Wed, Dec 18, 2024 at 2:53 PM Yonghong Song <yonghong.song@linux.dev> wrote: >> Jordan reported an issue in Meta production environment where func >> try_to_wake_up() is renamed to try_to_wake_up.llvm.<hash>() by clang >> compiler at lto mode. The original 'kprobe/try_to_wake_up' does not >> work any more since try_to_wake_up() does not match the actual func >> name in /proc/kallsyms. >> >> There are a couple of ways to resolve this issue. For example, in >> attach_kprobe(), we could do lookup in /proc/kallsyms so try_to_wake_up() >> can be replaced by try_to_wake_up.llvm.<hach>(). Or we can force users >> to use bpf_program__attach_kprobe() where they need to lookup >> /proc/kallsyms to find out try_to_wake_up.llvm.<hach>(). But these two >> approaches requires extra work by either libbpf or user. >> >> Luckily, suggested by Andrii, multi kprobe already supports wildcard ('*') >> for symbol matching. In the above example, 'try_to_wake_up*' can match >> to try_to_wake_up() or try_to_wake_up.llvm.<hash>() and this allows >> bpf prog works for different kernels as some kernels may have >> try_to_wake_up() and some others may have try_to_wake_up.llvm.<hash>(). >> >> The original intention is to kprobe try_to_wake_up() only, so an optional >> field unique_match is added to struct bpf_kprobe_multi_opts. If the >> field is set to true, the number of matched functions must be one. >> Otherwise, the attachment will fail. In the above case, multi kprobe >> with 'try_to_wake_up*' and unique_match preserves user functionality. >> >> Reported-by: Jordan Rome <linux@jordanrome.com> >> Suggested-by: Andrii Nakryiko <andrii@kernel.org> >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> >> --- >> tools/lib/bpf/libbpf.c | 10 +++++++++- >> tools/lib/bpf/libbpf.h | 4 +++- >> 2 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c >> index 66173ddb5a2d..649c6e92972a 100644 >> --- a/tools/lib/bpf/libbpf.c >> +++ b/tools/lib/bpf/libbpf.c >> @@ -11522,7 +11522,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, >> struct bpf_link *link = NULL; >> const unsigned long *addrs; >> int err, link_fd, prog_fd; >> - bool retprobe, session; >> + bool retprobe, session, unique_match; >> const __u64 *cookies; >> const char **syms; >> size_t cnt; >> @@ -11558,6 +11558,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, >> err = libbpf_available_kallsyms_parse(&res); >> if (err) >> goto error; >> + >> + unique_match = OPTS_GET(opts, unique_match, false); >> + if (unique_match && res.cnt != 1) { >> + pr_warn("prog '%s': failed to find unique match: cnt %lu\n", >> + prog->name, res.cnt); >> + return libbpf_err_ptr(-EINVAL); > goto error, leaking resources here Ack. Will fix. > > > we should also think about interaction of unique_match interaction for > !pattern case, and either reject it (if it makes no sense), or enforce > it (if it does, I haven't really thought about which case do we have) The unique_match only makes sense for pattern case. So I suggest to reject the case unique_match && !pattern. WDYT? > > pw-bot: cr > >> + } >> + >> addrs = res.addrs; >> cnt = res.cnt; >> } >> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h >> index d45807103565..3020ee45303a 100644 >> --- a/tools/lib/bpf/libbpf.h >> +++ b/tools/lib/bpf/libbpf.h >> @@ -552,10 +552,12 @@ struct bpf_kprobe_multi_opts { >> bool retprobe; >> /* create session kprobes */ >> bool session; >> + /* enforce unique match */ >> + bool unique_match; >> size_t :0; >> }; >> >> -#define bpf_kprobe_multi_opts__last_field session >> +#define bpf_kprobe_multi_opts__last_field unique_match >> >> LIBBPF_API struct bpf_link * >> bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, >> -- >> 2.43.5 >>
On Tue, Jan 7, 2025 at 10:29 AM Yonghong Song <yonghong.song@linux.dev> wrote: > > > > > On 1/6/25 4:24 PM, Andrii Nakryiko wrote: > > On Wed, Dec 18, 2024 at 2:53 PM Yonghong Song <yonghong.song@linux.dev> wrote: > >> Jordan reported an issue in Meta production environment where func > >> try_to_wake_up() is renamed to try_to_wake_up.llvm.<hash>() by clang > >> compiler at lto mode. The original 'kprobe/try_to_wake_up' does not > >> work any more since try_to_wake_up() does not match the actual func > >> name in /proc/kallsyms. > >> > >> There are a couple of ways to resolve this issue. For example, in > >> attach_kprobe(), we could do lookup in /proc/kallsyms so try_to_wake_up() > >> can be replaced by try_to_wake_up.llvm.<hach>(). Or we can force users > >> to use bpf_program__attach_kprobe() where they need to lookup > >> /proc/kallsyms to find out try_to_wake_up.llvm.<hach>(). But these two > >> approaches requires extra work by either libbpf or user. > >> > >> Luckily, suggested by Andrii, multi kprobe already supports wildcard ('*') > >> for symbol matching. In the above example, 'try_to_wake_up*' can match > >> to try_to_wake_up() or try_to_wake_up.llvm.<hash>() and this allows > >> bpf prog works for different kernels as some kernels may have > >> try_to_wake_up() and some others may have try_to_wake_up.llvm.<hash>(). > >> > >> The original intention is to kprobe try_to_wake_up() only, so an optional > >> field unique_match is added to struct bpf_kprobe_multi_opts. If the > >> field is set to true, the number of matched functions must be one. > >> Otherwise, the attachment will fail. In the above case, multi kprobe > >> with 'try_to_wake_up*' and unique_match preserves user functionality. > >> > >> Reported-by: Jordan Rome <linux@jordanrome.com> > >> Suggested-by: Andrii Nakryiko <andrii@kernel.org> > >> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> > >> --- > >> tools/lib/bpf/libbpf.c | 10 +++++++++- > >> tools/lib/bpf/libbpf.h | 4 +++- > >> 2 files changed, 12 insertions(+), 2 deletions(-) > >> > >> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c > >> index 66173ddb5a2d..649c6e92972a 100644 > >> --- a/tools/lib/bpf/libbpf.c > >> +++ b/tools/lib/bpf/libbpf.c > >> @@ -11522,7 +11522,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > >> struct bpf_link *link = NULL; > >> const unsigned long *addrs; > >> int err, link_fd, prog_fd; > >> - bool retprobe, session; > >> + bool retprobe, session, unique_match; > >> const __u64 *cookies; > >> const char **syms; > >> size_t cnt; > >> @@ -11558,6 +11558,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > >> err = libbpf_available_kallsyms_parse(&res); > >> if (err) > >> goto error; > >> + > >> + unique_match = OPTS_GET(opts, unique_match, false); > >> + if (unique_match && res.cnt != 1) { > >> + pr_warn("prog '%s': failed to find unique match: cnt %lu\n", > >> + prog->name, res.cnt); > >> + return libbpf_err_ptr(-EINVAL); > > goto error, leaking resources here > > Ack. Will fix. > > > > > > > we should also think about interaction of unique_match interaction for > > !pattern case, and either reject it (if it makes no sense), or enforce > > it (if it does, I haven't really thought about which case do we have) > > The unique_match only makes sense for pattern case. So I suggest to > reject the case unique_match && !pattern. WDYT? > Yep, let's reject (we could make it behave well, just making sure that cnt == 1 if unique_match == true, but why bother, it's not intended to be used together). > > > > pw-bot: cr > > > >> + } > >> + > >> addrs = res.addrs; > >> cnt = res.cnt; > >> } > >> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h > >> index d45807103565..3020ee45303a 100644 > >> --- a/tools/lib/bpf/libbpf.h > >> +++ b/tools/lib/bpf/libbpf.h > >> @@ -552,10 +552,12 @@ struct bpf_kprobe_multi_opts { > >> bool retprobe; > >> /* create session kprobes */ > >> bool session; > >> + /* enforce unique match */ > >> + bool unique_match; > >> size_t :0; > >> }; > >> > >> -#define bpf_kprobe_multi_opts__last_field session > >> +#define bpf_kprobe_multi_opts__last_field unique_match > >> > >> LIBBPF_API struct bpf_link * > >> bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, > >> -- > >> 2.43.5 > >> >
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c index 66173ddb5a2d..649c6e92972a 100644 --- a/tools/lib/bpf/libbpf.c +++ b/tools/lib/bpf/libbpf.c @@ -11522,7 +11522,7 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, struct bpf_link *link = NULL; const unsigned long *addrs; int err, link_fd, prog_fd; - bool retprobe, session; + bool retprobe, session, unique_match; const __u64 *cookies; const char **syms; size_t cnt; @@ -11558,6 +11558,14 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog, err = libbpf_available_kallsyms_parse(&res); if (err) goto error; + + unique_match = OPTS_GET(opts, unique_match, false); + if (unique_match && res.cnt != 1) { + pr_warn("prog '%s': failed to find unique match: cnt %lu\n", + prog->name, res.cnt); + return libbpf_err_ptr(-EINVAL); + } + addrs = res.addrs; cnt = res.cnt; } diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h index d45807103565..3020ee45303a 100644 --- a/tools/lib/bpf/libbpf.h +++ b/tools/lib/bpf/libbpf.h @@ -552,10 +552,12 @@ struct bpf_kprobe_multi_opts { bool retprobe; /* create session kprobes */ bool session; + /* enforce unique match */ + bool unique_match; size_t :0; }; -#define bpf_kprobe_multi_opts__last_field session +#define bpf_kprobe_multi_opts__last_field unique_match LIBBPF_API struct bpf_link * bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
Jordan reported an issue in Meta production environment where func try_to_wake_up() is renamed to try_to_wake_up.llvm.<hash>() by clang compiler at lto mode. The original 'kprobe/try_to_wake_up' does not work any more since try_to_wake_up() does not match the actual func name in /proc/kallsyms. There are a couple of ways to resolve this issue. For example, in attach_kprobe(), we could do lookup in /proc/kallsyms so try_to_wake_up() can be replaced by try_to_wake_up.llvm.<hach>(). Or we can force users to use bpf_program__attach_kprobe() where they need to lookup /proc/kallsyms to find out try_to_wake_up.llvm.<hach>(). But these two approaches requires extra work by either libbpf or user. Luckily, suggested by Andrii, multi kprobe already supports wildcard ('*') for symbol matching. In the above example, 'try_to_wake_up*' can match to try_to_wake_up() or try_to_wake_up.llvm.<hash>() and this allows bpf prog works for different kernels as some kernels may have try_to_wake_up() and some others may have try_to_wake_up.llvm.<hash>(). The original intention is to kprobe try_to_wake_up() only, so an optional field unique_match is added to struct bpf_kprobe_multi_opts. If the field is set to true, the number of matched functions must be one. Otherwise, the attachment will fail. In the above case, multi kprobe with 'try_to_wake_up*' and unique_match preserves user functionality. Reported-by: Jordan Rome <linux@jordanrome.com> Suggested-by: Andrii Nakryiko <andrii@kernel.org> Signed-off-by: Yonghong Song <yonghong.song@linux.dev> --- tools/lib/bpf/libbpf.c | 10 +++++++++- tools/lib/bpf/libbpf.h | 4 +++- 2 files changed, 12 insertions(+), 2 deletions(-)