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

["Jörn-Thorben Hinz" <jthinz@mailbox.tu-berlin.de>]

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