Re: [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock

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

Jakub Sitnicki wrote:
> On Thu, Nov 30, 2023 at 07:23 PM -08, John Fastabend wrote:
> > Add test to sockmap_basic to ensure af_unix sockets that are not connected
> > can not be added to the map. Ensure we keep DGRAM sockets working however
> > as these will not be connected typically.
> >
> > Signed-off-by: John Fastabend <john.fastabend@gmail.com>
> > ---
> >  .../selftests/bpf/prog_tests/sockmap_basic.c  | 34 +++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > index f75f84d0b3d7..ad96f4422def 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/sockmap_basic.c
> > @@ -524,6 +524,37 @@ static void test_sockmap_skb_verdict_peek(void)
> >  	test_sockmap_pass_prog__destroy(pass);
> >  }
> >  
> > +static void test_sockmap_unconnected_unix(void)
> > +{
> > +	int err, map, stream = 0, dgram = 0, zero = 0;
> > +	struct test_sockmap_pass_prog *skel;
> > +
> > +	skel = test_sockmap_pass_prog__open_and_load();
> > +	if (!ASSERT_OK_PTR(skel, "open_and_load"))
> > +		return;
> > +
> > +	map = bpf_map__fd(skel->maps.sock_map_rx);
> > +
> > +	stream = xsocket(AF_UNIX, SOCK_STREAM, 0);
> > +	if (!ASSERT_GT(stream, -1, "socket(AF_UNIX, SOCK_STREAM)"))
> > +		return;
> 
> Isn't it redudant to use both the xsocket wrapper and ASSERT_* macro?
> Or is there some debugging value that comes from that, which I missed?

It looks like xsocket does an error_at_liine() so guess not you
will have the line number if it fails so will know if it was stream
or dgram that failed. Probably can just drop the if altogether and
let the thing fall through and fail.

> 
> > +
> > +	dgram = xsocket(AF_UNIX, SOCK_DGRAM, 0);
> > +	if (!ASSERT_GT(dgram, -1, "socket(AF_UNIX, SOCK_DGRAM)")) {
> > +		close(stream);
> > +		return;
> > +	}
> > +
> > +	err = bpf_map_update_elem(map, &zero, &stream, BPF_ANY);
> > +	ASSERT_ERR(err, "bpf_map_update_elem(stream)");
> > +
> > +	err = bpf_map_update_elem(map, &zero, &dgram, BPF_ANY);
> > +	ASSERT_OK(err, "bpf_map_update_elem(dgram)");
> > +
> > +	close(stream);
> > +	close(dgram);
> > +}
> > +
> >  void test_sockmap_basic(void)
> >  {
> >  	if (test__start_subtest("sockmap create_update_free"))
> > @@ -566,4 +597,7 @@ void test_sockmap_basic(void)
> >  		test_sockmap_skb_verdict_fionread(false);
> >  	if (test__start_subtest("sockmap skb_verdict msg_f_peek"))
> >  		test_sockmap_skb_verdict_peek();
> > +
> > +	if (test__start_subtest("sockmap unconnected af_unix"))
> > +		test_sockmap_unconnected_unix();
> >  }
>