| Message ID | 20231120145639.3179656-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 11/20/23 9:56 AM, Jiri Olsa 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 | 72 ++++++++++++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 10 +++++ > 3 files changed, 92 insertions(+) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 7a5498242eaa..a63b5eb7f9ec 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -6562,6 +6562,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; /* in/out: real path size on success */ > + __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 ad0323f27288..ca453b642819 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -3044,6 +3044,7 @@ struct bpf_uprobe_multi_link { > u32 cnt; > struct bpf_uprobe *uprobes; > struct task_struct *task; > + u32 flags; > }; > > struct bpf_uprobe_multi_run_ctx { > @@ -3083,9 +3084,79 @@ 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 || uref_ctr_offsets || ucookies) && !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; Should we just return PTR_ERR(p)? In d_path, it is possible that -ENAMETOOLONG is returned. But path->dentry->d_op->d_dname() might return a different error reason than -ENAMETOOLONG or -ENOSPC? > + } > + upath_size = buf + upath_size - p; > + left = copy_to_user(upath, p, upath_size); Here, the data copied to user may contain more than actual path itself. I am okay with this since this is not in critical path. But early buf allocation is using kmalloc whose content could be arbitrary. Should we use kzalloc for the above 'buf' allocation? > + kfree(buf); > + if (left) > + return -EFAULT; > + info->uprobe_multi.path_size = upath_size - 1 /* NULL */; > + } > + > + if (!uoffsets && !ucookies && !uref_ctr_offsets) > + return 0; > + > + if (ucount < umulti_link->cnt) > + err = -ENOSPC; > + else > + ucount = umulti_link->cnt; > + > + for (i = 0; i < ucount; i++) { > + if (uoffsets && > + 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; > +} > + > [...]
On Mon, Nov 20, 2023 at 6:57 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 | 72 ++++++++++++++++++++++++++++++++++ > tools/include/uapi/linux/bpf.h | 10 +++++ > 3 files changed, 92 insertions(+) > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > index 7a5498242eaa..a63b5eb7f9ec 100644 > --- a/include/uapi/linux/bpf.h > +++ b/include/uapi/linux/bpf.h > @@ -6562,6 +6562,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; /* in/out: real path size on success */ > + __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 ad0323f27288..ca453b642819 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -3044,6 +3044,7 @@ struct bpf_uprobe_multi_link { > u32 cnt; > struct bpf_uprobe *uprobes; > struct task_struct *task; > + u32 flags; this fits better after cnt to avoid increasing the size of bpf_uprobe_multi_link, please it move up > }; > > struct bpf_uprobe_multi_run_ctx { > @@ -3083,9 +3084,79 @@ 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 || uref_ctr_offsets || ucookies) && !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; > + } > + upath_size = buf + upath_size - p; > + left = copy_to_user(upath, p, upath_size); > + kfree(buf); > + if (left) > + return -EFAULT; > + info->uprobe_multi.path_size = upath_size - 1 /* NULL */; why subtract zero terminating byte? I think we should drop this -1 and return filled out buffer content size, including zero terminator. > + } > + > + if (!uoffsets && !ucookies && !uref_ctr_offsets) > + return 0; > + > + if (ucount < umulti_link->cnt) > + err = -ENOSPC; > + else > + ucount = umulti_link->cnt; > + > + for (i = 0; i < ucount; i++) { > + if (uoffsets && > + 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, > @@ -3274,6 +3345,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 7a5498242eaa..a63b5eb7f9ec 100644 > --- a/tools/include/uapi/linux/bpf.h > +++ b/tools/include/uapi/linux/bpf.h > @@ -6562,6 +6562,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; /* in/out: real path size on success */ > + __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.42.0 >
On Tue, Nov 21, 2023 at 10:41:24AM -0800, Andrii Nakryiko wrote: > On Mon, Nov 20, 2023 at 6:57 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 | 72 ++++++++++++++++++++++++++++++++++ > > tools/include/uapi/linux/bpf.h | 10 +++++ > > 3 files changed, 92 insertions(+) > > > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index 7a5498242eaa..a63b5eb7f9ec 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -6562,6 +6562,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; /* in/out: real path size on success */ > > + __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 ad0323f27288..ca453b642819 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -3044,6 +3044,7 @@ struct bpf_uprobe_multi_link { > > u32 cnt; > > struct bpf_uprobe *uprobes; > > struct task_struct *task; > > + u32 flags; > > this fits better after cnt to avoid increasing the size of > bpf_uprobe_multi_link, please it move up ok > > > }; > > > > struct bpf_uprobe_multi_run_ctx { > > @@ -3083,9 +3084,79 @@ 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 || uref_ctr_offsets || ucookies) && !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; > > + } > > + upath_size = buf + upath_size - p; > > + left = copy_to_user(upath, p, upath_size); > > + kfree(buf); > > + if (left) > > + return -EFAULT; > > + info->uprobe_multi.path_size = upath_size - 1 /* NULL */; > > why subtract zero terminating byte? I think we should drop this -1 and > return filled out buffer content size, including zero terminator. I wanted to return the same as strlen would: The strlen() function calculates the length of the string pointed to by s, excluding the terminating null byte ('\0'). either way works for me, but perhaps we should document it in the uapi header jirka
On Mon, Nov 20, 2023 at 10:04:16AM -0800, Yonghong Song wrote: SNIP > > +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 || uref_ctr_offsets || ucookies) && !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; > > Should we just return PTR_ERR(p)? In d_path, it is possible that > -ENAMETOOLONG is returned. But path->dentry->d_op->d_dname() might > return a different error reason than -ENAMETOOLONG or -ENOSPC? true, will change > > > + } > > + upath_size = buf + upath_size - p; > > + left = copy_to_user(upath, p, upath_size); > > Here, the data copied to user may contain more than > actual path itself. I am okay with this since this > is not in critical path. But early buf allocation is using > kmalloc whose content could be arbitrary. Should we > use kzalloc for the above 'buf' allocation? good catch, will use kzalloc thanks, jirka
On Wed, Nov 22, 2023 at 10:50:06PM +0100, Jiri Olsa wrote: > On Mon, Nov 20, 2023 at 10:04:16AM -0800, Yonghong Song wrote: > > SNIP > > > > +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 || uref_ctr_offsets || ucookies) && !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; > > > > Should we just return PTR_ERR(p)? In d_path, it is possible that > > -ENAMETOOLONG is returned. But path->dentry->d_op->d_dname() might > > return a different error reason than -ENAMETOOLONG or -ENOSPC? > > true, will change > > > > > > + } > > > + upath_size = buf + upath_size - p; > > > + left = copy_to_user(upath, p, upath_size); > > > > Here, the data copied to user may contain more than > > actual path itself. I am okay with this since this > > is not in critical path. But early buf allocation is using > > kmalloc whose content could be arbitrary. Should we > > use kzalloc for the above 'buf' allocation? > > good catch, will use kzalloc hum, actually.. after checking d_path IIUC it copies into the end of buffer, so I can't see this code copying more data to user buffer jirka
On 11/23/23 4:20 AM, Jiri Olsa wrote: > On Wed, Nov 22, 2023 at 10:50:06PM +0100, Jiri Olsa wrote: >> On Mon, Nov 20, 2023 at 10:04:16AM -0800, Yonghong Song wrote: >> >> SNIP >> >>>> +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 || uref_ctr_offsets || ucookies) && !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; >>> Should we just return PTR_ERR(p)? In d_path, it is possible that >>> -ENAMETOOLONG is returned. But path->dentry->d_op->d_dname() might >>> return a different error reason than -ENAMETOOLONG or -ENOSPC? >> true, will change >> >>>> + } >>>> + upath_size = buf + upath_size - p; >>>> + left = copy_to_user(upath, p, upath_size); >>> Here, the data copied to user may contain more than >>> actual path itself. I am okay with this since this >>> is not in critical path. But early buf allocation is using >>> kmalloc whose content could be arbitrary. Should we >>> use kzalloc for the above 'buf' allocation? >> good catch, will use kzalloc > hum, actually.. after checking d_path IIUC it copies into the end of buffer, > so I can't see this code copying more data to user buffer Double checked as well. Indeed, the path is copied to the end of buffer, so kmalloc() should be okay. Sorry for noise.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index 7a5498242eaa..a63b5eb7f9ec 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -6562,6 +6562,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; /* in/out: real path size on success */ + __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 ad0323f27288..ca453b642819 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -3044,6 +3044,7 @@ struct bpf_uprobe_multi_link { u32 cnt; struct bpf_uprobe *uprobes; struct task_struct *task; + u32 flags; }; struct bpf_uprobe_multi_run_ctx { @@ -3083,9 +3084,79 @@ 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 || uref_ctr_offsets || ucookies) && !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; + } + upath_size = buf + upath_size - p; + left = copy_to_user(upath, p, upath_size); + kfree(buf); + if (left) + return -EFAULT; + info->uprobe_multi.path_size = upath_size - 1 /* NULL */; + } + + if (!uoffsets && !ucookies && !uref_ctr_offsets) + return 0; + + if (ucount < umulti_link->cnt) + err = -ENOSPC; + else + ucount = umulti_link->cnt; + + for (i = 0; i < ucount; i++) { + if (uoffsets && + 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, @@ -3274,6 +3345,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 7a5498242eaa..a63b5eb7f9ec 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -6562,6 +6562,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; /* in/out: real path size on success */ + __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 | 72 ++++++++++++++++++++++++++++++++++ tools/include/uapi/linux/bpf.h | 10 +++++ 3 files changed, 92 insertions(+)