On Tue, Jan 7, 2025 at 10:29 AM Yonghong Song <yonghong.song@xxxxxxxxx> wrote:
>
>
>
>
> On 1/6/25 4:24 PM, Andrii Nakryiko wrote:
> > On Wed, Dec 18, 2024 at 2:53 PM Yonghong Song <yonghong.song@xxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >> Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> >> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx>
> >> ---
> >> 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
> >>
>
