On Tue, Jul 11, 2023 at 2:05 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> 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
ah, I missed that it's else if, not just else. Never mind, it's good as is then.
>
> > > + }
> > > +
> > > + 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
