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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
> >
