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

[John Fastabend <john.fastabend@gmail.com>]

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