Re: [PATCHv3 bpf-next 13/26] libbpf: Add bpf_program__attach_uprobe_multi function

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, Jul 06, 2023 at 09:05:23PM -0700, Andrii Nakryiko wrote:

SNIP

> > +       if (!OPTS_VALID(opts, bpf_uprobe_multi_opts))
> > +               return libbpf_err_ptr(-EINVAL);
> > +
> > +       syms = OPTS_GET(opts, syms, NULL);
> > +       offsets = OPTS_GET(opts, offsets, NULL);
> > +       ref_ctr_offsets = OPTS_GET(opts, ref_ctr_offsets, NULL);
> > +       cookies = OPTS_GET(opts, cookies, NULL);
> > +       cnt = OPTS_GET(opts, cnt, 0);
> > +
> > +       /*
> > +        * User can specify 2 mutually exclusive set of inputs:
> > +        *
> > +        * 1) use only path/func_pattern/pid arguments
> > +        *
> > +        * 2) use path/pid with allowed combinations of:
> > +        *    syms/offsets/ref_ctr_offsets/cookies/cnt
> > +        *
> > +        *    - syms and offsets are mutually exclusive
> > +        *    - ref_ctr_offsets and cookies are optional
> > +        *
> > +        * Any other usage results in error.
> > +        */
> > +
> > +       if (!path && !func_pattern && !cnt)
> 
> weird, I'd expect separate if (!path) return error (already bad,
> regardless of func_pattern or cnt)
> 
> then if (!func_pattern && cnt == 0) return error
> 
> > +               return libbpf_err_ptr(-EINVAL);
> > +       if (func_pattern && !path)
> > +               return libbpf_err_ptr(-EINVAL);
> > +
> > +       has_pattern = path && func_pattern;
> 
> this and above check must be some leftovers from previous version.
> path should always be present. and so you don't need has_pattern
> variable, just use "func_pattern" check

hum, right, previous version had 2 paths, now there's just one,
I'll change that together with the suggested change above

> 
> > +
> > +       if (has_pattern) {
> > +               if (syms || offsets || ref_ctr_offsets || cookies || cnt)
> > +                       return libbpf_err_ptr(-EINVAL);
> > +       } else {
> > +               if (!cnt)
> > +                       return libbpf_err_ptr(-EINVAL);
> > +               if (!!syms == !!offsets)
> > +                       return libbpf_err_ptr(-EINVAL);
> > +       }
> > +
> > +       if (has_pattern) {
> > +               if (!strchr(path, '/')) {
> > +                       err = resolve_full_path(path, full_path, sizeof(full_path));
> > +                       if (err) {
> > +                               pr_warn("prog '%s': failed to resolve full path for '%s': %d\n",
> > +                                       prog->name, path, err);
> > +                               return libbpf_err_ptr(err);
> > +                       }
> > +                       path = full_path;
> > +               }
> > +
> > +               err = elf_resolve_pattern_offsets(path, func_pattern,
> > +                                                 &resolved_offsets, &cnt);
> > +               if (err < 0)
> > +                       return libbpf_err_ptr(err);
> > +               offsets = resolved_offsets;
> > +       } else if (syms) {
> > +               err = elf_resolve_syms_offsets(path, cnt, syms, &resolved_offsets);
> > +               if (err < 0)
> > +                       return libbpf_err_ptr(err);
> > +               offsets = resolved_offsets;
> 
> you can extract this common error checking and `offsets =
> resolved_offsets;` to after if, it's common for both branches

not sure what you mean in here, offsets can be also provided
by OPTS_GET(opts, offsets, NULL) earlier

> > +       }
> > +
> > +       retprobe = OPTS_GET(opts, retprobe, false);
> > +
> > +       lopts.uprobe_multi.path = path;
> > +       lopts.uprobe_multi.offsets = offsets;
> > +       lopts.uprobe_multi.ref_ctr_offsets = ref_ctr_offsets;
> > +       lopts.uprobe_multi.cookies = cookies;
> > +       lopts.uprobe_multi.cnt = cnt;
> > +       lopts.uprobe_multi.flags = retprobe ? BPF_F_UPROBE_MULTI_RETURN : 0;
> 
> retprobe is another unnecessary var, just inline check here to keep it simpler

ok

> 
> > +
> > +       if (pid == 0)
> > +               pid = getpid();
> > +       if (pid > 0)
> > +               lopts.uprobe_multi.pid = pid;
> > +
> > +       link = calloc(1, sizeof(*link));
> > +       if (!link) {
> > +               err = -ENOMEM;
> > +               goto error;
> > +       }
> > +       link->detach = &bpf_link__detach_fd;
> > +
> > +       prog_fd = bpf_program__fd(prog);
> > +       link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &lopts);
> > +       if (link_fd < 0) {
> > +               err = -errno;
> > +               pr_warn("prog '%s': failed to attach: %s\n",
> 
> "failed to attach multi-uprobe"? We probably should have added "failed
> to attach multi-kprobe" in bpf_program__attach_kprobe_multi_opts as
> well?

ook, will add

> 
> > +                       prog->name, libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> > +               goto error;
> > +       }
> > +       link->fd = link_fd;
> > +       free(resolved_offsets);
> > +       return link;
> > +
> > +error:
> > +       free(resolved_offsets);
> > +       free(link);
> > +       return libbpf_err_ptr(err);
> > +}
> > +
> >  LIBBPF_API struct bpf_link *
> >  bpf_program__attach_uprobe_opts(const struct bpf_program *prog, pid_t pid,
> >                                 const char *binary_path, size_t func_offset,
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 754da73c643b..7c218f610210 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -529,6 +529,33 @@ bpf_program__attach_kprobe_multi_opts(const struct bpf_program *prog,
> >                                       const char *pattern,
> >                                       const struct bpf_kprobe_multi_opts *opts);
> >
> > +struct bpf_uprobe_multi_opts {
> > +       /* size of this struct, for forward/backward compatibility */
> > +       size_t sz;
> > +       /* array of function symbols to attach */
> 
> attach to?

ok

> 
> > +       const char **syms;
> > +       /* array of function addresses to attach */
> 
> attach to?

ook

> 
> > +       const unsigned long *offsets;
> > +       /* array of refctr offsets to attach */
> 
> we don't really attach to ref counters, so maybe "optional, array of
> associated ref counter offsets" or something along those lines ?

ok

> 
> > +       const unsigned long *ref_ctr_offsets;
> > +       /* array of user-provided values fetchable through bpf_get_attach_cookie */
> 
> "array of associated BPF cookies"? we can't keep explaining what BPF
> cookie is in every possible API :)

ook

> 
> > +       const __u64 *cookies;
> > +       /* number of elements in syms/addrs/cookies arrays */
> > +       size_t cnt;
> > +       /* create return uprobes */
> > +       bool retprobe;
> > +       size_t :0;
> > +};
> > +
> > +#define bpf_uprobe_multi_opts__last_field retprobe
> > +
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
> > +                                pid_t pid,
> > +                                const char *binary_path,
> > +                                const char *func_pattern,
> > +                                const struct bpf_uprobe_multi_opts *opts);
> > +
> 
> ok, let's be good citizens and add documentation for this new API.
> Those comments about valid combinations belong here as well. Please
> take a look at existing doccomments for the format and conventions.
> Thanks!

ok, will add

thanks,
jirka