| Message ID | 20231109092838.721233-4-jolsa@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: Add link_info support for uprobe multi link | expand |
On Thu, Nov 9, 2023 at 1:29 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding support to get uprobe_link details through bpf_link_info > interface. > > Adding new struct uprobe_multi to struct bpf_link_info to carry > the uprobe_multi link details. > > The uprobe_multi.count is passed from user space to denote size > of array fields (offsets/ref_ctr_offsets/cookies). The actual > array size is stored back to uprobe_multi.count (allowing user > to find out the actual array size) and array fields are populated > up to the user passed size. > > All the non-array fields (path/count/flags/pid) are always set. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/uapi/linux/bpf.h | 10 +++++ > kernel/trace/bpf_trace.c | 69 ++++++++++++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 10 +++++ > 3 files changed, 89 insertions(+) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 0f6cdf52b1da..05b355da4508 100644 > --- a/include/uapi/linux/bpf.h > +++ b/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; > 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? > + 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? > + } > + > + 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? > + > + 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 >
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 > > > + 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 > > > + } > > + > > + 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 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 > >
On Fri, Nov 10, 2023 at 1:01 AM Jiri Olsa <olsajiri@gmail.com> 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 > > >
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 0f6cdf52b1da..05b355da4508 100644 --- a/include/uapi/linux/bpf.h +++ b/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; 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) + 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; + } + + if (!uoffsets) + return 0; + + 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;
Adding support to get uprobe_link details through bpf_link_info interface. Adding new struct uprobe_multi to struct bpf_link_info to carry the uprobe_multi link details. The uprobe_multi.count is passed from user space to denote size of array fields (offsets/ref_ctr_offsets/cookies). The actual array size is stored back to uprobe_multi.count (allowing user to find out the actual array size) and array fields are populated up to the user passed size. All the non-array fields (path/count/flags/pid) are always set. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/uapi/linux/bpf.h | 10 +++++ kernel/trace/bpf_trace.c | 69 ++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 10 +++++ 3 files changed, 89 insertions(+)