On Fri, Nov 10, 2023 at 1:01 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Thu, Nov 09, 2023 at 09:57:03PM -0800, Andrii Nakryiko wrote:
>
> SNIP
>
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 52c1ec3a0467..1ea54f3b3f73 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -3046,6 +3046,7 @@ struct bpf_uprobe_multi_link {
> > > u32 cnt;
> > > struct bpf_uprobe *uprobes;
> > > struct task_struct *task;
> > > + u32 flags;
> > > };
> > >
> > > struct bpf_uprobe_multi_run_ctx {
> > > @@ -3085,9 +3086,76 @@ static void bpf_uprobe_multi_link_dealloc(struct bpf_link *link)
> > > kfree(umulti_link);
> > > }
> > >
> > > +static int bpf_uprobe_multi_link_fill_link_info(const struct bpf_link *link,
> > > + struct bpf_link_info *info)
> > > +{
> > > + u64 __user *uref_ctr_offsets = u64_to_user_ptr(info->uprobe_multi.ref_ctr_offsets);
> > > + u64 __user *ucookies = u64_to_user_ptr(info->uprobe_multi.cookies);
> > > + u64 __user *uoffsets = u64_to_user_ptr(info->uprobe_multi.offsets);
> > > + u64 __user *upath = u64_to_user_ptr(info->uprobe_multi.path);
> > > + u32 upath_size = info->uprobe_multi.path_size;
> > > + struct bpf_uprobe_multi_link *umulti_link;
> > > + u32 ucount = info->uprobe_multi.count;
> > > + int err = 0, i;
> > > + long left;
> > > +
> > > + if (!upath ^ !upath_size)
> > > + return -EINVAL;
> > > +
> > > + if (!uoffsets ^ !ucount)
> >
> > uoffsets is not the only one that requires ucount, right?
>
> yep, cookies as well
so I think all those arrays should be treated as completely
independent and optional. So if any of ref_ctr_offsets, cookies, or
offsets are requested, then count should be non-zero.
>
> >
> > > + return -EINVAL;
> > > +
> > > + umulti_link = container_of(link, struct bpf_uprobe_multi_link, link);
> > > + info->uprobe_multi.count = umulti_link->cnt;
> > > + info->uprobe_multi.flags = umulti_link->flags;
> > > + info->uprobe_multi.pid = umulti_link->task ?
> > > + task_pid_nr_ns(umulti_link->task, task_active_pid_ns(current)) : 0;
> > > +
> > > + if (upath) {
> > > + char *p, *buf;
> > > +
> > > + upath_size = min_t(u32, upath_size, PATH_MAX);
> > > +
> > > + buf = kmalloc(upath_size, GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > + p = d_path(&umulti_link->path, buf, upath_size);
> > > + if (IS_ERR(p)) {
> > > + kfree(buf);
> > > + return -ENOSPC;
> > > + }
> > > + left = copy_to_user(upath, p, buf + upath_size - p);
> > > + kfree(buf);
> > > + if (left)
> > > + return -EFAULT;
> >
> > hmm.. I expected the actual path_size to be reported back to the
> > user?.. Is there a problem with doing that?
>
> we return back the string, if the string fits in provided buffer it's
> terminated with 0 and user space can do strlen on it if needed
sure, but we can also specify the exact size. We know if, what's the
problem with that? It's just basically saying that path_size is in/out
parameter, just like count
>
> >
> > > + }
> > > +
> > > + if (!uoffsets)
> > > + return 0;
> >
> > why guard by uoffsets? what if users only wanted cookies? I think each
> > array should do its own checking and be independent, no?
>
> I did not think of the use case to get just the cookies (at least not the
> one in bpftool), I saw it as optional to offsets, which is mandatory..
> but that should be an easy change I think
yeah, let's not bake in any assumptions. Each array is optional, user
should be able to request any or all of them. Having this dependency
on offsets is confusing from user POV.
>
> jirka
>
> >
> > > +
> > > + if (ucount < umulti_link->cnt)
> > > + err = -ENOSPC;
> > > + else
> > > + ucount = umulti_link->cnt;
> > > +
> > > + for (i = 0; i < ucount; i++) {
> > > + if (put_user(umulti_link->uprobes[i].offset, uoffsets + i))
> > > + return -EFAULT;
> > > + if (uref_ctr_offsets &&
> > > + put_user(umulti_link->uprobes[i].ref_ctr_offset, uref_ctr_offsets + i))
> > > + return -EFAULT;
> > > + if (ucookies &&
> > > + put_user(umulti_link->uprobes[i].cookie, ucookies + i))
> > > + return -EFAULT;
> > > + }
> > > +
> > > + return err;
> > > +}
> > > +
> > > static const struct bpf_link_ops bpf_uprobe_multi_link_lops = {
> > > .release = bpf_uprobe_multi_link_release,
> > > .dealloc = bpf_uprobe_multi_link_dealloc,
> > > + .fill_link_info = bpf_uprobe_multi_link_fill_link_info,
> > > };
> > >
> > > static int uprobe_prog_run(struct bpf_uprobe *uprobe,
> > > @@ -3276,6 +3344,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > > link->uprobes = uprobes;
> > > link->path = path;
> > > link->task = task;
> > > + link->flags = flags;
> > >
> > > bpf_link_init(&link->link, BPF_LINK_TYPE_UPROBE_MULTI,
> > > &bpf_uprobe_multi_link_lops, prog);
> > > diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
> > > index 0f6cdf52b1da..05b355da4508 100644
> > > --- a/tools/include/uapi/linux/bpf.h
> > > +++ b/tools/include/uapi/linux/bpf.h
> > > @@ -6556,6 +6556,16 @@ struct bpf_link_info {
> > > __u32 flags;
> > > __u64 missed;
> > > } kprobe_multi;
> > > + struct {
> > > + __aligned_u64 path;
> > > + __aligned_u64 offsets;
> > > + __aligned_u64 ref_ctr_offsets;
> > > + __aligned_u64 cookies;
> > > + __u32 path_size;
> > > + __u32 count; /* in/out: uprobe_multi offsets/ref_ctr_offsets/cookies count */
> > > + __u32 flags;
> > > + __u32 pid;
> > > + } uprobe_multi;
> > > struct {
> > > __u32 type; /* enum bpf_perf_event_type */
> > > __u32 :32;
> > > --
> > > 2.41.0
> > >
