* [PATCH bpf-next 1/6] libbpf: Add st_type argument to elf_resolve_syms_offsets function
2023-10-25 20:24 [PATCH bpf-next 0/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
@ 2023-10-25 20:24 ` Jiri Olsa
2023-10-26 16:29 ` Song Liu
2023-10-25 20:24 ` [PATCH bpf-next 2/6] bpf: Store ref_ctr_offsets values in bpf_uprobe array Jiri Olsa
` (4 subsequent siblings)
5 siblings, 1 reply; 33+ messages in thread
From: Jiri Olsa @ 2023-10-25 20:24 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
We need to get offsets for static variables in following changes,
so making elf_resolve_syms_offsets to take st_type value as argument
and passing it to elf_sym_iter_new.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/bpf/elf.c | 5 +++--
tools/lib/bpf/libbpf.c | 2 +-
tools/lib/bpf/libbpf_internal.h | 3 ++-
tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c | 2 +-
4 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/tools/lib/bpf/elf.c b/tools/lib/bpf/elf.c
index 2a62bf411bb3..b02faec748a5 100644
--- a/tools/lib/bpf/elf.c
+++ b/tools/lib/bpf/elf.c
@@ -407,7 +407,8 @@ static int symbol_cmp(const void *a, const void *b)
* size, that needs to be released by the caller.
*/
int elf_resolve_syms_offsets(const char *binary_path, int cnt,
- const char **syms, unsigned long **poffsets)
+ const char **syms, unsigned long **poffsets,
+ int st_type)
{
int sh_types[2] = { SHT_DYNSYM, SHT_SYMTAB };
int err = 0, i, cnt_done = 0;
@@ -438,7 +439,7 @@ int elf_resolve_syms_offsets(const char *binary_path, int cnt,
struct elf_sym_iter iter;
struct elf_sym *sym;
- err = elf_sym_iter_new(&iter, elf_fd.elf, binary_path, sh_types[i], STT_FUNC);
+ err = elf_sym_iter_new(&iter, elf_fd.elf, binary_path, sh_types[i], st_type);
if (err == -ENOENT)
continue;
if (err)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index e067be95da3c..ea9b8158c20d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -11447,7 +11447,7 @@ bpf_program__attach_uprobe_multi(const struct bpf_program *prog,
return libbpf_err_ptr(err);
offsets = resolved_offsets;
} else if (syms) {
- err = elf_resolve_syms_offsets(path, cnt, syms, &resolved_offsets);
+ err = elf_resolve_syms_offsets(path, cnt, syms, &resolved_offsets, STT_FUNC);
if (err < 0)
return libbpf_err_ptr(err);
offsets = resolved_offsets;
diff --git a/tools/lib/bpf/libbpf_internal.h b/tools/lib/bpf/libbpf_internal.h
index f0f08635adb0..b5d334754e5d 100644
--- a/tools/lib/bpf/libbpf_internal.h
+++ b/tools/lib/bpf/libbpf_internal.h
@@ -594,7 +594,8 @@ int elf_open(const char *binary_path, struct elf_fd *elf_fd);
void elf_close(struct elf_fd *elf_fd);
int elf_resolve_syms_offsets(const char *binary_path, int cnt,
- const char **syms, unsigned long **poffsets);
+ const char **syms, unsigned long **poffsets,
+ int st_type);
int elf_resolve_pattern_offsets(const char *binary_path, const char *pattern,
unsigned long **poffsets, size_t *pcnt);
diff --git a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
index cd051d3901a9..ece260cf2c0b 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -249,7 +249,7 @@ static void __test_link_api(struct child *child)
int link_extra_fd = -1;
int err;
- err = elf_resolve_syms_offsets(path, 3, syms, (unsigned long **) &offsets);
+ err = elf_resolve_syms_offsets(path, 3, syms, (unsigned long **) &offsets, STT_FUNC);
if (!ASSERT_OK(err, "elf_resolve_syms_offsets"))
return;
--
2.41.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 1/6] libbpf: Add st_type argument to elf_resolve_syms_offsets function
2023-10-25 20:24 ` [PATCH bpf-next 1/6] libbpf: Add st_type argument to elf_resolve_syms_offsets function Jiri Olsa
@ 2023-10-26 16:29 ` Song Liu
0 siblings, 0 replies; 33+ messages in thread
From: Song Liu @ 2023-10-26 16:29 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> We need to get offsets for static variables in following changes,
> so making elf_resolve_syms_offsets to take st_type value as argument
> and passing it to elf_sym_iter_new.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Song Liu <song@kernel.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next 2/6] bpf: Store ref_ctr_offsets values in bpf_uprobe array
2023-10-25 20:24 [PATCH bpf-next 0/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
2023-10-25 20:24 ` [PATCH bpf-next 1/6] libbpf: Add st_type argument to elf_resolve_syms_offsets function Jiri Olsa
@ 2023-10-25 20:24 ` Jiri Olsa
2023-10-26 16:31 ` Song Liu
2023-10-25 20:24 ` [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
` (3 subsequent siblings)
5 siblings, 1 reply; 33+ messages in thread
From: Jiri Olsa @ 2023-10-25 20:24 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
We will need to return ref_ctr_offsets values through link_info
interface in following change, so we need to keep them around.
Storing ref_ctr_offsets values directly into bpf_uprobe array.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/trace/bpf_trace.c | 14 +++-----------
1 file changed, 3 insertions(+), 11 deletions(-)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index df697c74d519..843b3846d3f8 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3031,6 +3031,7 @@ struct bpf_uprobe_multi_link;
struct bpf_uprobe {
struct bpf_uprobe_multi_link *link;
loff_t offset;
+ unsigned long ref_ctr_offset;
u64 cookie;
struct uprobe_consumer consumer;
};
@@ -3170,7 +3171,6 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
{
struct bpf_uprobe_multi_link *link = NULL;
unsigned long __user *uref_ctr_offsets;
- unsigned long *ref_ctr_offsets = NULL;
struct bpf_link_primer link_primer;
struct bpf_uprobe *uprobes = NULL;
struct task_struct *task = NULL;
@@ -3243,18 +3243,12 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
if (!uprobes || !link)
goto error_free;
- if (uref_ctr_offsets) {
- ref_ctr_offsets = kvcalloc(cnt, sizeof(*ref_ctr_offsets), GFP_KERNEL);
- if (!ref_ctr_offsets)
- goto error_free;
- }
-
for (i = 0; i < cnt; i++) {
if (ucookies && __get_user(uprobes[i].cookie, ucookies + i)) {
err = -EFAULT;
goto error_free;
}
- if (uref_ctr_offsets && __get_user(ref_ctr_offsets[i], uref_ctr_offsets + i)) {
+ if (uref_ctr_offsets && __get_user(uprobes[i].ref_ctr_offset, uref_ctr_offsets + i)) {
err = -EFAULT;
goto error_free;
}
@@ -3285,7 +3279,7 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
for (i = 0; i < cnt; i++) {
err = uprobe_register_refctr(d_real_inode(link->path.dentry),
uprobes[i].offset,
- ref_ctr_offsets ? ref_ctr_offsets[i] : 0,
+ uprobes[i].ref_ctr_offset,
&uprobes[i].consumer);
if (err) {
bpf_uprobe_unregister(&path, uprobes, i);
@@ -3297,11 +3291,9 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
if (err)
goto error_free;
- kvfree(ref_ctr_offsets);
return bpf_link_settle(&link_primer);
error_free:
- kvfree(ref_ctr_offsets);
kvfree(uprobes);
kfree(link);
if (task)
--
2.41.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 2/6] bpf: Store ref_ctr_offsets values in bpf_uprobe array
2023-10-25 20:24 ` [PATCH bpf-next 2/6] bpf: Store ref_ctr_offsets values in bpf_uprobe array Jiri Olsa
@ 2023-10-26 16:31 ` Song Liu
2023-10-27 13:56 ` Jiri Olsa
0 siblings, 1 reply; 33+ messages in thread
From: Song Liu @ 2023-10-26 16:31 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> We will need to return ref_ctr_offsets values through link_info
> interface in following change, so we need to keep them around.
>
> Storing ref_ctr_offsets values directly into bpf_uprobe array.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Song Liu <song@kernel.org>
with one nitpick below.
> ---
> kernel/trace/bpf_trace.c | 14 +++-----------
> 1 file changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index df697c74d519..843b3846d3f8 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3031,6 +3031,7 @@ struct bpf_uprobe_multi_link;
> struct bpf_uprobe {
> struct bpf_uprobe_multi_link *link;
> loff_t offset;
> + unsigned long ref_ctr_offset;
nit: s/unsigned long/loff_t/ ?
> u64 cookie;
> struct uprobe_consumer consumer;
> };
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 2/6] bpf: Store ref_ctr_offsets values in bpf_uprobe array
2023-10-26 16:31 ` Song Liu
@ 2023-10-27 13:56 ` Jiri Olsa
2023-10-27 14:23 ` Song Liu
0 siblings, 1 reply; 33+ messages in thread
From: Jiri Olsa @ 2023-10-27 13:56 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Thu, Oct 26, 2023 at 09:31:00AM -0700, Song Liu wrote:
> On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > We will need to return ref_ctr_offsets values through link_info
> > interface in following change, so we need to keep them around.
> >
> > Storing ref_ctr_offsets values directly into bpf_uprobe array.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> Acked-by: Song Liu <song@kernel.org>
>
> with one nitpick below.
>
> > ---
> > kernel/trace/bpf_trace.c | 14 +++-----------
> > 1 file changed, 3 insertions(+), 11 deletions(-)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index df697c74d519..843b3846d3f8 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3031,6 +3031,7 @@ struct bpf_uprobe_multi_link;
> > struct bpf_uprobe {
> > struct bpf_uprobe_multi_link *link;
> > loff_t offset;
> > + unsigned long ref_ctr_offset;
>
> nit: s/unsigned long/loff_t/ ?
hum, the single uprobe interface also keeps it as 'unsigned long'
in 'struct trace_uprobe' .. while uprobe code keeps both offset and
ref_ctr_offset values as loff_t
is there any benefit by changing that to loff_t?
jirka
>
> > u64 cookie;
> > struct uprobe_consumer consumer;
> > };
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 2/6] bpf: Store ref_ctr_offsets values in bpf_uprobe array
2023-10-27 13:56 ` Jiri Olsa
@ 2023-10-27 14:23 ` Song Liu
2023-11-01 22:21 ` Andrii Nakryiko
0 siblings, 1 reply; 33+ messages in thread
From: Song Liu @ 2023-10-27 14:23 UTC (permalink / raw)
To: Jiri Olsa
Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
bpf@vger.kernel.org, Martin Lau, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
> On Oct 27, 2023, at 6:56 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Oct 26, 2023 at 09:31:00AM -0700, Song Liu wrote:
>> On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
>>>
>>> We will need to return ref_ctr_offsets values through link_info
>>> interface in following change, so we need to keep them around.
>>>
>>> Storing ref_ctr_offsets values directly into bpf_uprobe array.
>>>
>>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>>
>> Acked-by: Song Liu <song@kernel.org>
>>
>> with one nitpick below.
>>
>>> ---
>>> kernel/trace/bpf_trace.c | 14 +++-----------
>>> 1 file changed, 3 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
>>> index df697c74d519..843b3846d3f8 100644
>>> --- a/kernel/trace/bpf_trace.c
>>> +++ b/kernel/trace/bpf_trace.c
>>> @@ -3031,6 +3031,7 @@ struct bpf_uprobe_multi_link;
>>> struct bpf_uprobe {
>>> struct bpf_uprobe_multi_link *link;
>>> loff_t offset;
>>> + unsigned long ref_ctr_offset;
>>
>> nit: s/unsigned long/loff_t/ ?
>
> hum, the single uprobe interface also keeps it as 'unsigned long'
> in 'struct trace_uprobe' .. while uprobe code keeps both offset and
> ref_ctr_offset values as loff_t
>
> is there any benefit by changing that to loff_t?
We have "loff_t offset;" right above this line. So it is better to
use same type for the two offsets.
Thanks,
Song
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 2/6] bpf: Store ref_ctr_offsets values in bpf_uprobe array
2023-10-27 14:23 ` Song Liu
@ 2023-11-01 22:21 ` Andrii Nakryiko
0 siblings, 0 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 22:21 UTC (permalink / raw)
To: Song Liu
Cc: Jiri Olsa, Song Liu, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf@vger.kernel.org, Martin Lau, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo,
Yafang Shao
On Fri, Oct 27, 2023 at 7:24 AM Song Liu <songliubraving@meta.com> wrote:
>
>
>
> > On Oct 27, 2023, at 6:56 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Oct 26, 2023 at 09:31:00AM -0700, Song Liu wrote:
> >> On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >>>
> >>> We will need to return ref_ctr_offsets values through link_info
> >>> interface in following change, so we need to keep them around.
> >>>
> >>> Storing ref_ctr_offsets values directly into bpf_uprobe array.
> >>>
> >>> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> >>
> >> Acked-by: Song Liu <song@kernel.org>
> >>
> >> with one nitpick below.
> >>
> >>> ---
> >>> kernel/trace/bpf_trace.c | 14 +++-----------
> >>> 1 file changed, 3 insertions(+), 11 deletions(-)
> >>>
> >>> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> >>> index df697c74d519..843b3846d3f8 100644
> >>> --- a/kernel/trace/bpf_trace.c
> >>> +++ b/kernel/trace/bpf_trace.c
> >>> @@ -3031,6 +3031,7 @@ struct bpf_uprobe_multi_link;
> >>> struct bpf_uprobe {
> >>> struct bpf_uprobe_multi_link *link;
> >>> loff_t offset;
> >>> + unsigned long ref_ctr_offset;
> >>
> >> nit: s/unsigned long/loff_t/ ?
> >
> > hum, the single uprobe interface also keeps it as 'unsigned long'
> > in 'struct trace_uprobe' .. while uprobe code keeps both offset and
> > ref_ctr_offset values as loff_t
> >
> > is there any benefit by changing that to loff_t?
>
> We have "loff_t offset;" right above this line. So it is better to
> use same type for the two offsets.
but user is providing it as `unsigned long *` array, so instead of
relying on loff_t being the same as unsigned long, let's just keep the
original data type?
>
> Thanks,
> Song
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-25 20:24 [PATCH bpf-next 0/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
2023-10-25 20:24 ` [PATCH bpf-next 1/6] libbpf: Add st_type argument to elf_resolve_syms_offsets function Jiri Olsa
2023-10-25 20:24 ` [PATCH bpf-next 2/6] bpf: Store ref_ctr_offsets values in bpf_uprobe array Jiri Olsa
@ 2023-10-25 20:24 ` Jiri Olsa
2023-10-26 11:57 ` Yafang Shao
` (3 more replies)
2023-10-25 20:24 ` [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests Jiri Olsa
` (2 subsequent siblings)
5 siblings, 4 replies; 33+ messages in thread
From: Jiri Olsa @ 2023-10-25 20:24 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
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 | 68 ++++++++++++++++++++++++++++++++++
tools/include/uapi/linux/bpf.h | 10 +++++
3 files changed, 88 insertions(+)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 0f6cdf52b1da..960cf2914d63 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_max; /* in/out: uprobe_multi 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 843b3846d3f8..9f8ad19a1a93 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3042,6 +3042,7 @@ struct bpf_uprobe_multi_link {
u32 cnt;
struct bpf_uprobe *uprobes;
struct task_struct *task;
+ u32 flags;
};
struct bpf_uprobe_multi_run_ctx {
@@ -3081,9 +3082,75 @@ 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_max = info->uprobe_multi.path_max;
+ struct bpf_uprobe_multi_link *umulti_link;
+ u32 ucount = info->uprobe_multi.count;
+ int err = 0, i;
+ char *p, *buf;
+ long left;
+
+ if (!upath ^ !upath_max)
+ 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(umulti_link->task) : (u32) -1;
+
+ if (upath) {
+ if (upath_max > PATH_MAX)
+ return -E2BIG;
+ buf = kmalloc(upath_max, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+ p = d_path(&umulti_link->path, buf, upath_max);
+ if (IS_ERR(p)) {
+ kfree(buf);
+ return -ENOSPC;
+ }
+ left = copy_to_user(upath, p, buf + upath_max - 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,
@@ -3272,6 +3339,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..960cf2914d63 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_max; /* in/out: uprobe_multi 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
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-25 20:24 ` [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
@ 2023-10-26 11:57 ` Yafang Shao
2023-10-27 13:59 ` Jiri Olsa
2023-10-26 17:55 ` Song Liu
` (2 subsequent siblings)
3 siblings, 1 reply; 33+ messages in thread
From: Yafang Shao @ 2023-10-26 11:57 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo
On Thu, Oct 26, 2023 at 4:24 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 | 68 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 10 +++++
> 3 files changed, 88 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0f6cdf52b1da..960cf2914d63 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;
The bpf cookie for the perf_event link is exposed through
'pid_iter.bpf.c,' while the cookies for the tracing link and
kprobe_multi link are not exposed at all. This inconsistency can be
confusing. I believe it would be better to include all of them in the
link_info. The reason is that 'pid_iter' depends on the task holding
the links, which may not exist. However, I think we handle this in a
separate patchset. What do you think?
> + __u32 path_max; /* in/out: uprobe_multi 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 843b3846d3f8..9f8ad19a1a93 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3042,6 +3042,7 @@ struct bpf_uprobe_multi_link {
> u32 cnt;
> struct bpf_uprobe *uprobes;
> struct task_struct *task;
> + u32 flags;
> };
>
> struct bpf_uprobe_multi_run_ctx {
> @@ -3081,9 +3082,75 @@ 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_max = info->uprobe_multi.path_max;
> + struct bpf_uprobe_multi_link *umulti_link;
> + u32 ucount = info->uprobe_multi.count;
> + int err = 0, i;
> + char *p, *buf;
> + long left;
> +
> + if (!upath ^ !upath_max)
> + 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(umulti_link->task) : (u32) -1;
> +
> + if (upath) {
> + if (upath_max > PATH_MAX)
> + return -E2BIG;
> + buf = kmalloc(upath_max, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> + p = d_path(&umulti_link->path, buf, upath_max);
> + if (IS_ERR(p)) {
> + kfree(buf);
> + return -ENOSPC;
> + }
> + left = copy_to_user(upath, p, buf + upath_max - 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,
> @@ -3272,6 +3339,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..960cf2914d63 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_max; /* in/out: uprobe_multi 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
>
--
Regards
Yafang
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-26 11:57 ` Yafang Shao
@ 2023-10-27 13:59 ` Jiri Olsa
2023-11-09 8:56 ` Jiri Olsa
0 siblings, 1 reply; 33+ messages in thread
From: Jiri Olsa @ 2023-10-27 13:59 UTC (permalink / raw)
To: Yafang Shao
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo
On Thu, Oct 26, 2023 at 07:57:27PM +0800, Yafang Shao wrote:
> On Thu, Oct 26, 2023 at 4:24 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 | 68 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 10 +++++
> > 3 files changed, 88 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 0f6cdf52b1da..960cf2914d63 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;
>
> The bpf cookie for the perf_event link is exposed through
> 'pid_iter.bpf.c,' while the cookies for the tracing link and
> kprobe_multi link are not exposed at all. This inconsistency can be
> confusing. I believe it would be better to include all of them in the
> link_info. The reason is that 'pid_iter' depends on the task holding
> the links, which may not exist. However, I think we handle this in a
> separate patchset. What do you think?
right, I think we should add cookies for both kprobe_multi
and tracing link, I'll add that in new version
thanks,
jirka
>
> > + __u32 path_max; /* in/out: uprobe_multi 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 843b3846d3f8..9f8ad19a1a93 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3042,6 +3042,7 @@ struct bpf_uprobe_multi_link {
> > u32 cnt;
> > struct bpf_uprobe *uprobes;
> > struct task_struct *task;
> > + u32 flags;
> > };
> >
> > struct bpf_uprobe_multi_run_ctx {
> > @@ -3081,9 +3082,75 @@ 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_max = info->uprobe_multi.path_max;
> > + struct bpf_uprobe_multi_link *umulti_link;
> > + u32 ucount = info->uprobe_multi.count;
> > + int err = 0, i;
> > + char *p, *buf;
> > + long left;
> > +
> > + if (!upath ^ !upath_max)
> > + 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(umulti_link->task) : (u32) -1;
> > +
> > + if (upath) {
> > + if (upath_max > PATH_MAX)
> > + return -E2BIG;
> > + buf = kmalloc(upath_max, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > + p = d_path(&umulti_link->path, buf, upath_max);
> > + if (IS_ERR(p)) {
> > + kfree(buf);
> > + return -ENOSPC;
> > + }
> > + left = copy_to_user(upath, p, buf + upath_max - 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,
> > @@ -3272,6 +3339,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..960cf2914d63 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_max; /* in/out: uprobe_multi 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
> >
>
>
> --
> Regards
> Yafang
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-27 13:59 ` Jiri Olsa
@ 2023-11-09 8:56 ` Jiri Olsa
0 siblings, 0 replies; 33+ messages in thread
From: Jiri Olsa @ 2023-11-09 8:56 UTC (permalink / raw)
To: Jiri Olsa
Cc: Yafang Shao, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo
On Fri, Oct 27, 2023 at 03:59:33PM +0200, Jiri Olsa wrote:
> On Thu, Oct 26, 2023 at 07:57:27PM +0800, Yafang Shao wrote:
> > On Thu, Oct 26, 2023 at 4:24 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 | 68 ++++++++++++++++++++++++++++++++++
> > > tools/include/uapi/linux/bpf.h | 10 +++++
> > > 3 files changed, 88 insertions(+)
> > >
> > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > > index 0f6cdf52b1da..960cf2914d63 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;
> >
> > The bpf cookie for the perf_event link is exposed through
> > 'pid_iter.bpf.c,' while the cookies for the tracing link and
> > kprobe_multi link are not exposed at all. This inconsistency can be
> > confusing. I believe it would be better to include all of them in the
> > link_info. The reason is that 'pid_iter' depends on the task holding
> > the links, which may not exist. However, I think we handle this in a
> > separate patchset. What do you think?
>
> right, I think we should add cookies for both kprobe_multi
> and tracing link, I'll add that in new version
actually.. ;-) it's 5 extra patches already even without bpftool
changes, so I'll send it separately after this one gets merged
jirka
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-25 20:24 ` [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
2023-10-26 11:57 ` Yafang Shao
@ 2023-10-26 17:55 ` Song Liu
2023-10-27 14:29 ` Jiri Olsa
2023-10-30 10:18 ` Quentin Monnet
2023-11-01 22:21 ` Andrii Nakryiko
3 siblings, 1 reply; 33+ messages in thread
From: Song Liu @ 2023-10-26 17:55 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
[...]
> __u64 missed;
> } kprobe_multi;
> + struct {
> + __aligned_u64 path;
> + __aligned_u64 offsets;
> + __aligned_u64 ref_ctr_offsets;
> + __aligned_u64 cookies;
> + __u32 path_max; /* in/out: uprobe_multi path size */
I don't think we use path_max for output. Did I miss something?
> + __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;
[...]
> +
> + 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(umulti_link->task) : (u32) -1;
I think we can just use 0 here (instead of (u32)-1)?
> +
> + if (upath) {
nit: we are only using buf and p in this {}. It is cleaner to define them here.
> + if (upath_max > PATH_MAX)
> + return -E2BIG;
I don't think we need to fail here. How about we simply do
upath_max = min_ut(u32, upath_max, PATH_MAX);
> + buf = kmalloc(upath_max, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> + p = d_path(&umulti_link->path, buf, upath_max);
> + if (IS_ERR(p)) {
> + kfree(buf);
> + return -ENOSPC;
> + }
> + left = copy_to_user(upath, p, buf + upath_max - 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;
It feels expensive to put_user() 3x in a loop. Maybe we need a new struct
with offset, ref_ctr_offset, and cookie?
Thanks,
Song
> + }
> +
> + return err;
> +}
> +
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-26 17:55 ` Song Liu
@ 2023-10-27 14:29 ` Jiri Olsa
2023-11-01 22:21 ` Andrii Nakryiko
0 siblings, 1 reply; 33+ messages in thread
From: Jiri Olsa @ 2023-10-27 14:29 UTC (permalink / raw)
To: Song Liu
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Thu, Oct 26, 2023 at 10:55:35AM -0700, Song Liu wrote:
> On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
> [...]
> > __u64 missed;
> > } kprobe_multi;
> > + struct {
> > + __aligned_u64 path;
> > + __aligned_u64 offsets;
> > + __aligned_u64 ref_ctr_offsets;
> > + __aligned_u64 cookies;
> > + __u32 path_max; /* in/out: uprobe_multi path size */
>
> I don't think we use path_max for output. Did I miss something?
it's used by user to specify the size of the buffer
for storing the path, so you're right just 'in'
>
> > + __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;
>
> [...]
>
> > +
> > + 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(umulti_link->task) : (u32) -1;
>
> I think we can just use 0 here (instead of (u32)-1)?
ok
>
> > +
> > + if (upath) {
>
> nit: we are only using buf and p in this {}. It is cleaner to define them here.
ok
>
> > + if (upath_max > PATH_MAX)
> > + return -E2BIG;
>
> I don't think we need to fail here. How about we simply do
>
> upath_max = min_ut(u32, upath_max, PATH_MAX);
ok
>
> > + buf = kmalloc(upath_max, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > + p = d_path(&umulti_link->path, buf, upath_max);
> > + if (IS_ERR(p)) {
> > + kfree(buf);
> > + return -ENOSPC;
> > + }
> > + left = copy_to_user(upath, p, buf + upath_max - 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;
>
> It feels expensive to put_user() 3x in a loop. Maybe we need a new struct
> with offset, ref_ctr_offset, and cookie?
good idea, I think we could store offsets/uref_ctr_offsets/cookies
together both in kernel and user sapce and use just single put_user
call... will check
thanks,
jirka
>
> Thanks,
> Song
>
> > + }
> > +
> > + return err;
> > +}
> > +
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-27 14:29 ` Jiri Olsa
@ 2023-11-01 22:21 ` Andrii Nakryiko
2023-11-02 14:58 ` Jiri Olsa
0 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 22:21 UTC (permalink / raw)
To: Jiri Olsa
Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Fri, Oct 27, 2023 at 7:30 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Thu, Oct 26, 2023 at 10:55:35AM -0700, Song Liu wrote:
> > On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > [...]
[...]
> >
> > > + if (upath_max > PATH_MAX)
> > > + return -E2BIG;
> >
> > I don't think we need to fail here. How about we simply do
> >
> > upath_max = min_ut(u32, upath_max, PATH_MAX);
>
> ok
+1, was going to say the same
>
> >
> > > + buf = kmalloc(upath_max, GFP_KERNEL);
> > > + if (!buf)
> > > + return -ENOMEM;
> > > + p = d_path(&umulti_link->path, buf, upath_max);
> > > + if (IS_ERR(p)) {
> > > + kfree(buf);
> > > + return -ENOSPC;
> > > + }
> > > + left = copy_to_user(upath, p, buf + upath_max - 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;
> >
> > It feels expensive to put_user() 3x in a loop. Maybe we need a new struct
> > with offset, ref_ctr_offset, and cookie?
>
> good idea, I think we could store offsets/uref_ctr_offsets/cookies
> together both in kernel and user sapce and use just single put_user
> call... will check
>
hm... only offset is mandatory, and then we can have either cookie or
ref_ctr_offset or both, so co-locating them in the same struct seems
inconvenient and unnecessary
> thanks,
> jirka
>
> >
> > Thanks,
> > Song
> >
> > > + }
> > > +
> > > + return err;
> > > +}
> > > +
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-11-01 22:21 ` Andrii Nakryiko
@ 2023-11-02 14:58 ` Jiri Olsa
2023-11-02 16:21 ` Andrii Nakryiko
0 siblings, 1 reply; 33+ messages in thread
From: Jiri Olsa @ 2023-11-02 14:58 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Jiri Olsa, Song Liu, Alexei Starovoitov, Daniel Borkmann,
Andrii Nakryiko, bpf, Martin KaFai Lau, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo,
Yafang Shao
On Wed, Nov 01, 2023 at 03:21:42PM -0700, Andrii Nakryiko wrote:
> On Fri, Oct 27, 2023 at 7:30 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Thu, Oct 26, 2023 at 10:55:35AM -0700, Song Liu wrote:
> > > On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > [...]
>
> [...]
>
> > >
> > > > + if (upath_max > PATH_MAX)
> > > > + return -E2BIG;
> > >
> > > I don't think we need to fail here. How about we simply do
> > >
> > > upath_max = min_ut(u32, upath_max, PATH_MAX);
> >
> > ok
>
> +1, was going to say the same
>
> >
> > >
> > > > + buf = kmalloc(upath_max, GFP_KERNEL);
> > > > + if (!buf)
> > > > + return -ENOMEM;
> > > > + p = d_path(&umulti_link->path, buf, upath_max);
> > > > + if (IS_ERR(p)) {
> > > > + kfree(buf);
> > > > + return -ENOSPC;
> > > > + }
> > > > + left = copy_to_user(upath, p, buf + upath_max - 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;
> > >
> > > It feels expensive to put_user() 3x in a loop. Maybe we need a new struct
> > > with offset, ref_ctr_offset, and cookie?
> >
> > good idea, I think we could store offsets/uref_ctr_offsets/cookies
> > together both in kernel and user sapce and use just single put_user
> > call... will check
> >
>
> hm... only offset is mandatory, and then we can have either cookie or
> ref_ctr_offset or both, so co-locating them in the same struct seems
> inconvenient and unnecessary
yes, using struct seems too complicated because of this
but during attach we could store offsets/ref_ctr_offsets/cookies
in separated arrays (instead of in bpf_uprobe) and just use single
copy_to_user call for each array in here
jirka
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-11-02 14:58 ` Jiri Olsa
@ 2023-11-02 16:21 ` Andrii Nakryiko
0 siblings, 0 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2023-11-02 16:21 UTC (permalink / raw)
To: Jiri Olsa
Cc: Song Liu, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Thu, Nov 2, 2023 at 7:58 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Nov 01, 2023 at 03:21:42PM -0700, Andrii Nakryiko wrote:
> > On Fri, Oct 27, 2023 at 7:30 AM Jiri Olsa <olsajiri@gmail.com> wrote:
> > >
> > > On Thu, Oct 26, 2023 at 10:55:35AM -0700, Song Liu wrote:
> > > > On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@kernel.org> wrote:
> > > > [...]
> >
> > [...]
> >
> > > >
> > > > > + if (upath_max > PATH_MAX)
> > > > > + return -E2BIG;
> > > >
> > > > I don't think we need to fail here. How about we simply do
> > > >
> > > > upath_max = min_ut(u32, upath_max, PATH_MAX);
> > >
> > > ok
> >
> > +1, was going to say the same
> >
> > >
> > > >
> > > > > + buf = kmalloc(upath_max, GFP_KERNEL);
> > > > > + if (!buf)
> > > > > + return -ENOMEM;
> > > > > + p = d_path(&umulti_link->path, buf, upath_max);
> > > > > + if (IS_ERR(p)) {
> > > > > + kfree(buf);
> > > > > + return -ENOSPC;
> > > > > + }
> > > > > + left = copy_to_user(upath, p, buf + upath_max - 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;
> > > >
> > > > It feels expensive to put_user() 3x in a loop. Maybe we need a new struct
> > > > with offset, ref_ctr_offset, and cookie?
> > >
> > > good idea, I think we could store offsets/uref_ctr_offsets/cookies
> > > together both in kernel and user sapce and use just single put_user
> > > call... will check
> > >
> >
> > hm... only offset is mandatory, and then we can have either cookie or
> > ref_ctr_offset or both, so co-locating them in the same struct seems
> > inconvenient and unnecessary
>
> yes, using struct seems too complicated because of this
>
> but during attach we could store offsets/ref_ctr_offsets/cookies
> in separated arrays (instead of in bpf_uprobe) and just use single
> copy_to_user call for each array in here
I think you are trying to optimize for the wrong thing here. link_info
fetching isn't the most performance critical operation, I wouldn't
bother complicating code just to make it a microsecond faster.
>
> jirka
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-25 20:24 ` [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
2023-10-26 11:57 ` Yafang Shao
2023-10-26 17:55 ` Song Liu
@ 2023-10-30 10:18 ` Quentin Monnet
2023-10-30 21:17 ` Jiri Olsa
2023-11-01 22:21 ` Andrii Nakryiko
3 siblings, 1 reply; 33+ messages in thread
From: Quentin Monnet @ 2023-10-30 10:18 UTC (permalink / raw)
To: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
2023-10-25 21:24 UTC+0100 ~ Jiri Olsa
> 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 | 68 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 10 +++++
> 3 files changed, 88 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0f6cdf52b1da..960cf2914d63 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_max; /* in/out: uprobe_multi path size */
Just a nit on the naming here: I don't really understand why this is
"path_max", should it be "path_size" instead?
Quentin
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-30 10:18 ` Quentin Monnet
@ 2023-10-30 21:17 ` Jiri Olsa
0 siblings, 0 replies; 33+ messages in thread
From: Jiri Olsa @ 2023-10-30 21:17 UTC (permalink / raw)
To: Quentin Monnet
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Mon, Oct 30, 2023 at 10:18:59AM +0000, Quentin Monnet wrote:
>
> 2023-10-25 21:24 UTC+0100 ~ Jiri Olsa
> > 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 | 68 ++++++++++++++++++++++++++++++++++
> > tools/include/uapi/linux/bpf.h | 10 +++++
> > 3 files changed, 88 insertions(+)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 0f6cdf52b1da..960cf2914d63 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_max; /* in/out: uprobe_multi path size */
>
> Just a nit on the naming here: I don't really understand why this is
> "path_max", should it be "path_size" instead?
right, path_size fits better, will change
thanks,
jirka
>
> Quentin
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-10-25 20:24 ` [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
` (2 preceding siblings ...)
2023-10-30 10:18 ` Quentin Monnet
@ 2023-11-01 22:21 ` Andrii Nakryiko
2023-11-02 14:43 ` Jiri Olsa
3 siblings, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 22:21 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Oct 25, 2023 at 1:24 PM 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 | 68 ++++++++++++++++++++++++++++++++++
> tools/include/uapi/linux/bpf.h | 10 +++++
> 3 files changed, 88 insertions(+)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 0f6cdf52b1da..960cf2914d63 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_max; /* in/out: uprobe_multi path size */
people already called out that path_size makes for a better name, I agree
> + __u32 count; /* in/out: uprobe_multi offsets/ref_ctr_offsets/cookies count */
otherwise we'd have to call this count_max :)
> + __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 843b3846d3f8..9f8ad19a1a93 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3042,6 +3042,7 @@ struct bpf_uprobe_multi_link {
> u32 cnt;
> struct bpf_uprobe *uprobes;
> struct task_struct *task;
> + u32 flags;
> };
>
> struct bpf_uprobe_multi_run_ctx {
> @@ -3081,9 +3082,75 @@ 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_max = info->uprobe_multi.path_max;
> + struct bpf_uprobe_multi_link *umulti_link;
> + u32 ucount = info->uprobe_multi.count;
> + int err = 0, i;
> + char *p, *buf;
> + long left;
> +
> + if (!upath ^ !upath_max)
> + 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(umulti_link->task) : (u32) -1;
on attach we do
task = get_pid_task(find_vpid(pid), PIDTYPE_PID);
So on attachment we take pid in user's namespace, is that right? It's
kind of asymmetrical that we return the global PID back? Should we try
to convert PID to user's namespace instead?
> +
> + if (upath) {
> + if (upath_max > PATH_MAX)
> + return -E2BIG;
no need to fail here, as pointed out elsewhere
> + buf = kmalloc(upath_max, GFP_KERNEL);
here we can allocate min(PATH_MAX, upath_max)
> + if (!buf)
> + return -ENOMEM;
> + p = d_path(&umulti_link->path, buf, upath_max);
> + if (IS_ERR(p)) {
> + kfree(buf);
> + return -ENOSPC;
> + }
> + left = copy_to_user(upath, p, buf + upath_max - p);
> + kfree(buf);
> + if (left)
> + return -EFAULT;
> + }
> +
> + if (!uoffsets)
> + return 0;
it would be good to still return actual counts for out parameters, 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;
> +}
> +
[...]
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-11-01 22:21 ` Andrii Nakryiko
@ 2023-11-02 14:43 ` Jiri Olsa
2023-11-02 16:19 ` Andrii Nakryiko
0 siblings, 1 reply; 33+ messages in thread
From: Jiri Olsa @ 2023-11-02 14:43 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Nov 01, 2023 at 03:21:36PM -0700, Andrii Nakryiko wrote:
SNIP
> > + struct {
> > + __aligned_u64 path;
> > + __aligned_u64 offsets;
> > + __aligned_u64 ref_ctr_offsets;
> > + __aligned_u64 cookies;
> > + __u32 path_max; /* in/out: uprobe_multi path size */
>
> people already called out that path_size makes for a better name, I agree
>
> > + __u32 count; /* in/out: uprobe_multi offsets/ref_ctr_offsets/cookies count */
>
> otherwise we'd have to call this count_max :)
path_size is good ;-)
>
> > + __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 843b3846d3f8..9f8ad19a1a93 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3042,6 +3042,7 @@ struct bpf_uprobe_multi_link {
> > u32 cnt;
> > struct bpf_uprobe *uprobes;
> > struct task_struct *task;
> > + u32 flags;
> > };
> >
> > struct bpf_uprobe_multi_run_ctx {
> > @@ -3081,9 +3082,75 @@ 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_max = info->uprobe_multi.path_max;
> > + struct bpf_uprobe_multi_link *umulti_link;
> > + u32 ucount = info->uprobe_multi.count;
> > + int err = 0, i;
> > + char *p, *buf;
> > + long left;
> > +
> > + if (!upath ^ !upath_max)
> > + 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(umulti_link->task) : (u32) -1;
>
> on attach we do
>
> task = get_pid_task(find_vpid(pid), PIDTYPE_PID);
>
> So on attachment we take pid in user's namespace, is that right? It's
> kind of asymmetrical that we return the global PID back? Should we try
> to convert PID to user's namespace instead?
you're right, I think we should use this:
task_pid_nr_ns(umulti_link->task, task_active_pid_ns(current))
>
> > +
> > + if (upath) {
> > + if (upath_max > PATH_MAX)
> > + return -E2BIG;
>
> no need to fail here, as pointed out elsewhere
>
> > + buf = kmalloc(upath_max, GFP_KERNEL);
>
> here we can allocate min(PATH_MAX, upath_max)
yes, will do that
>
> > + if (!buf)
> > + return -ENOMEM;
> > + p = d_path(&umulti_link->path, buf, upath_max);
> > + if (IS_ERR(p)) {
> > + kfree(buf);
> > + return -ENOSPC;
> > + }
> > + left = copy_to_user(upath, p, buf + upath_max - p);
> > + kfree(buf);
> > + if (left)
> > + return -EFAULT;
> > + }
> > +
> > + if (!uoffsets)
> > + return 0;
>
> it would be good to still return actual counts for out parameters, no?
hm, we do that few lines above with:
info->uprobe_multi.count = umulti_link->cnt;
if that's what you mean
thanks,
jirka
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link
2023-11-02 14:43 ` Jiri Olsa
@ 2023-11-02 16:19 ` Andrii Nakryiko
0 siblings, 0 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2023-11-02 16:19 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Thu, Nov 2, 2023 at 7:43 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Nov 01, 2023 at 03:21:36PM -0700, Andrii Nakryiko wrote:
>
> SNIP
>
> > > + struct {
> > > + __aligned_u64 path;
> > > + __aligned_u64 offsets;
> > > + __aligned_u64 ref_ctr_offsets;
> > > + __aligned_u64 cookies;
> > > + __u32 path_max; /* in/out: uprobe_multi path size */
> >
> > people already called out that path_size makes for a better name, I agree
> >
> > > + __u32 count; /* in/out: uprobe_multi offsets/ref_ctr_offsets/cookies count */
> >
> > otherwise we'd have to call this count_max :)
>
> path_size is good ;-)
>
>
> >
> > > + __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 843b3846d3f8..9f8ad19a1a93 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -3042,6 +3042,7 @@ struct bpf_uprobe_multi_link {
> > > u32 cnt;
> > > struct bpf_uprobe *uprobes;
> > > struct task_struct *task;
> > > + u32 flags;
> > > };
> > >
> > > struct bpf_uprobe_multi_run_ctx {
> > > @@ -3081,9 +3082,75 @@ 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_max = info->uprobe_multi.path_max;
> > > + struct bpf_uprobe_multi_link *umulti_link;
> > > + u32 ucount = info->uprobe_multi.count;
> > > + int err = 0, i;
> > > + char *p, *buf;
> > > + long left;
> > > +
> > > + if (!upath ^ !upath_max)
> > > + 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(umulti_link->task) : (u32) -1;
> >
> > on attach we do
> >
> > task = get_pid_task(find_vpid(pid), PIDTYPE_PID);
> >
> > So on attachment we take pid in user's namespace, is that right? It's
> > kind of asymmetrical that we return the global PID back? Should we try
> > to convert PID to user's namespace instead?
>
> you're right, I think we should use this:
>
> task_pid_nr_ns(umulti_link->task, task_active_pid_ns(current))
>
> >
> > > +
> > > + if (upath) {
> > > + if (upath_max > PATH_MAX)
> > > + return -E2BIG;
> >
> > no need to fail here, as pointed out elsewhere
> >
> > > + buf = kmalloc(upath_max, GFP_KERNEL);
> >
> > here we can allocate min(PATH_MAX, upath_max)
>
> yes, will do that
>
> >
> > > + if (!buf)
> > > + return -ENOMEM;
> > > + p = d_path(&umulti_link->path, buf, upath_max);
> > > + if (IS_ERR(p)) {
> > > + kfree(buf);
> > > + return -ENOSPC;
> > > + }
> > > + left = copy_to_user(upath, p, buf + upath_max - p);
> > > + kfree(buf);
> > > + if (left)
> > > + return -EFAULT;
> > > + }
> > > +
> > > + if (!uoffsets)
> > > + return 0;
> >
> > it would be good to still return actual counts for out parameters, no?
>
> hm, we do that few lines above with:
>
> info->uprobe_multi.count = umulti_link->cnt;
>
> if that's what you mean
>
oh, yeah, my bad. I was for some reason expecting put_user() for this,
but it's a get_link_info operation, doh. Never mind.
> thanks,
> jirka
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests
2023-10-25 20:24 [PATCH bpf-next 0/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
` (2 preceding siblings ...)
2023-10-25 20:24 ` [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
@ 2023-10-25 20:24 ` Jiri Olsa
2023-10-26 11:41 ` Yafang Shao
2023-11-01 22:24 ` Andrii Nakryiko
2023-10-25 20:24 ` [PATCH bpf-next 5/6] selftests/bpf: Add link_info test for uprobe_multi link Jiri Olsa
2023-10-25 20:24 ` [PATCH bpf-next 6/6] bpftool: Add support to display uprobe_multi links Jiri Olsa
5 siblings, 2 replies; 33+ messages in thread
From: Jiri Olsa @ 2023-10-25 20:24 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
The fill_link_info test keeps skeleton open and just creates
various links. We are wrongly calling bpf_link__detach after
each test to close them, we need to call bpf_link__destroy.
Also we need to set the link NULL so the skeleton destroy
won't try to destroy them again.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/fill_link_info.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index 97142a4db374..0379872c445a 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -22,6 +22,11 @@ static __u64 kmulti_addrs[KMULTI_CNT];
#define KPROBE_FUNC "bpf_fentry_test1"
static __u64 kprobe_addr;
+#define LINK_DESTROY(__link) ({ \
+ bpf_link__destroy(__link); \
+ __link = NULL; \
+})
+
#define UPROBE_FILE "/proc/self/exe"
static ssize_t uprobe_offset;
/* uprobe attach point */
@@ -157,7 +162,7 @@ static void test_kprobe_fill_link_info(struct test_fill_link_info *skel,
} else {
kprobe_fill_invalid_user_buffer(link_fd);
}
- bpf_link__detach(skel->links.kprobe_run);
+ LINK_DESTROY(skel->links.kprobe_run);
}
static void test_tp_fill_link_info(struct test_fill_link_info *skel)
@@ -171,7 +176,7 @@ static void test_tp_fill_link_info(struct test_fill_link_info *skel)
link_fd = bpf_link__fd(skel->links.tp_run);
err = verify_perf_link_info(link_fd, BPF_PERF_EVENT_TRACEPOINT, 0, 0, 0);
ASSERT_OK(err, "verify_perf_link_info");
- bpf_link__detach(skel->links.tp_run);
+ LINK_DESTROY(skel->links.tp_run);
}
static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
@@ -189,7 +194,7 @@ static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
link_fd = bpf_link__fd(skel->links.uprobe_run);
err = verify_perf_link_info(link_fd, type, 0, uprobe_offset, 0);
ASSERT_OK(err, "verify_perf_link_info");
- bpf_link__detach(skel->links.uprobe_run);
+ LINK_DESTROY(skel->links.uprobe_run);
}
static int verify_kmulti_link_info(int fd, bool retprobe)
@@ -295,7 +300,7 @@ static void test_kprobe_multi_fill_link_info(struct test_fill_link_info *skel,
} else {
verify_kmulti_invalid_user_buffer(link_fd);
}
- bpf_link__detach(skel->links.kmulti_run);
+ LINK_DESTROY(skel->links.kmulti_run);
}
void test_fill_link_info(void)
--
2.41.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests
2023-10-25 20:24 ` [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests Jiri Olsa
@ 2023-10-26 11:41 ` Yafang Shao
2023-10-26 18:00 ` Song Liu
2023-11-01 22:24 ` Andrii Nakryiko
1 sibling, 1 reply; 33+ messages in thread
From: Yafang Shao @ 2023-10-26 11:41 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo
On Thu, Oct 26, 2023 at 4:25 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The fill_link_info test keeps skeleton open and just creates
> various links. We are wrongly calling bpf_link__detach after
> each test to close them, we need to call bpf_link__destroy.
>
> Also we need to set the link NULL so the skeleton destroy
> won't try to destroy them again.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Yafang Shao <laoar.shao@gmail.com>
> ---
> .../selftests/bpf/prog_tests/fill_link_info.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> index 97142a4db374..0379872c445a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> @@ -22,6 +22,11 @@ static __u64 kmulti_addrs[KMULTI_CNT];
> #define KPROBE_FUNC "bpf_fentry_test1"
> static __u64 kprobe_addr;
>
> +#define LINK_DESTROY(__link) ({ \
> + bpf_link__destroy(__link); \
> + __link = NULL; \
> +})
> +
> #define UPROBE_FILE "/proc/self/exe"
> static ssize_t uprobe_offset;
> /* uprobe attach point */
> @@ -157,7 +162,7 @@ static void test_kprobe_fill_link_info(struct test_fill_link_info *skel,
> } else {
> kprobe_fill_invalid_user_buffer(link_fd);
> }
> - bpf_link__detach(skel->links.kprobe_run);
> + LINK_DESTROY(skel->links.kprobe_run);
> }
>
> static void test_tp_fill_link_info(struct test_fill_link_info *skel)
> @@ -171,7 +176,7 @@ static void test_tp_fill_link_info(struct test_fill_link_info *skel)
> link_fd = bpf_link__fd(skel->links.tp_run);
> err = verify_perf_link_info(link_fd, BPF_PERF_EVENT_TRACEPOINT, 0, 0, 0);
> ASSERT_OK(err, "verify_perf_link_info");
> - bpf_link__detach(skel->links.tp_run);
> + LINK_DESTROY(skel->links.tp_run);
> }
>
> static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> @@ -189,7 +194,7 @@ static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> link_fd = bpf_link__fd(skel->links.uprobe_run);
> err = verify_perf_link_info(link_fd, type, 0, uprobe_offset, 0);
> ASSERT_OK(err, "verify_perf_link_info");
> - bpf_link__detach(skel->links.uprobe_run);
> + LINK_DESTROY(skel->links.uprobe_run);
> }
>
> static int verify_kmulti_link_info(int fd, bool retprobe)
> @@ -295,7 +300,7 @@ static void test_kprobe_multi_fill_link_info(struct test_fill_link_info *skel,
> } else {
> verify_kmulti_invalid_user_buffer(link_fd);
> }
> - bpf_link__detach(skel->links.kmulti_run);
> + LINK_DESTROY(skel->links.kmulti_run);
> }
>
> void test_fill_link_info(void)
> --
> 2.41.0
>
--
Regards
Yafang
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests
2023-10-26 11:41 ` Yafang Shao
@ 2023-10-26 18:00 ` Song Liu
0 siblings, 0 replies; 33+ messages in thread
From: Song Liu @ 2023-10-26 18:00 UTC (permalink / raw)
To: Yafang Shao
Cc: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo
On Thu, Oct 26, 2023 at 4:42 AM Yafang Shao <laoar.shao@gmail.com> wrote:
>
> On Thu, Oct 26, 2023 at 4:25 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The fill_link_info test keeps skeleton open and just creates
> > various links. We are wrongly calling bpf_link__detach after
> > each test to close them, we need to call bpf_link__destroy.
> >
> > Also we need to set the link NULL so the skeleton destroy
> > won't try to destroy them again.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> Acked-by: Yafang Shao <laoar.shao@gmail.com>
>
Acked-by: Song Liu <song@kernel.org>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests
2023-10-25 20:24 ` [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests Jiri Olsa
2023-10-26 11:41 ` Yafang Shao
@ 2023-11-01 22:24 ` Andrii Nakryiko
2023-11-02 14:12 ` Jiri Olsa
1 sibling, 1 reply; 33+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 22:24 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Oct 25, 2023 at 1:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> The fill_link_info test keeps skeleton open and just creates
> various links. We are wrongly calling bpf_link__detach after
> each test to close them, we need to call bpf_link__destroy.
>
> Also we need to set the link NULL so the skeleton destroy
> won't try to destroy them again.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../selftests/bpf/prog_tests/fill_link_info.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> index 97142a4db374..0379872c445a 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> @@ -22,6 +22,11 @@ static __u64 kmulti_addrs[KMULTI_CNT];
> #define KPROBE_FUNC "bpf_fentry_test1"
> static __u64 kprobe_addr;
>
> +#define LINK_DESTROY(__link) ({ \
> + bpf_link__destroy(__link); \
> + __link = NULL; \
> +})
> +
> #define UPROBE_FILE "/proc/self/exe"
> static ssize_t uprobe_offset;
> /* uprobe attach point */
> @@ -157,7 +162,7 @@ static void test_kprobe_fill_link_info(struct test_fill_link_info *skel,
> } else {
> kprobe_fill_invalid_user_buffer(link_fd);
> }
> - bpf_link__detach(skel->links.kprobe_run);
> + LINK_DESTROY(skel->links.kprobe_run);
> }
>
> static void test_tp_fill_link_info(struct test_fill_link_info *skel)
> @@ -171,7 +176,7 @@ static void test_tp_fill_link_info(struct test_fill_link_info *skel)
> link_fd = bpf_link__fd(skel->links.tp_run);
> err = verify_perf_link_info(link_fd, BPF_PERF_EVENT_TRACEPOINT, 0, 0, 0);
> ASSERT_OK(err, "verify_perf_link_info");
> - bpf_link__detach(skel->links.tp_run);
> + LINK_DESTROY(skel->links.tp_run);
> }
>
> static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> @@ -189,7 +194,7 @@ static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> link_fd = bpf_link__fd(skel->links.uprobe_run);
> err = verify_perf_link_info(link_fd, type, 0, uprobe_offset, 0);
> ASSERT_OK(err, "verify_perf_link_info");
> - bpf_link__detach(skel->links.uprobe_run);
> + LINK_DESTROY(skel->links.uprobe_run);
> }
>
> static int verify_kmulti_link_info(int fd, bool retprobe)
> @@ -295,7 +300,7 @@ static void test_kprobe_multi_fill_link_info(struct test_fill_link_info *skel,
> } else {
> verify_kmulti_invalid_user_buffer(link_fd);
> }
> - bpf_link__detach(skel->links.kmulti_run);
> + LINK_DESTROY(skel->links.kmulti_run);
if we don't want skeleton to take care of these links, we shouldn't
assign them into skel->links region, IMO
so perhaps the proper fix is to have local bpf_link variable in these tests?
> }
>
> void test_fill_link_info(void)
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests
2023-11-01 22:24 ` Andrii Nakryiko
@ 2023-11-02 14:12 ` Jiri Olsa
0 siblings, 0 replies; 33+ messages in thread
From: Jiri Olsa @ 2023-11-02 14:12 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Nov 01, 2023 at 03:24:36PM -0700, Andrii Nakryiko wrote:
> On Wed, Oct 25, 2023 at 1:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > The fill_link_info test keeps skeleton open and just creates
> > various links. We are wrongly calling bpf_link__detach after
> > each test to close them, we need to call bpf_link__destroy.
> >
> > Also we need to set the link NULL so the skeleton destroy
> > won't try to destroy them again.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > .../selftests/bpf/prog_tests/fill_link_info.c | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> > index 97142a4db374..0379872c445a 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
> > @@ -22,6 +22,11 @@ static __u64 kmulti_addrs[KMULTI_CNT];
> > #define KPROBE_FUNC "bpf_fentry_test1"
> > static __u64 kprobe_addr;
> >
> > +#define LINK_DESTROY(__link) ({ \
> > + bpf_link__destroy(__link); \
> > + __link = NULL; \
> > +})
> > +
> > #define UPROBE_FILE "/proc/self/exe"
> > static ssize_t uprobe_offset;
> > /* uprobe attach point */
> > @@ -157,7 +162,7 @@ static void test_kprobe_fill_link_info(struct test_fill_link_info *skel,
> > } else {
> > kprobe_fill_invalid_user_buffer(link_fd);
> > }
> > - bpf_link__detach(skel->links.kprobe_run);
> > + LINK_DESTROY(skel->links.kprobe_run);
> > }
> >
> > static void test_tp_fill_link_info(struct test_fill_link_info *skel)
> > @@ -171,7 +176,7 @@ static void test_tp_fill_link_info(struct test_fill_link_info *skel)
> > link_fd = bpf_link__fd(skel->links.tp_run);
> > err = verify_perf_link_info(link_fd, BPF_PERF_EVENT_TRACEPOINT, 0, 0, 0);
> > ASSERT_OK(err, "verify_perf_link_info");
> > - bpf_link__detach(skel->links.tp_run);
> > + LINK_DESTROY(skel->links.tp_run);
> > }
> >
> > static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> > @@ -189,7 +194,7 @@ static void test_uprobe_fill_link_info(struct test_fill_link_info *skel,
> > link_fd = bpf_link__fd(skel->links.uprobe_run);
> > err = verify_perf_link_info(link_fd, type, 0, uprobe_offset, 0);
> > ASSERT_OK(err, "verify_perf_link_info");
> > - bpf_link__detach(skel->links.uprobe_run);
> > + LINK_DESTROY(skel->links.uprobe_run);
> > }
> >
> > static int verify_kmulti_link_info(int fd, bool retprobe)
> > @@ -295,7 +300,7 @@ static void test_kprobe_multi_fill_link_info(struct test_fill_link_info *skel,
> > } else {
> > verify_kmulti_invalid_user_buffer(link_fd);
> > }
> > - bpf_link__detach(skel->links.kmulti_run);
> > + LINK_DESTROY(skel->links.kmulti_run);
>
> if we don't want skeleton to take care of these links, we shouldn't
> assign them into skel->links region, IMO
>
> so perhaps the proper fix is to have local bpf_link variable in these tests?
ok, that looks cleaner
jirka
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next 5/6] selftests/bpf: Add link_info test for uprobe_multi link
2023-10-25 20:24 [PATCH bpf-next 0/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
` (3 preceding siblings ...)
2023-10-25 20:24 ` [PATCH bpf-next 4/6] selftests/bpf: Use bpf_link__destroy in fill_link_info tests Jiri Olsa
@ 2023-10-25 20:24 ` Jiri Olsa
2023-10-26 18:13 ` Song Liu
2023-11-01 22:27 ` Andrii Nakryiko
2023-10-25 20:24 ` [PATCH bpf-next 6/6] bpftool: Add support to display uprobe_multi links Jiri Olsa
5 siblings, 2 replies; 33+ messages in thread
From: Jiri Olsa @ 2023-10-25 20:24 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
Adding fill_link_info test for uprobe_multi link.
Setting up uprobes with bogus ref_ctr_offsets and cookie values
to test all the bpf_link_info::uprobe_multi fields.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../selftests/bpf/prog_tests/fill_link_info.c | 189 ++++++++++++++++++
.../selftests/bpf/progs/test_fill_link_info.c | 6 +
2 files changed, 195 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
index 0379872c445a..96029fcbd47f 100644
--- a/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
+++ b/tools/testing/selftests/bpf/prog_tests/fill_link_info.c
@@ -7,6 +7,7 @@
#include <test_progs.h>
#include "trace_helpers.h"
#include "test_fill_link_info.skel.h"
+#include "bpf/libbpf_internal.h"
#define TP_CAT "sched"
#define TP_NAME "sched_switch"
@@ -303,6 +304,187 @@ static void test_kprobe_multi_fill_link_info(struct test_fill_link_info *skel,
LINK_DESTROY(skel->links.kmulti_run);
}
+/* Initialize semaphore variables so they don't end up in bss
+ * section and we could get retrieve their offsets.
+ */
+static short uprobe_link_info_sema_1 = 1;
+static short uprobe_link_info_sema_2 = 1;
+static short uprobe_link_info_sema_3 = 1;
+
+noinline void uprobe_link_info_func_1(void)
+{
+ uprobe_link_info_sema_1++;
+ asm volatile ("");
+}
+
+noinline void uprobe_link_info_func_2(void)
+{
+ uprobe_link_info_sema_2++;
+ asm volatile ("");
+}
+
+noinline void uprobe_link_info_func_3(void)
+{
+ uprobe_link_info_sema_3++;
+ asm volatile ("");
+}
+
+static int
+verify_umulti_link_info(int fd, bool retprobe, int pid, __u64 *offsets,
+ __u64 *cookies, __u64 *ref_ctr_offsets)
+{
+ char path[PATH_MAX], path_buf[PATH_MAX];
+ struct bpf_link_info info;
+ __u32 len = sizeof(info);
+ __u64 ref_ctr_offsets_buf[3];
+ __u64 offsets_buf[3];
+ __u64 cookies_buf[3];
+ int i, err;
+
+ memset(path, 0, sizeof(path));
+ err = readlink("/proc/self/exe", path, sizeof(path));
+ if (!ASSERT_NEQ(err, -1, "readlink"))
+ return -1;
+
+ memset(&info, 0, sizeof(info));
+ info.uprobe_multi.path = ptr_to_u64(path_buf);
+ info.uprobe_multi.path_max = sizeof(path_buf);
+
+again:
+ err = bpf_link_get_info_by_fd(fd, &info, &len);
+ if (!ASSERT_OK(err, "bpf_link_get_info_by_fd"))
+ return -1;
+
+ if (!ASSERT_EQ(info.type, BPF_LINK_TYPE_UPROBE_MULTI, "info.type"))
+ return -1;
+
+ ASSERT_EQ(info.uprobe_multi.pid, pid ?: getpid(), "info.uprobe_multi.pid");
+ ASSERT_EQ(info.uprobe_multi.count, 3, "info.uprobe_multi.count");
+ ASSERT_EQ(info.uprobe_multi.flags & BPF_F_KPROBE_MULTI_RETURN,
+ retprobe, "info.uprobe_multi.flags.retprobe");
+ ASSERT_STREQ(path_buf, path, "info.uprobe_multi.path");
+
+ if (!info.uprobe_multi.offsets) {
+ info.uprobe_multi.offsets = ptr_to_u64(offsets_buf);
+ info.uprobe_multi.cookies = ptr_to_u64(cookies_buf);
+ info.uprobe_multi.ref_ctr_offsets = ptr_to_u64(ref_ctr_offsets_buf);
+ goto again;
+ }
+
+ for (i = 0; i < info.uprobe_multi.count; i++) {
+ ASSERT_EQ(offsets_buf[i], offsets[i], "info.uprobe_multi.offsets");
+ ASSERT_EQ(cookies_buf[i], cookies[i], "info.uprobe_multi.cookies");
+ ASSERT_EQ(ref_ctr_offsets_buf[i], ref_ctr_offsets[i], "info.uprobe_multi.ref_ctr_offsets");
+ }
+ return 0;
+}
+
+static void verify_umulti_invalid_user_buffer(int fd)
+{
+ struct bpf_link_info info;
+ __u32 len = sizeof(info);
+ char path[PATH_MAX + 1];
+ __u64 offsets[3];
+ int err;
+
+ // upath_max defined, not path
+ memset(&info, 0, sizeof(info));
+ info.uprobe_multi.path_max = 3;
+ err = bpf_link_get_info_by_fd(fd, &info, &len);
+ ASSERT_EQ(err, -EINVAL, "failed_upath_max");
+
+ // path has wrong pointer
+ memset(&info, 0, sizeof(info));
+ info.uprobe_multi.path_max = PATH_MAX;
+ info.uprobe_multi.path = 123;
+ err = bpf_link_get_info_by_fd(fd, &info, &len);
+ ASSERT_EQ(err, -EFAULT, "failed_bad_path_ptr");
+
+ // count defined, not offsets
+ memset(&info, 0, sizeof(info));
+ info.uprobe_multi.count = 3;
+ err = bpf_link_get_info_by_fd(fd, &info, &len);
+ ASSERT_EQ(err, -EINVAL, "failed_count");
+
+ // path_max too big
+ memset(&info, 0, sizeof(info));
+ info.uprobe_multi.path = ptr_to_u64(path);
+ info.uprobe_multi.path_max = sizeof(path);
+ err = bpf_link_get_info_by_fd(fd, &info, &len);
+ ASSERT_EQ(err, -E2BIG, "failed_path_max");
+
+ // offsets not big enough
+ memset(&info, 0, sizeof(info));
+ info.uprobe_multi.offsets = ptr_to_u64(offsets);
+ info.uprobe_multi.count = 2;
+ err = bpf_link_get_info_by_fd(fd, &info, &len);
+ ASSERT_EQ(err, -ENOSPC, "failed_small_count");
+
+ // offsets has wrong pointer
+ memset(&info, 0, sizeof(info));
+ info.uprobe_multi.offsets = 123;
+ info.uprobe_multi.count = 3;
+ err = bpf_link_get_info_by_fd(fd, &info, &len);
+ ASSERT_EQ(err, -EFAULT, "failed_wrong_offsets");
+}
+
+static void test_uprobe_multi_fill_link_info(struct test_fill_link_info *skel,
+ bool retprobe, bool invalid, int pid)
+{
+ LIBBPF_OPTS(bpf_uprobe_multi_opts, opts,
+ .retprobe = retprobe,
+ );
+ const char *syms[3] = {
+ "uprobe_link_info_func_1",
+ "uprobe_link_info_func_2",
+ "uprobe_link_info_func_3",
+ };
+ const char *sema[3] = {
+ "uprobe_link_info_sema_1",
+ "uprobe_link_info_sema_2",
+ "uprobe_link_info_sema_3",
+ };
+ __u64 cookies[3] = {
+ 0xdead,
+ 0xbeef,
+ 0xcafe,
+ };
+ __u64 *offsets, *ref_ctr_offsets;
+ int link_fd, err;
+
+ err = elf_resolve_syms_offsets("/proc/self/exe", 3, sema,
+ (unsigned long **) &ref_ctr_offsets, STT_OBJECT);
+ if (!ASSERT_OK(err, "elf_resolve_syms_offsets_object"))
+ return;
+
+ err = elf_resolve_syms_offsets("/proc/self/exe", 3, syms,
+ (unsigned long **) &offsets, STT_FUNC);
+ if (!ASSERT_OK(err, "elf_resolve_syms_offsets_func"))
+ return;
+
+ opts.syms = syms;
+ opts.cookies = &cookies[0];
+ opts.ref_ctr_offsets = (unsigned long *) &ref_ctr_offsets[0];
+ opts.cnt = ARRAY_SIZE(syms);
+
+ skel->links.umulti_run = bpf_program__attach_uprobe_multi(skel->progs.umulti_run, pid,
+ "/proc/self/exe", NULL, &opts);
+ if (!ASSERT_OK_PTR(skel->links.umulti_run, "bpf_program__attach_uprobe_multi"))
+ goto out;
+
+ link_fd = bpf_link__fd(skel->links.umulti_run);
+ if (invalid)
+ verify_umulti_invalid_user_buffer(link_fd);
+ else
+ verify_umulti_link_info(link_fd, retprobe, pid, offsets, cookies, ref_ctr_offsets);
+
+ LINK_DESTROY(skel->links.umulti_run);
+
+out:
+ free(ref_ctr_offsets);
+ free(offsets);
+}
+
void test_fill_link_info(void)
{
struct test_fill_link_info *skel;
@@ -342,6 +524,13 @@ void test_fill_link_info(void)
if (test__start_subtest("kprobe_multi_invalid_ubuff"))
test_kprobe_multi_fill_link_info(skel, true, true);
+ if (test__start_subtest("uprobe_multi_link_info"))
+ test_uprobe_multi_fill_link_info(skel, false, false, -1);
+ if (test__start_subtest("uretprobe_multi_link_info"))
+ test_uprobe_multi_fill_link_info(skel, true, false, 0);
+ if (test__start_subtest("uprobe_multi_invalid"))
+ test_uprobe_multi_fill_link_info(skel, false, true, -1);
+
cleanup:
test_fill_link_info__destroy(skel);
}
diff --git a/tools/testing/selftests/bpf/progs/test_fill_link_info.c b/tools/testing/selftests/bpf/progs/test_fill_link_info.c
index 564f402d56fe..69509f8bb680 100644
--- a/tools/testing/selftests/bpf/progs/test_fill_link_info.c
+++ b/tools/testing/selftests/bpf/progs/test_fill_link_info.c
@@ -39,4 +39,10 @@ int BPF_PROG(kmulti_run)
return 0;
}
+SEC("uprobe.multi")
+int BPF_PROG(umulti_run)
+{
+ return 0;
+}
+
char _license[] SEC("license") = "GPL";
--
2.41.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 5/6] selftests/bpf: Add link_info test for uprobe_multi link
2023-10-25 20:24 ` [PATCH bpf-next 5/6] selftests/bpf: Add link_info test for uprobe_multi link Jiri Olsa
@ 2023-10-26 18:13 ` Song Liu
2023-11-01 22:27 ` Andrii Nakryiko
1 sibling, 0 replies; 33+ messages in thread
From: Song Liu @ 2023-10-26 18:13 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Oct 25, 2023 at 1:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding fill_link_info test for uprobe_multi link.
>
> Setting up uprobes with bogus ref_ctr_offsets and cookie values
> to test all the bpf_link_info::uprobe_multi fields.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Song Liu <song@kernel.org>
> ---
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH bpf-next 5/6] selftests/bpf: Add link_info test for uprobe_multi link
2023-10-25 20:24 ` [PATCH bpf-next 5/6] selftests/bpf: Add link_info test for uprobe_multi link Jiri Olsa
2023-10-26 18:13 ` Song Liu
@ 2023-11-01 22:27 ` Andrii Nakryiko
1 sibling, 0 replies; 33+ messages in thread
From: Andrii Nakryiko @ 2023-11-01 22:27 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Oct 25, 2023 at 1:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding fill_link_info test for uprobe_multi link.
>
> Setting up uprobes with bogus ref_ctr_offsets and cookie values
> to test all the bpf_link_info::uprobe_multi fields.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> .../selftests/bpf/prog_tests/fill_link_info.c | 189 ++++++++++++++++++
> .../selftests/bpf/progs/test_fill_link_info.c | 6 +
> 2 files changed, 195 insertions(+)
>
Please don't use C++-style comments.
[...]
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH bpf-next 6/6] bpftool: Add support to display uprobe_multi links
2023-10-25 20:24 [PATCH bpf-next 0/6] bpf: Add link_info support for uprobe multi link Jiri Olsa
` (4 preceding siblings ...)
2023-10-25 20:24 ` [PATCH bpf-next 5/6] selftests/bpf: Add link_info test for uprobe_multi link Jiri Olsa
@ 2023-10-25 20:24 ` Jiri Olsa
2023-10-26 18:27 ` Song Liu
2023-10-30 10:17 ` Quentin Monnet
5 siblings, 2 replies; 33+ messages in thread
From: Jiri Olsa @ 2023-10-25 20:24 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
Adding support to display details for uprobe_multi links,
both plain:
# bpftool link -p
...
24: uprobe_multi prog 126
uprobe.multi path /home/jolsa/bpf/test_progs func_cnt 3 pid 4143
offset ref_ctr_offset cookies
0xd1f88 0xf5d5a8 0xdead
0xd1f8f 0xf5d5aa 0xbeef
0xd1f96 0xf5d5ac 0xcafe
and json:
# bpftool link -p | jq
[{
...
},{
"id": 24,
"type": "uprobe_multi",
"prog_id": 126,
"retprobe": false,
"path": "/home/jolsa/bpf/test_progs",
"func_cnt": 3,
"pid": 4143,
"funcs": [{
"offset": 860040,
"ref_ctr_offset": 16111016,
"cookie": 57005
},{
"offset": 860047,
"ref_ctr_offset": 16111018,
"cookie": 48879
},{
"offset": 860054,
"ref_ctr_offset": 16111020,
"cookie": 51966
}
]
}
]
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/bpf/bpftool/link.c | 102 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 100 insertions(+), 2 deletions(-)
diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
index a1528cde81ab..21c7e4f038c4 100644
--- a/tools/bpf/bpftool/link.c
+++ b/tools/bpf/bpftool/link.c
@@ -294,6 +294,34 @@ show_kprobe_multi_json(struct bpf_link_info *info, json_writer_t *wtr)
jsonw_end_array(json_wtr);
}
+#define U64_PTR(__val) ((__u64 *) u64_to_ptr(__val))
+
+static void
+show_uprobe_multi_json(struct bpf_link_info *info, json_writer_t *wtr)
+{
+ __u32 i;
+
+ jsonw_bool_field(json_wtr, "retprobe",
+ info->uprobe_multi.flags & BPF_F_UPROBE_MULTI_RETURN);
+ jsonw_string_field(json_wtr, "path", (char *) u64_to_ptr(info->uprobe_multi.path));
+ jsonw_uint_field(json_wtr, "func_cnt", info->uprobe_multi.count);
+ jsonw_int_field(json_wtr, "pid", (int) info->uprobe_multi.pid);
+ jsonw_name(json_wtr, "funcs");
+ jsonw_start_array(json_wtr);
+
+ for (i = 0; i < info->uprobe_multi.count; i++) {
+ jsonw_start_object(json_wtr);
+ jsonw_uint_field(json_wtr, "offset",
+ U64_PTR(info->uprobe_multi.offsets)[i]);
+ jsonw_uint_field(json_wtr, "ref_ctr_offset",
+ U64_PTR(info->uprobe_multi.ref_ctr_offsets)[i]);
+ jsonw_uint_field(json_wtr, "cookie",
+ U64_PTR(info->uprobe_multi.cookies)[i]);
+ jsonw_end_object(json_wtr);
+ }
+ jsonw_end_array(json_wtr);
+}
+
static void
show_perf_event_kprobe_json(struct bpf_link_info *info, json_writer_t *wtr)
{
@@ -465,6 +493,9 @@ static int show_link_close_json(int fd, struct bpf_link_info *info)
case BPF_LINK_TYPE_KPROBE_MULTI:
show_kprobe_multi_json(info, json_wtr);
break;
+ case BPF_LINK_TYPE_UPROBE_MULTI:
+ show_uprobe_multi_json(info, json_wtr);
+ break;
case BPF_LINK_TYPE_PERF_EVENT:
switch (info->perf_event.type) {
case BPF_PERF_EVENT_EVENT:
@@ -674,6 +705,33 @@ static void show_kprobe_multi_plain(struct bpf_link_info *info)
}
}
+static void show_uprobe_multi_plain(struct bpf_link_info *info)
+{
+ __u32 i;
+
+ if (!info->uprobe_multi.count)
+ return;
+
+ if (info->uprobe_multi.flags & BPF_F_UPROBE_MULTI_RETURN)
+ printf("\n\turetprobe.multi ");
+ else
+ printf("\n\tuprobe.multi ");
+
+ printf("path %s ", (char *) u64_to_ptr(info->uprobe_multi.path));
+ printf("func_cnt %u ", info->uprobe_multi.count);
+
+ if (info->uprobe_multi.pid != (__u32) -1)
+ printf("pid %d ", info->uprobe_multi.pid);
+
+ printf("\n\t%-16s %-16s %-16s", "offset", "ref_ctr_offset", "cookies");
+ for (i = 0; i < info->uprobe_multi.count; i++) {
+ printf("\n\t0x%-16llx 0x%-16llx 0x%-16llx",
+ U64_PTR(info->uprobe_multi.offsets)[i],
+ U64_PTR(info->uprobe_multi.ref_ctr_offsets)[i],
+ U64_PTR(info->uprobe_multi.cookies)[i]);
+ }
+}
+
static void show_perf_event_kprobe_plain(struct bpf_link_info *info)
{
const char *buf;
@@ -807,6 +865,9 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
case BPF_LINK_TYPE_KPROBE_MULTI:
show_kprobe_multi_plain(info);
break;
+ case BPF_LINK_TYPE_UPROBE_MULTI:
+ show_uprobe_multi_plain(info);
+ break;
case BPF_LINK_TYPE_PERF_EVENT:
switch (info->perf_event.type) {
case BPF_PERF_EVENT_EVENT:
@@ -846,8 +907,10 @@ static int show_link_close_plain(int fd, struct bpf_link_info *info)
static int do_show_link(int fd)
{
+ __u64 *ref_ctr_offsets = NULL, *offsets = NULL, *cookies = NULL;
struct bpf_link_info info;
__u32 len = sizeof(info);
+ char path_buf[PATH_MAX];
__u64 *addrs = NULL;
char buf[PATH_MAX];
int count;
@@ -889,6 +952,39 @@ static int do_show_link(int fd)
goto again;
}
}
+ if (info.type == BPF_LINK_TYPE_UPROBE_MULTI &&
+ !info.uprobe_multi.offsets) {
+ count = info.uprobe_multi.count;
+ if (count) {
+ offsets = calloc(count, sizeof(__u64));
+ if (!offsets) {
+ p_err("mem alloc failed");
+ close(fd);
+ return -ENOMEM;
+ }
+ info.uprobe_multi.offsets = ptr_to_u64(offsets);
+ ref_ctr_offsets = calloc(count, sizeof(__u64));
+ if (!ref_ctr_offsets) {
+ p_err("mem alloc failed");
+ free(offsets);
+ close(fd);
+ return -ENOMEM;
+ }
+ info.uprobe_multi.ref_ctr_offsets = ptr_to_u64(ref_ctr_offsets);
+ cookies = calloc(count, sizeof(__u64));
+ if (!cookies) {
+ p_err("mem alloc failed");
+ free(cookies);
+ free(offsets);
+ close(fd);
+ return -ENOMEM;
+ }
+ info.uprobe_multi.cookies = ptr_to_u64(cookies);
+ info.uprobe_multi.path = ptr_to_u64(path_buf);
+ info.uprobe_multi.path_max = sizeof(path_buf);
+ goto again;
+ }
+ }
if (info.type == BPF_LINK_TYPE_PERF_EVENT) {
switch (info.perf_event.type) {
case BPF_PERF_EVENT_TRACEPOINT:
@@ -924,8 +1020,10 @@ static int do_show_link(int fd)
else
show_link_close_plain(fd, &info);
- if (addrs)
- free(addrs);
+ free(ref_ctr_offsets);
+ free(cookies);
+ free(offsets);
+ free(addrs);
close(fd);
return 0;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 6/6] bpftool: Add support to display uprobe_multi links
2023-10-25 20:24 ` [PATCH bpf-next 6/6] bpftool: Add support to display uprobe_multi links Jiri Olsa
@ 2023-10-26 18:27 ` Song Liu
2023-10-30 10:17 ` Quentin Monnet
1 sibling, 0 replies; 33+ messages in thread
From: Song Liu @ 2023-10-26 18:27 UTC (permalink / raw)
To: Jiri Olsa
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
On Wed, Oct 25, 2023 at 1:25 PM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Adding support to display details for uprobe_multi links,
> both plain:
>
> # bpftool link -p
> ...
> 24: uprobe_multi prog 126
> uprobe.multi path /home/jolsa/bpf/test_progs func_cnt 3 pid 4143
> offset ref_ctr_offset cookies
> 0xd1f88 0xf5d5a8 0xdead
> 0xd1f8f 0xf5d5aa 0xbeef
> 0xd1f96 0xf5d5ac 0xcafe
>
> and json:
>
> # bpftool link -p | jq
> [{
> ...
> },{
> "id": 24,
> "type": "uprobe_multi",
> "prog_id": 126,
> "retprobe": false,
> "path": "/home/jolsa/bpf/test_progs",
> "func_cnt": 3,
> "pid": 4143,
> "funcs": [{
> "offset": 860040,
> "ref_ctr_offset": 16111016,
> "cookie": 57005
> },{
> "offset": 860047,
> "ref_ctr_offset": 16111018,
> "cookie": 48879
> },{
> "offset": 860054,
> "ref_ctr_offset": 16111020,
> "cookie": 51966
> }
> ]
> }
> ]
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Song Liu <song@kernel.org>
^ permalink raw reply [flat|nested] 33+ messages in thread* Re: [PATCH bpf-next 6/6] bpftool: Add support to display uprobe_multi links
2023-10-25 20:24 ` [PATCH bpf-next 6/6] bpftool: Add support to display uprobe_multi links Jiri Olsa
2023-10-26 18:27 ` Song Liu
@ 2023-10-30 10:17 ` Quentin Monnet
1 sibling, 0 replies; 33+ messages in thread
From: Quentin Monnet @ 2023-10-30 10:17 UTC (permalink / raw)
To: Jiri Olsa, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko
Cc: bpf, Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Yafang Shao
2023-10-25 21:25 UTC+0100 ~ Jiri Olsa
> Adding support to display details for uprobe_multi links,
> both plain:
>
> # bpftool link -p
> ...
> 24: uprobe_multi prog 126
> uprobe.multi path /home/jolsa/bpf/test_progs func_cnt 3 pid 4143
> offset ref_ctr_offset cookies
> 0xd1f88 0xf5d5a8 0xdead
> 0xd1f8f 0xf5d5aa 0xbeef
> 0xd1f96 0xf5d5ac 0xcafe
>
> and json:
>
> # bpftool link -p | jq
> [{
> ...
> },{
> "id": 24,
> "type": "uprobe_multi",
> "prog_id": 126,
> "retprobe": false,
> "path": "/home/jolsa/bpf/test_progs",
> "func_cnt": 3,
> "pid": 4143,
> "funcs": [{
> "offset": 860040,
> "ref_ctr_offset": 16111016,
> "cookie": 57005
> },{
> "offset": 860047,
> "ref_ctr_offset": 16111018,
> "cookie": 48879
> },{
> "offset": 860054,
> "ref_ctr_offset": 16111020,
> "cookie": 51966
> }
> ]
> }
> ]
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/bpf/bpftool/link.c | 102 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/tools/bpf/bpftool/link.c b/tools/bpf/bpftool/link.c
> index a1528cde81ab..21c7e4f038c4 100644
> --- a/tools/bpf/bpftool/link.c
> +++ b/tools/bpf/bpftool/link.c
> @@ -889,6 +952,39 @@ static int do_show_link(int fd)
> goto again;
> }
> }
> + if (info.type == BPF_LINK_TYPE_UPROBE_MULTI &&
> + !info.uprobe_multi.offsets) {
> + count = info.uprobe_multi.count;
> + if (count) {
> + offsets = calloc(count, sizeof(__u64));
> + if (!offsets) {
> + p_err("mem alloc failed");
> + close(fd);
> + return -ENOMEM;
> + }
> + info.uprobe_multi.offsets = ptr_to_u64(offsets);
> + ref_ctr_offsets = calloc(count, sizeof(__u64));
> + if (!ref_ctr_offsets) {
> + p_err("mem alloc failed");
> + free(offsets);
> + close(fd);
> + return -ENOMEM;
> + }
> + info.uprobe_multi.ref_ctr_offsets = ptr_to_u64(ref_ctr_offsets);
> + cookies = calloc(count, sizeof(__u64));
> + if (!cookies) {
> + p_err("mem alloc failed");
> + free(cookies);
> + free(offsets);
> + close(fd);
> + return -ENOMEM;
> + }
> + info.uprobe_multi.cookies = ptr_to_u64(cookies);
> + info.uprobe_multi.path = ptr_to_u64(path_buf);
> + info.uprobe_multi.path_max = sizeof(path_buf);
It seems we're not using "info.uprobe_multi.path_max" after setting it,
although there's no harm in retrieving the value so OK.
Reviewed-by: Quentin Monnet <quentin@isovalent.com>
^ permalink raw reply [flat|nested] 33+ messages in thread