Re: [PATCHv3 bpf-next 15/26] libbpf: Add uprobe multi link detection

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Jul 06, 2023 at 09:20:42PM -0700, Andrii Nakryiko wrote:
> On Fri, Jun 30, 2023 at 1:36 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Adding uprobe-multi link detection. It will be used later in
> > bpf_program__attach_usdt function to check and use uprobe_multi
> > link over standard uprobe links.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/bpf/libbpf.c          | 35 +++++++++++++++++++++++++++++++++
> >  tools/lib/bpf/libbpf_internal.h |  2 ++
> >  2 files changed, 37 insertions(+)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 06092b9752f1..4f61f9dc1748 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -4817,6 +4817,38 @@ static int probe_perf_link(void)
> >         return link_fd < 0 && err == -EBADF;
> >  }
> >
> > +static int probe_uprobe_multi_link(void)
> > +{
> > +       LIBBPF_OPTS(bpf_prog_load_opts, load_opts,
> > +               .expected_attach_type = BPF_TRACE_UPROBE_MULTI,
> > +       );
> > +       LIBBPF_OPTS(bpf_link_create_opts, link_opts);
> > +       struct bpf_insn insns[] = {
> > +               BPF_MOV64_IMM(BPF_REG_0, 0),
> > +               BPF_EXIT_INSN(),
> > +       };
> > +       unsigned long offset = 0;
> > +       int prog_fd, link_fd;
> > +
> > +       prog_fd = bpf_prog_load(BPF_PROG_TYPE_KPROBE, NULL, "GPL",
> > +                               insns, ARRAY_SIZE(insns), &load_opts);
> > +       if (prog_fd < 0)
> > +               return -errno;
> > +
> > +       /* create single uprobe on offset 0 in current process */
> > +       link_opts.uprobe_multi.path = "/proc/self/exe";
> > +       link_opts.uprobe_multi.offsets = &offset;
> > +       link_opts.uprobe_multi.cnt = 1;
> > +
> > +       link_fd = bpf_link_create(prog_fd, -1, BPF_TRACE_UPROBE_MULTI, &link_opts);
> > +
> 
> so I'd like us to avoid successfully attaching anything. This might
> have unintended consequences (e.g., unintentionally breaking backing
> huge pages into normal pages, just because we happen to successfully
> attach briefly). So let's work on feature detection that fails to
> create a link, but does it in a way that we know that the feature
> itself is supported by the kernel
> 
> Some ideas we could do:
> 
> 1. Pass invalid file (e.g., just root, "/" as path), but modify
> kernel-side logic to return not -EINVAL, but -EBADF (and I think it
> would be good to do this anyway). Then expect -EBADF as a signal that
> the feature is supported.

ah ok, so -EBADF from inside uprobe_multi link setup code would mean
it's supported, anything else means it's not.. should work also for
old kernels, I don't think we have -EBADF in related paths, will check

> 
> 2. Also, we can return -EPROTO instead of -EINVAL on invalid
> combination of paramers or something like that
> 
> I'd start with -EBADF change.
> 
> In general, we should write kernel-side code in such a way that allows
> simple and efficient feature-detection. We shouldn't repeat the
> nightmare of memcg-based mem accounting :(
> 
> > +       if (link_fd >= 0)
> > +               close(link_fd);
> > +       close(prog_fd);
> > +
> > +       return link_fd >= 0;
> > +}
> > +
> >  static int probe_kern_bpf_cookie(void)
> >  {
> >         struct bpf_insn insns[] = {
> > @@ -4913,6 +4945,9 @@ static struct kern_feature_desc {
> >         [FEAT_SYSCALL_WRAPPER] = {
> >                 "Kernel using syscall wrapper", probe_kern_syscall_wrapper,
> >         },
> > +       [FEAT_UPROBE_MULTI_LINK] = {
> > +               "BPF uprobe multi link support", probe_uprobe_multi_link,
> 
> nit: BPF multi-uprobe link support
> 
> > +       },
> >  };
> >
> >  bool kernel_supports(const struct bpf_object *obj, enum kern_feature_id feat_id)
> > diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
> > index 7d75b92e531a..9c04b3fe1207 100644
> > --- a/tools/lib/bpf/libbpf_internal.h
> > +++ b/tools/lib/bpf/libbpf_internal.h
> > @@ -354,6 +354,8 @@ enum kern_feature_id {
> >         FEAT_BTF_ENUM64,
> >         /* Kernel uses syscall wrapper (CONFIG_ARCH_HAS_SYSCALL_WRAPPER) */
> >         FEAT_SYSCALL_WRAPPER,
> > +       /* BPF uprobe_multi link support */
> 
> same, multi-uprobe link support

ok, thanks

jirka

> 
> 
> 
> > +       FEAT_UPROBE_MULTI_LINK,
> >         __FEAT_CNT,
> >  };
> >
> > --
> > 2.41.0
> >