[PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[Jörn-Thorben Hinz]

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

Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[Daniel Borkmann]

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);

Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[Jakub Sitnicki]

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>

Re: [External] Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[Jörn-Thorben Hinz]

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);

RE: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[John Fastabend]

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()

Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[Jörn-Thorben Hinz]

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()

Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[John Fastabend]

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()
> 
> 

Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[Martin KaFai Lau]

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>

Re: [PATCH bpf-next] selftests/bpf: Fix rare segfault in sock_fields prog test

[patchwork-bot+netdevbpf]

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