* [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset
@ 2023-12-13 14:12 Jiri Olsa
2023-12-13 14:12 ` [RFC bpf-next 2/2] selftests/bpf: Add uprobe multi fail test Jiri Olsa
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Jiri Olsa @ 2023-12-13 14:12 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, Oleg Nesterov
Currently the __uprobe_register will return 0 (success) when called with
negative offset. The reason is that the call to register_for_each_vma and
then build_map_info won't return error for negative offset. They just won't
do anything - no matching vma is found so there's no registered breakpoint
for the uprobe.
I don't think we can change the behaviour of __uprobe_register and fail
for negative uprobe offset, because apps might depend on that already.
But I think we can still make the change and check for it on bpf multi
link syscall level.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
kernel/trace/bpf_trace.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index 774cf476a892..0dbf8d9b3ace 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -3397,6 +3397,11 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
goto error_free;
}
+ if (uprobes[i].offset < 0) {
+ err = -EINVAL;
+ goto error_free;
+ }
+
uprobes[i].link = link;
if (flags & BPF_F_UPROBE_MULTI_RETURN)
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [RFC bpf-next 2/2] selftests/bpf: Add uprobe multi fail test
2023-12-13 14:12 [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset Jiri Olsa
@ 2023-12-13 14:12 ` Jiri Olsa
2023-12-13 18:08 ` Alan Maguire
2023-12-13 17:51 ` [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset Alan Maguire
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2023-12-13 14:12 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, Oleg Nesterov
We fail to create uprobe if we pass negative offset,
adding test for that.
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
.../bpf/prog_tests/uprobe_multi_test.c | 27 +++++++++++++++++++
1 file changed, 27 insertions(+)
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 ece260cf2c0b..aebfa7e6bfd6 100644
--- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
+++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
@@ -326,6 +326,31 @@ void test_link_api(void)
__test_link_api(child);
}
+static void test_attach_api_fails(void)
+{
+ LIBBPF_OPTS(bpf_link_create_opts, opts);
+ const char *path = "/proc/self/exe";
+ struct uprobe_multi *skel = NULL;
+ int prog_fd, link_fd;
+ long offset = -1;
+
+ skel = uprobe_multi__open_and_load();
+ if (!ASSERT_OK_PTR(skel, "uprobe_multi__open_and_load"))
+ return;
+
+ /* fail_1 - attach on negative offset */
+ opts.uprobe_multi.path = path;
+ opts.uprobe_multi.offsets = (unsigned long *) &offset;
+ opts.uprobe_multi.cnt = 1;
+
+ prog_fd = bpf_program__fd(skel->progs.uprobe_extra);
+ link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &opts);
+ if (!ASSERT_EQ(link_fd, -EINVAL, "link_fd"))
+ close(link_fd);
+
+ uprobe_multi__destroy(skel);
+}
+
static void test_bench_attach_uprobe(void)
{
long attach_start_ns = 0, attach_end_ns = 0;
@@ -412,4 +437,6 @@ void test_uprobe_multi_test(void)
test_bench_attach_uprobe();
if (test__start_subtest("bench_usdt"))
test_bench_attach_usdt();
+ if (test__start_subtest("attach_api_fails"))
+ test_attach_api_fails();
}
--
2.43.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset
2023-12-13 14:12 [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset Jiri Olsa
2023-12-13 14:12 ` [RFC bpf-next 2/2] selftests/bpf: Add uprobe multi fail test Jiri Olsa
@ 2023-12-13 17:51 ` Alan Maguire
2023-12-13 23:35 ` Song Liu
2023-12-13 23:43 ` Andrii Nakryiko
3 siblings, 0 replies; 10+ messages in thread
From: Alan Maguire @ 2023-12-13 17:51 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, Oleg Nesterov
On 13/12/2023 14:12, Jiri Olsa wrote:
> Currently the __uprobe_register will return 0 (success) when called with
> negative offset. The reason is that the call to register_for_each_vma and
> then build_map_info won't return error for negative offset. They just won't
> do anything - no matching vma is found so there's no registered breakpoint
> for the uprobe.
>
> I don't think we can change the behaviour of __uprobe_register and fail
> for negative uprobe offset, because apps might depend on that already.
>
just my view, but since passing negative offsets never made sense, I
wouldn't be as worried about breaking existing consumers. Regardless of
what a user thought would happen passing a negative value, nothing did,
so that can't have been their intent.
> But I think we can still make the change and check for it on bpf multi
> link syscall level.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
> ---
> kernel/trace/bpf_trace.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 774cf476a892..0dbf8d9b3ace 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3397,6 +3397,11 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> goto error_free;
> }
>
> + if (uprobes[i].offset < 0) {
> + err = -EINVAL;
> + goto error_free;
> + }
> +
> uprobes[i].link = link;
>
> if (flags & BPF_F_UPROBE_MULTI_RETURN)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC bpf-next 2/2] selftests/bpf: Add uprobe multi fail test
2023-12-13 14:12 ` [RFC bpf-next 2/2] selftests/bpf: Add uprobe multi fail test Jiri Olsa
@ 2023-12-13 18:08 ` Alan Maguire
2023-12-14 9:15 ` Jiri Olsa
0 siblings, 1 reply; 10+ messages in thread
From: Alan Maguire @ 2023-12-13 18:08 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, Oleg Nesterov
On 13/12/2023 14:12, Jiri Olsa wrote:
> We fail to create uprobe if we pass negative offset,
> adding test for that.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
small suggestion below..
> ---
> .../bpf/prog_tests/uprobe_multi_test.c | 27 +++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> 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 ece260cf2c0b..aebfa7e6bfd6 100644
> --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> @@ -326,6 +326,31 @@ void test_link_api(void)
> __test_link_api(child);
> }
>
> +static void test_attach_api_fails(void)
> +{
> + LIBBPF_OPTS(bpf_link_create_opts, opts);
> + const char *path = "/proc/self/exe";
> + struct uprobe_multi *skel = NULL;
> + int prog_fd, link_fd;
> + long offset = -1;
> +
> + skel = uprobe_multi__open_and_load();
> + if (!ASSERT_OK_PTR(skel, "uprobe_multi__open_and_load"))
> + return;
> +
> + /* fail_1 - attach on negative offset */
> + opts.uprobe_multi.path = path;
> + opts.uprobe_multi.offsets = (unsigned long *) &offset;
> + opts.uprobe_multi.cnt = 1;
> +
> + prog_fd = bpf_program__fd(skel->progs.uprobe_extra);
> + link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &opts);
> + if (!ASSERT_EQ(link_fd, -EINVAL, "link_fd"))
> + close(link_fd);
> +
would it be worth exercising a few additional error cases here? not
specifying offsets/cnt triggers an EINVAL too.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset
2023-12-13 14:12 [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset Jiri Olsa
2023-12-13 14:12 ` [RFC bpf-next 2/2] selftests/bpf: Add uprobe multi fail test Jiri Olsa
2023-12-13 17:51 ` [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset Alan Maguire
@ 2023-12-13 23:35 ` Song Liu
2023-12-14 9:22 ` Jiri Olsa
2023-12-13 23:43 ` Andrii Nakryiko
3 siblings, 1 reply; 10+ messages in thread
From: Song Liu @ 2023-12-13 23:35 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, Oleg Nesterov
On Wed, Dec 13, 2023 at 6:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently the __uprobe_register will return 0 (success) when called with
> negative offset. The reason is that the call to register_for_each_vma and
> then build_map_info won't return error for negative offset. They just won't
> do anything - no matching vma is found so there's no registered breakpoint
> for the uprobe.
>
> I don't think we can change the behaviour of __uprobe_register and fail
> for negative uprobe offset, because apps might depend on that already.
>
> But I think we can still make the change and check for it on bpf multi
> link syscall level.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/trace/bpf_trace.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 774cf476a892..0dbf8d9b3ace 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3397,6 +3397,11 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> goto error_free;
> }
>
> + if (uprobes[i].offset < 0) {
> + err = -EINVAL;
> + goto error_free;
> + }
> +
nit: We have 3 __get_user() here. How about we move __get_user(offset)
to the first
and check offset immediately after it? This will save us a few
__get_user() in the
error path.
Thanks,
Song
> uprobes[i].link = link;
>
> if (flags & BPF_F_UPROBE_MULTI_RETURN)
> --
> 2.43.0
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset
2023-12-13 14:12 [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset Jiri Olsa
` (2 preceding siblings ...)
2023-12-13 23:35 ` Song Liu
@ 2023-12-13 23:43 ` Andrii Nakryiko
2023-12-14 9:07 ` Jiri Olsa
3 siblings, 1 reply; 10+ messages in thread
From: Andrii Nakryiko @ 2023-12-13 23:43 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, Oleg Nesterov
On Wed, Dec 13, 2023 at 6:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
>
> Currently the __uprobe_register will return 0 (success) when called with
> negative offset. The reason is that the call to register_for_each_vma and
> then build_map_info won't return error for negative offset. They just won't
> do anything - no matching vma is found so there's no registered breakpoint
> for the uprobe.
>
> I don't think we can change the behaviour of __uprobe_register and fail
> for negative uprobe offset, because apps might depend on that already.
>
> But I think we can still make the change and check for it on bpf multi
> link syscall level.
>
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> kernel/trace/bpf_trace.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 774cf476a892..0dbf8d9b3ace 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -3397,6 +3397,11 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> goto error_free;
> }
>
> + if (uprobes[i].offset < 0) {
offset in UAPI is defined as unsigned, so how can it be negative?
> + err = -EINVAL;
> + goto error_free;
> + }
> +
> uprobes[i].link = link;
>
> if (flags & BPF_F_UPROBE_MULTI_RETURN)
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset
2023-12-13 23:43 ` Andrii Nakryiko
@ 2023-12-14 9:07 ` Jiri Olsa
2023-12-14 23:28 ` Andrii Nakryiko
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2023-12-14 9:07 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, Oleg Nesterov
On Wed, Dec 13, 2023 at 03:43:04PM -0800, Andrii Nakryiko wrote:
> On Wed, Dec 13, 2023 at 6:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently the __uprobe_register will return 0 (success) when called with
> > negative offset. The reason is that the call to register_for_each_vma and
> > then build_map_info won't return error for negative offset. They just won't
> > do anything - no matching vma is found so there's no registered breakpoint
> > for the uprobe.
> >
> > I don't think we can change the behaviour of __uprobe_register and fail
> > for negative uprobe offset, because apps might depend on that already.
> >
> > But I think we can still make the change and check for it on bpf multi
> > link syscall level.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/trace/bpf_trace.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 774cf476a892..0dbf8d9b3ace 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3397,6 +3397,11 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > goto error_free;
> > }
> >
> > + if (uprobes[i].offset < 0) {
>
> offset in UAPI is defined as unsigned, so how can it be negative?
right, but then it's passed to uprobe_register_refctr as loff_t which is 'long long'
jirka
>
> > + err = -EINVAL;
> > + goto error_free;
> > + }
> > +
> > uprobes[i].link = link;
> >
> > if (flags & BPF_F_UPROBE_MULTI_RETURN)
> > --
> > 2.43.0
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC bpf-next 2/2] selftests/bpf: Add uprobe multi fail test
2023-12-13 18:08 ` Alan Maguire
@ 2023-12-14 9:15 ` Jiri Olsa
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Olsa @ 2023-12-14 9:15 UTC (permalink / raw)
To: Alan Maguire
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, bpf,
Martin KaFai Lau, Song Liu, Yonghong Song, John Fastabend,
KP Singh, Stanislav Fomichev, Hao Luo, Oleg Nesterov
On Wed, Dec 13, 2023 at 06:08:54PM +0000, Alan Maguire wrote:
> On 13/12/2023 14:12, Jiri Olsa wrote:
> > We fail to create uprobe if we pass negative offset,
> > adding test for that.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
>
> Reviewed-by: Alan Maguire <alan.maguire@oracle.com>
>
> small suggestion below..
>
> > ---
> > .../bpf/prog_tests/uprobe_multi_test.c | 27 +++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > 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 ece260cf2c0b..aebfa7e6bfd6 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/uprobe_multi_test.c
> > @@ -326,6 +326,31 @@ void test_link_api(void)
> > __test_link_api(child);
> > }
> >
> > +static void test_attach_api_fails(void)
> > +{
> > + LIBBPF_OPTS(bpf_link_create_opts, opts);
> > + const char *path = "/proc/self/exe";
> > + struct uprobe_multi *skel = NULL;
> > + int prog_fd, link_fd;
> > + long offset = -1;
> > +
> > + skel = uprobe_multi__open_and_load();
> > + if (!ASSERT_OK_PTR(skel, "uprobe_multi__open_and_load"))
> > + return;
> > +
> > + /* fail_1 - attach on negative offset */
> > + opts.uprobe_multi.path = path;
> > + opts.uprobe_multi.offsets = (unsigned long *) &offset;
> > + opts.uprobe_multi.cnt = 1;
> > +
> > + prog_fd = bpf_program__fd(skel->progs.uprobe_extra);
> > + link_fd = bpf_link_create(prog_fd, 0, BPF_TRACE_UPROBE_MULTI, &opts);
> > + if (!ASSERT_EQ(link_fd, -EINVAL, "link_fd"))
> > + close(link_fd);
> > +
>
> would it be worth exercising a few additional error cases here? not
> specifying offsets/cnt triggers an EINVAL too.
yes, I think we can add more tests in here, will check
thanks,
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset
2023-12-13 23:35 ` Song Liu
@ 2023-12-14 9:22 ` Jiri Olsa
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Olsa @ 2023-12-14 9:22 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, Oleg Nesterov
On Wed, Dec 13, 2023 at 03:35:07PM -0800, Song Liu wrote:
> On Wed, Dec 13, 2023 at 6:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
> >
> > Currently the __uprobe_register will return 0 (success) when called with
> > negative offset. The reason is that the call to register_for_each_vma and
> > then build_map_info won't return error for negative offset. They just won't
> > do anything - no matching vma is found so there's no registered breakpoint
> > for the uprobe.
> >
> > I don't think we can change the behaviour of __uprobe_register and fail
> > for negative uprobe offset, because apps might depend on that already.
> >
> > But I think we can still make the change and check for it on bpf multi
> > link syscall level.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> > kernel/trace/bpf_trace.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > index 774cf476a892..0dbf8d9b3ace 100644
> > --- a/kernel/trace/bpf_trace.c
> > +++ b/kernel/trace/bpf_trace.c
> > @@ -3397,6 +3397,11 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > goto error_free;
> > }
> >
> > + if (uprobes[i].offset < 0) {
> > + err = -EINVAL;
> > + goto error_free;
> > + }
> > +
>
> nit: We have 3 __get_user() here. How about we move __get_user(offset)
> to the first
> and check offset immediately after it? This will save us a few
> __get_user() in the
> error path.
right, we can move it up
thanks,
jirka
>
> Thanks,
> Song
>
> > uprobes[i].link = link;
> >
> > if (flags & BPF_F_UPROBE_MULTI_RETURN)
> > --
> > 2.43.0
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset
2023-12-14 9:07 ` Jiri Olsa
@ 2023-12-14 23:28 ` Andrii Nakryiko
0 siblings, 0 replies; 10+ messages in thread
From: Andrii Nakryiko @ 2023-12-14 23:28 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, Oleg Nesterov
On Thu, Dec 14, 2023 at 1:07 AM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Wed, Dec 13, 2023 at 03:43:04PM -0800, Andrii Nakryiko wrote:
> > On Wed, Dec 13, 2023 at 6:12 AM Jiri Olsa <jolsa@kernel.org> wrote:
> > >
> > > Currently the __uprobe_register will return 0 (success) when called with
> > > negative offset. The reason is that the call to register_for_each_vma and
> > > then build_map_info won't return error for negative offset. They just won't
> > > do anything - no matching vma is found so there's no registered breakpoint
> > > for the uprobe.
> > >
> > > I don't think we can change the behaviour of __uprobe_register and fail
> > > for negative uprobe offset, because apps might depend on that already.
> > >
> > > But I think we can still make the change and check for it on bpf multi
> > > link syscall level.
> > >
> > > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > > ---
> > > kernel/trace/bpf_trace.c | 5 +++++
> > > 1 file changed, 5 insertions(+)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index 774cf476a892..0dbf8d9b3ace 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -3397,6 +3397,11 @@ int bpf_uprobe_multi_link_attach(const union bpf_attr *attr, struct bpf_prog *pr
> > > goto error_free;
> > > }
> > >
> > > + if (uprobes[i].offset < 0) {
> >
> > offset in UAPI is defined as unsigned, so how can it be negative?
>
> right, but then it's passed to uprobe_register_refctr as loff_t which is 'long long'
ah, so it's not rejected because uprobe_register expects signed offset
(for some reason...) and it only does
if (offset > i_size_read(inode))
got it, thanks.
>
> jirka
>
> >
> > > + err = -EINVAL;
> > > + goto error_free;
> > > + }
> > > +
> > > uprobes[i].link = link;
> > >
> > > if (flags & BPF_F_UPROBE_MULTI_RETURN)
> > > --
> > > 2.43.0
> > >
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-12-14 23:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-13 14:12 [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset Jiri Olsa
2023-12-13 14:12 ` [RFC bpf-next 2/2] selftests/bpf: Add uprobe multi fail test Jiri Olsa
2023-12-13 18:08 ` Alan Maguire
2023-12-14 9:15 ` Jiri Olsa
2023-12-13 17:51 ` [RFC bpf-next 1/2] bpf: Fail uprobe multi link with negative offset Alan Maguire
2023-12-13 23:35 ` Song Liu
2023-12-14 9:22 ` Jiri Olsa
2023-12-13 23:43 ` Andrii Nakryiko
2023-12-14 9:07 ` Jiri Olsa
2023-12-14 23:28 ` Andrii Nakryiko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox