* Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
2022-06-21 7:01 [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test Jörn-Thorben Hinz
@ 2022-06-21 17:00 ` Daniel Borkmann
2022-06-21 17:09 ` Jakub Sitnicki
2022-06-21 18:20 ` [External] " Jörn-Thorben Hinz
2022-06-21 19:54 ` John Fastabend
` (2 subsequent siblings)
3 siblings, 2 replies; 9+ messages in thread
From: Daniel Borkmann @ 2022-06-21 17:00 UTC (permalink / raw)
To: Jörn-Thorben Hinz, bpf
Cc: Alexei Starovoitov, Andrii Nakryiko, Jakub Sitnicki
On 6/21/22 9:01 AM, Jörn-Thorben Hinz wrote:
> test_sock_fields__detach() got called with a null pointer here when one
> of the CHECKs or ASSERTs up to the test_sock_fields__open_and_load()
> call resulted in a jump to the "done" label.
>
> A skeletons *__detach() is not safe to call with a null pointer, though.
> This led to a segfault.
>
> Go the easy route and only call test_sock_fields__destroy() which is
> null-pointer safe and includes detaching.
>
> Came across this while looking[1] to introduce the usage of
> bpf_tcp_helpers.h (included in progs/test_sock_fields.c) together with
> vmlinux.h.
>
> [1] https://lore.kernel.org/bpf/629bc069dd807d7ac646f836e9dca28bbc1108e2.camel@mailbox.tu-berlin.de/
>
> Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock tests for dst_port loads")
> Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> ---
> tools/testing/selftests/bpf/prog_tests/sock_fields.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> index 9d211b5c22c4..7d23166c77af 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> @@ -394,7 +394,6 @@ void serial_test_sock_fields(void)
> test();
>
> done:
> - test_sock_fields__detach(skel);
> test_sock_fields__destroy(skel);
> if (child_cg_fd >= 0)
> close(child_cg_fd);
>
Great catch! I think we have similar detach & destroy pattern in a number
of places in selftests.
Should we rather just move the label, like:
diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
index 9d211b5c22c4..e8a947241e37 100644
--- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
+++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
@@ -393,8 +393,8 @@ void serial_test_sock_fields(void)
test();
-done:
test_sock_fields__detach(skel);
+done:
test_sock_fields__destroy(skel);
if (child_cg_fd >= 0)
close(child_cg_fd);
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
2022-06-21 17:00 ` Daniel Borkmann
@ 2022-06-21 17:09 ` Jakub Sitnicki
2022-06-21 18:20 ` [External] " Jörn-Thorben Hinz
1 sibling, 0 replies; 9+ messages in thread
From: Jakub Sitnicki @ 2022-06-21 17:09 UTC (permalink / raw)
To: Daniel Borkmann, Jörn-Thorben Hinz
Cc: bpf, Alexei Starovoitov, Andrii Nakryiko
On Tue, Jun 21, 2022 at 07:00 PM +02, Daniel Borkmann wrote:
> On 6/21/22 9:01 AM, Jörn-Thorben Hinz wrote:
>> test_sock_fields__detach() got called with a null pointer here when one
>> of the CHECKs or ASSERTs up to the test_sock_fields__open_and_load()
>> call resulted in a jump to the "done" label.
>> A skeletons *__detach() is not safe to call with a null pointer, though.
>> This led to a segfault.
>> Go the easy route and only call test_sock_fields__destroy() which is
>> null-pointer safe and includes detaching.
>> Came across this while looking[1] to introduce the usage of
>> bpf_tcp_helpers.h (included in progs/test_sock_fields.c) together with
>> vmlinux.h.
>> [1]
>> https://lore.kernel.org/bpf/629bc069dd807d7ac646f836e9dca28bbc1108e2.camel@mailbox.tu-berlin.de/
>> Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock tests for
>> dst_port loads")
>> Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
>> ---
>> tools/testing/selftests/bpf/prog_tests/sock_fields.c | 1 -
>> 1 file changed, 1 deletion(-)
>> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
>> b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
>> index 9d211b5c22c4..7d23166c77af 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
>> @@ -394,7 +394,6 @@ void serial_test_sock_fields(void)
>> test();
>> done:
>> - test_sock_fields__detach(skel);
>> test_sock_fields__destroy(skel);
>> if (child_cg_fd >= 0)
>> close(child_cg_fd);
>>
>
> Great catch! I think we have similar detach & destroy pattern in a number
> of places in selftests.
>
> Should we rather just move the label, like:
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> index 9d211b5c22c4..e8a947241e37 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> @@ -393,8 +393,8 @@ void serial_test_sock_fields(void)
>
> test();
>
> -done:
> test_sock_fields__detach(skel);
> +done:
> test_sock_fields__destroy(skel);
> if (child_cg_fd >= 0)
> close(child_cg_fd);
*__destroy() will call bpf_object__detach_skeleton(), so it LGTM.
Thanks for the fix.
Reviewed-by: Jakub Sitnicki <jakub@cloudflare.com>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [External] Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
2022-06-21 17:00 ` Daniel Borkmann
2022-06-21 17:09 ` Jakub Sitnicki
@ 2022-06-21 18:20 ` Jörn-Thorben Hinz
1 sibling, 0 replies; 9+ messages in thread
From: Jörn-Thorben Hinz @ 2022-06-21 18:20 UTC (permalink / raw)
To: Daniel Borkmann, bpf; +Cc: Alexei Starovoitov, Andrii Nakryiko, Jakub Sitnicki
On Tue, 2022-06-21 at 19:00 +0200, Daniel Borkmann wrote:
> On 6/21/22 9:01 AM, Jörn-Thorben Hinz wrote:
> > test_sock_fields__detach() got called with a null pointer here when
> > one
> > of the CHECKs or ASSERTs up to the
> > test_sock_fields__open_and_load()
> > call resulted in a jump to the "done" label.
> >
> > A skeletons *__detach() is not safe to call with a null pointer,
> > though.
> > This led to a segfault.
> >
> > Go the easy route and only call test_sock_fields__destroy() which
> > is
> > null-pointer safe and includes detaching.
> >
> > Came across this while looking[1] to introduce the usage of
> > bpf_tcp_helpers.h (included in progs/test_sock_fields.c) together
> > with
> > vmlinux.h.
> >
> > [1]
> > https://lore.kernel.org/bpf/629bc069dd807d7ac646f836e9dca28bbc1108e2.camel@mailbox.tu-berlin.de/
> >
> > Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock
> > tests for dst_port loads")
> > Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> > ---
> > tools/testing/selftests/bpf/prog_tests/sock_fields.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > index 9d211b5c22c4..7d23166c77af 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > @@ -394,7 +394,6 @@ void serial_test_sock_fields(void)
> > test();
> >
> > done:
> > - test_sock_fields__detach(skel);
> > test_sock_fields__destroy(skel);
> > if (child_cg_fd >= 0)
> > close(child_cg_fd);
> >
>
> Great catch! I think we have similar detach & destroy pattern in a
> number
> of places in selftests.
Did a quick grep for other __detach(skel) calls yesterday. I didn’t
find similar places that were too obviously problematic.
>
> Should we rather just move the label, like:
Sure, if you would prefer that? Let me know.
Since this test—unlike others—does not attach the skel twice (like
prog_tests/test_lsm.c), or reads/asserts values from the skel’s data
sections between detach and destroy (like prog_tests/timer.c), my
thought was to just let __destroy() do all the clean-up.
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> index 9d211b5c22c4..e8a947241e37 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> @@ -393,8 +393,8 @@ void serial_test_sock_fields(void)
>
> test();
>
> -done:
> test_sock_fields__detach(skel);
> +done:
> test_sock_fields__destroy(skel);
> if (child_cg_fd >= 0)
> close(child_cg_fd);
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
2022-06-21 7:01 [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test Jörn-Thorben Hinz
2022-06-21 17:00 ` Daniel Borkmann
@ 2022-06-21 19:54 ` John Fastabend
2022-06-21 20:29 ` Jörn-Thorben Hinz
2022-06-23 16:11 ` Martin KaFai Lau
2022-06-23 18:00 ` patchwork-bot+netdevbpf
3 siblings, 1 reply; 9+ messages in thread
From: John Fastabend @ 2022-06-21 19:54 UTC (permalink / raw)
To: Jörn-Thorben Hinz, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Jakub Sitnicki, Jörn-Thorben Hinz
Jörn-Thorben Hinz wrote:
> test_sock_fields__detach() got called with a null pointer here when one
> of the CHECKs or ASSERTs up to the test_sock_fields__open_and_load()
> call resulted in a jump to the "done" label.
>
> A skeletons *__detach() is not safe to call with a null pointer, though.
> This led to a segfault.
>
> Go the easy route and only call test_sock_fields__destroy() which is
> null-pointer safe and includes detaching.
>
> Came across this while looking[1] to introduce the usage of
> bpf_tcp_helpers.h (included in progs/test_sock_fields.c) together with
> vmlinux.h.
>
> [1] https://lore.kernel.org/bpf/629bc069dd807d7ac646f836e9dca28bbc1108e2.camel@mailbox.tu-berlin.de/
>
> Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock tests for dst_port loads")
> Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> ---
> tools/testing/selftests/bpf/prog_tests/sock_fields.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> index 9d211b5c22c4..7d23166c77af 100644
> --- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> +++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> @@ -394,7 +394,6 @@ void serial_test_sock_fields(void)
> test();
>
> done:
> - test_sock_fields__detach(skel);
> test_sock_fields__destroy(skel);
> if (child_cg_fd >= 0)
> close(child_cg_fd);
> --
> 2.30.2
>
But we should still call __detach(skel) after the !skel check
is done I assume. So rather than remove it should add a new label
and jump to that,
done:
test_sock_fields__detach();
done_no_skel:
test_sock_fields__destroy()
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
2022-06-21 19:54 ` John Fastabend
@ 2022-06-21 20:29 ` Jörn-Thorben Hinz
2022-06-23 3:29 ` John Fastabend
0 siblings, 1 reply; 9+ messages in thread
From: Jörn-Thorben Hinz @ 2022-06-21 20:29 UTC (permalink / raw)
To: John Fastabend, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Jakub Sitnicki
On Tue, 2022-06-21 at 12:54 -0700, John Fastabend wrote:
> Jörn-Thorben Hinz wrote:
> > test_sock_fields__detach() got called with a null pointer here when
> > one
> > of the CHECKs or ASSERTs up to the
> > test_sock_fields__open_and_load()
> > call resulted in a jump to the "done" label.
> >
> > A skeletons *__detach() is not safe to call with a null pointer,
> > though.
> > This led to a segfault.
> >
> > Go the easy route and only call test_sock_fields__destroy() which
> > is
> > null-pointer safe and includes detaching.
> >
> > Came across this while looking[1] to introduce the usage of
> > bpf_tcp_helpers.h (included in progs/test_sock_fields.c) together
> > with
> > vmlinux.h.
> >
> > [1]
> > https://lore.kernel.org/bpf/629bc069dd807d7ac646f836e9dca28bbc1108e2.camel@mailbox.tu-berlin.de/
> >
> > Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock
> > tests for dst_port loads")
> > Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> > ---
> > tools/testing/selftests/bpf/prog_tests/sock_fields.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > index 9d211b5c22c4..7d23166c77af 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > @@ -394,7 +394,6 @@ void serial_test_sock_fields(void)
> > test();
> >
> > done:
> > - test_sock_fields__detach(skel);
> > test_sock_fields__destroy(skel);
> > if (child_cg_fd >= 0)
> > close(child_cg_fd);
> > --
> > 2.30.2
> >
>
> But we should still call __detach(skel) after the !skel check
> is done I assume.
If I’m not mistaken, that’s not necessary for a proper clean-up. It
should be more of a stylistic question. See the parallel message from
Daniel (and replies).
test_sock_fields__detach() directly translates to
bpf_object__detach_skeleton(). test_sock_fields__destroy() basically
translates to bpf_object__destroy_skeleton(), including a null-ptr
check.
But bpf_object__destroy_skeleton() calls bpf_object__detach_skeleton()
as its first step. So calling __detach()/__detach_skeleton() explicitly
and separately is not necessary for a clean exit, if not otherwise
required.
> So rather than remove it should add a new label
> and jump to that,
>
>
> done:
> test_sock_fields__detach();
> done_no_skel:
> test_sock_fields__destroy()
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
2022-06-21 20:29 ` Jörn-Thorben Hinz
@ 2022-06-23 3:29 ` John Fastabend
0 siblings, 0 replies; 9+ messages in thread
From: John Fastabend @ 2022-06-23 3:29 UTC (permalink / raw)
To: Jörn-Thorben Hinz, John Fastabend, bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko, Jakub Sitnicki
Jörn-Thorben Hinz wrote:
> On Tue, 2022-06-21 at 12:54 -0700, John Fastabend wrote:
> > Jörn-Thorben Hinz wrote:
> > > test_sock_fields__detach() got called with a null pointer here when
> > > one
> > > of the CHECKs or ASSERTs up to the
> > > test_sock_fields__open_and_load()
> > > call resulted in a jump to the "done" label.
> > >
> > > A skeletons *__detach() is not safe to call with a null pointer,
> > > though.
> > > This led to a segfault.
> > >
> > > Go the easy route and only call test_sock_fields__destroy() which
> > > is
> > > null-pointer safe and includes detaching.
> > >
> > > Came across this while looking[1] to introduce the usage of
> > > bpf_tcp_helpers.h (included in progs/test_sock_fields.c) together
> > > with
> > > vmlinux.h.
> > >
> > > [1]
> > > https://lore.kernel.org/bpf/629bc069dd807d7ac646f836e9dca28bbc1108e2.camel@mailbox.tu-berlin.de/
> > >
> > > Fixes: 8f50f16ff39d ("selftests/bpf: Extend verifier and bpf_sock
> > > tests for dst_port loads")
> > > Signed-off-by: Jörn-Thorben Hinz <jthinz@mailbox.tu-berlin.de>
> > > ---
> > > tools/testing/selftests/bpf/prog_tests/sock_fields.c | 1 -
> > > 1 file changed, 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > > b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > > index 9d211b5c22c4..7d23166c77af 100644
> > > --- a/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > > +++ b/tools/testing/selftests/bpf/prog_tests/sock_fields.c
> > > @@ -394,7 +394,6 @@ void serial_test_sock_fields(void)
> > > test();
> > >
> > > done:
> > > - test_sock_fields__detach(skel);
> > > test_sock_fields__destroy(skel);
> > > if (child_cg_fd >= 0)
> > > close(child_cg_fd);
> > > --
> > > 2.30.2
> > >
> >
> > But we should still call __detach(skel) after the !skel check
> > is done I assume.
> If I’m not mistaken, that’s not necessary for a proper clean-up. It
> should be more of a stylistic question. See the parallel message from
> Daniel (and replies).
>
> test_sock_fields__detach() directly translates to
> bpf_object__detach_skeleton(). test_sock_fields__destroy() basically
> translates to bpf_object__destroy_skeleton(), including a null-ptr
> check.
>
> But bpf_object__destroy_skeleton() calls bpf_object__detach_skeleton()
> as its first step. So calling __detach()/__detach_skeleton() explicitly
> and separately is not necessary for a clean exit, if not otherwise
> required.
Seems to be the case nice catch. I'm OK with it as is then.
Acked-by: John Fastabend <john.fastabend@gmail.com>
>
>
> > So rather than remove it should add a new label
> > and jump to that,
> >
> >
> > done:
> > test_sock_fields__detach();
> > done_no_skel:
> > test_sock_fields__destroy()
>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
2022-06-21 7:01 [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test Jörn-Thorben Hinz
2022-06-21 17:00 ` Daniel Borkmann
2022-06-21 19:54 ` John Fastabend
@ 2022-06-23 16:11 ` Martin KaFai Lau
2022-06-23 18:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 9+ messages in thread
From: Martin KaFai Lau @ 2022-06-23 16:11 UTC (permalink / raw)
To: Jörn-Thorben Hinz
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Jakub Sitnicki
On Tue, Jun 21, 2022 at 09:01:16AM +0200, Jörn-Thorben Hinz wrote:
> Go the easy route and only call test_sock_fields__destroy() which is
> null-pointer safe and includes detaching.
Reviewed-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
2022-06-21 7:01 [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test Jörn-Thorben Hinz
` (2 preceding siblings ...)
2022-06-23 16:11 ` Martin KaFai Lau
@ 2022-06-23 18:00 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-06-23 18:00 UTC (permalink / raw)
To: =?utf-8?q?J=C3=B6rn-Thorben_Hinz_=3Cjthinz=40mailbox=2Etu-berlin=2Ede=3E?=
Cc: bpf, ast, daniel, andrii, jakub
Hello:
This patch was applied to bpf/bpf-next.git (master)
by Andrii Nakryiko <andrii@kernel.org>:
On Tue, 21 Jun 2022 09:01:16 +0200 you wrote:
> test_sock_fields__detach() got called with a null pointer here when one
> of the CHECKs or ASSERTs up to the test_sock_fields__open_and_load()
> call resulted in a jump to the "done" label.
>
> A skeletons *__detach() is not safe to call with a null pointer, though.
> This led to a segfault.
>
> [...]
Here is the summary with links:
- [bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test
https://git.kernel.org/bpf/bpf-next/c/6dc7a0baf1a7
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 9+ messages in thread