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

[Jakub Sitnicki <jakub@cloudflare.com>]

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>