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
