netdev - Re: [PATCH bpf 2/2] bpf: sockmap, test for unconnected af

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <656a0b35cd1eb_444022086c@john.notmuch>
Date: Fri, 01 Dec 2023 08:35:01 -0800
From: John Fastabend <john.fastabend@...il.com>
To: Jakub Sitnicki <jakub@...udflare.com>, 
 John Fastabend <john.fastabend@...il.com>
Cc: kuniyu@...zon.com, 
 edumazet@...gle.com, 
 bpf@...r.kernel.org, 
 netdev@...r.kernel.org
Subject: Re: [PATCH bpf 2/2] bpf: sockmap, test for unconnected af_unix sock

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@...il.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();
> >  }
>