All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH bpf-next v2 0/2] Add more bpf_*_ct_lookup() selftests
@ 2022-08-09 16:34 Daniel Xu
  2022-08-09 16:34 ` [PATCH bpf-next v2 1/2] selftests/bpf: Add existing connection bpf_*_ct_lookup() test Daniel Xu
  2022-08-09 16:34 ` [PATCH bpf-next v2 2/2] selftests/bpf: Add connmark read test Daniel Xu
  0 siblings, 2 replies; 5+ messages in thread
From: Daniel Xu @ 2022-08-09 16:34 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii, memxor; +Cc: Daniel Xu, linux-kernel

This patchset adds more bpf_*_ct_lookup() selftests. The goal is to test
interaction with netfilter subsystem as well as reading from `struct
nf_conn`. The first is important when migrating legacy systems towards
bpf. The latter is important in general to take full advantage of
connection tracking.

I'll follow this patchset up with support for writing to `struct nf_conn`.

This change will require two changes to BPF CI kconfig:

* CONFIG_NF_CONNTRACK_MARK=y
* CONFIG_NETFILTER_XT_CONNMARK=y

I can put up the PR if this patchset looks good.

Past discussion:
- v1: https://lore.kernel.org/bpf/cover.1659209738.git.dxu@dxuuu.xyz/

Changes since v1:
- Reword commit message / cover letter to not mention connmark writing

Daniel Xu (2):
  selftests/bpf: Add existing connection bpf_*_ct_lookup() test
  selftests/bpf: Add connmark read test

 .../testing/selftests/bpf/prog_tests/bpf_nf.c | 60 +++++++++++++++++++
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 21 +++++++
 2 files changed, 81 insertions(+)

-- 
2.37.1


^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH bpf-next v2 1/2] selftests/bpf: Add existing connection bpf_*_ct_lookup() test
  2022-08-09 16:34 [PATCH bpf-next v2 0/2] Add more bpf_*_ct_lookup() selftests Daniel Xu
@ 2022-08-09 16:34 ` Daniel Xu
  2022-08-09 16:34 ` [PATCH bpf-next v2 2/2] selftests/bpf: Add connmark read test Daniel Xu
  1 sibling, 0 replies; 5+ messages in thread
From: Daniel Xu @ 2022-08-09 16:34 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii, memxor; +Cc: Daniel Xu, linux-kernel

Add a test where we do a conntrack lookup on an existing connection.
This is nice because it's a more realistic test than artifically
creating a ct entry and looking it up afterwards.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 .../testing/selftests/bpf/prog_tests/bpf_nf.c | 59 +++++++++++++++++++
 .../testing/selftests/bpf/progs/test_bpf_nf.c | 18 ++++++
 2 files changed, 77 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
index 7a74a1579076..317978cac029 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
@@ -24,10 +24,34 @@ enum {
 	TEST_TC_BPF,
 };
 
+#define TIMEOUT_MS 3000
+
+static int connect_to_server(int srv_fd)
+{
+	int fd = -1;
+
+	fd = socket(AF_INET, SOCK_STREAM, 0);
+	if (!ASSERT_GE(fd, 0, "socket"))
+		goto out;
+
+	if (CHECK_FAIL(connect_fd_to_fd(fd, srv_fd, TIMEOUT_MS))) {
+		close(fd);
+		fd = -1;
+	}
+out:
+	return fd;
+}
+
 static void test_bpf_nf_ct(int mode)
 {
+	const char *iptables = "iptables -t raw %s PREROUTING -j CT";
+	int srv_fd = -1, client_fd = -1, srv_client_fd = -1;
+	struct sockaddr_in peer_addr = {};
 	struct test_bpf_nf *skel;
 	int prog_fd, err;
+	socklen_t len;
+	u16 srv_port;
+	char cmd[64];
 	LIBBPF_OPTS(bpf_test_run_opts, topts,
 		.data_in = &pkt_v4,
 		.data_size_in = sizeof(pkt_v4),
@@ -38,6 +62,32 @@ static void test_bpf_nf_ct(int mode)
 	if (!ASSERT_OK_PTR(skel, "test_bpf_nf__open_and_load"))
 		return;
 
+	/* Enable connection tracking */
+	snprintf(cmd, sizeof(cmd), iptables, "-A");
+	if (!ASSERT_OK(system(cmd), "iptables"))
+		goto end;
+
+	srv_port = (mode == TEST_XDP) ? 5005 : 5006;
+	srv_fd = start_server(AF_INET, SOCK_STREAM, "127.0.0.1", srv_port, TIMEOUT_MS);
+	if (!ASSERT_GE(srv_fd, 0, "start_server"))
+		goto end;
+
+	client_fd = connect_to_server(srv_fd);
+	if (!ASSERT_GE(client_fd, 0, "connect_to_server"))
+		goto end;
+
+	len = sizeof(peer_addr);
+	srv_client_fd = accept(srv_fd, (struct sockaddr *)&peer_addr, &len);
+	if (!ASSERT_GE(srv_client_fd, 0, "accept"))
+		goto end;
+	if (!ASSERT_EQ(len, sizeof(struct sockaddr_in), "sockaddr len"))
+		goto end;
+
+	skel->bss->saddr = peer_addr.sin_addr.s_addr;
+	skel->bss->sport = peer_addr.sin_port;
+	skel->bss->daddr = peer_addr.sin_addr.s_addr;
+	skel->bss->dport = htons(srv_port);
+
 	if (mode == TEST_XDP)
 		prog_fd = bpf_program__fd(skel->progs.nf_xdp_ct_test);
 	else
@@ -63,7 +113,16 @@ static void test_bpf_nf_ct(int mode)
 	ASSERT_LE(skel->bss->test_delta_timeout, 10, "Test for max ct timeout update");
 	/* expected status is IPS_SEEN_REPLY */
 	ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
+	ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
 end:
+	if (srv_client_fd != -1)
+		close(srv_client_fd);
+	if (client_fd != -1)
+		close(client_fd);
+	if (srv_fd != -1)
+		close(srv_fd);
+	snprintf(cmd, sizeof(cmd), iptables, "-D");
+	system(cmd);
 	test_bpf_nf__destroy(skel);
 }
 
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
index 196cd8dfe42a..84e0fd479794 100644
--- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
+++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
@@ -23,6 +23,11 @@ int test_insert_entry = -EAFNOSUPPORT;
 int test_succ_lookup = -ENOENT;
 u32 test_delta_timeout = 0;
 u32 test_status = 0;
+__be32 saddr = 0;
+__be16 sport = 0;
+__be32 daddr = 0;
+__be16 dport = 0;
+int test_exist_lookup = -ENOENT;
 
 struct nf_conn;
 
@@ -160,6 +165,19 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
 		}
 		test_alloc_entry = 0;
 	}
+
+	bpf_tuple.ipv4.saddr = saddr;
+	bpf_tuple.ipv4.daddr = daddr;
+	bpf_tuple.ipv4.sport = sport;
+	bpf_tuple.ipv4.dport = dport;
+	ct = lookup_fn(ctx, &bpf_tuple, sizeof(bpf_tuple.ipv4), &opts_def,
+		       sizeof(opts_def));
+	if (ct) {
+		test_exist_lookup = 0;
+		bpf_ct_release(ct);
+	} else {
+		test_exist_lookup = opts_def.error;
+	}
 }
 
 SEC("xdp")
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH bpf-next v2 2/2] selftests/bpf: Add connmark read test
  2022-08-09 16:34 [PATCH bpf-next v2 0/2] Add more bpf_*_ct_lookup() selftests Daniel Xu
  2022-08-09 16:34 ` [PATCH bpf-next v2 1/2] selftests/bpf: Add existing connection bpf_*_ct_lookup() test Daniel Xu
@ 2022-08-09 16:34 ` Daniel Xu
  2022-08-09 21:14   ` Daniel Borkmann
  1 sibling, 1 reply; 5+ messages in thread
From: Daniel Xu @ 2022-08-09 16:34 UTC (permalink / raw)
  To: bpf, ast, daniel, andrii, memxor; +Cc: Daniel Xu, linux-kernel

Test that the prog can read from the connection mark. This test is nice
because it ensures progs can interact with netfilter subsystem
correctly.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tools/testing/selftests/bpf/prog_tests/bpf_nf.c | 3 ++-
 tools/testing/selftests/bpf/progs/test_bpf_nf.c | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
index 317978cac029..7232f6dcd252 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
@@ -44,7 +44,7 @@ static int connect_to_server(int srv_fd)
 
 static void test_bpf_nf_ct(int mode)
 {
-	const char *iptables = "iptables -t raw %s PREROUTING -j CT";
+	const char *iptables = "iptables -t raw %s PREROUTING -j CONNMARK --set-mark 42/0";
 	int srv_fd = -1, client_fd = -1, srv_client_fd = -1;
 	struct sockaddr_in peer_addr = {};
 	struct test_bpf_nf *skel;
@@ -114,6 +114,7 @@ static void test_bpf_nf_ct(int mode)
 	/* expected status is IPS_SEEN_REPLY */
 	ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
 	ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
+	ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
 end:
 	if (srv_client_fd != -1)
 		close(srv_client_fd);
diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
index 84e0fd479794..2722441850cc 100644
--- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
+++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
@@ -28,6 +28,7 @@ __be16 sport = 0;
 __be32 daddr = 0;
 __be16 dport = 0;
 int test_exist_lookup = -ENOENT;
+u32 test_exist_lookup_mark = 0;
 
 struct nf_conn;
 
@@ -174,6 +175,8 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
 		       sizeof(opts_def));
 	if (ct) {
 		test_exist_lookup = 0;
+		if (ct->mark == 42)
+			test_exist_lookup_mark = 43;
 		bpf_ct_release(ct);
 	} else {
 		test_exist_lookup = opts_def.error;
-- 
2.37.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add connmark read test
  2022-08-09 16:34 ` [PATCH bpf-next v2 2/2] selftests/bpf: Add connmark read test Daniel Xu
@ 2022-08-09 21:14   ` Daniel Borkmann
  2022-08-10  0:20     ` Daniel Xu
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Borkmann @ 2022-08-09 21:14 UTC (permalink / raw)
  To: Daniel Xu, bpf, ast, andrii, memxor; +Cc: linux-kernel

Hi Daniel,

On 8/9/22 6:34 PM, Daniel Xu wrote:
> Test that the prog can read from the connection mark. This test is nice
> because it ensures progs can interact with netfilter subsystem
> correctly.
> 
> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> ---
>   tools/testing/selftests/bpf/prog_tests/bpf_nf.c | 3 ++-
>   tools/testing/selftests/bpf/progs/test_bpf_nf.c | 3 +++
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> index 317978cac029..7232f6dcd252 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
> @@ -44,7 +44,7 @@ static int connect_to_server(int srv_fd)
>   
>   static void test_bpf_nf_ct(int mode)
>   {
> -	const char *iptables = "iptables -t raw %s PREROUTING -j CT";
> +	const char *iptables = "iptables -t raw %s PREROUTING -j CONNMARK --set-mark 42/0";
>   	int srv_fd = -1, client_fd = -1, srv_client_fd = -1;
>   	struct sockaddr_in peer_addr = {};
>   	struct test_bpf_nf *skel;
> @@ -114,6 +114,7 @@ static void test_bpf_nf_ct(int mode)
>   	/* expected status is IPS_SEEN_REPLY */
>   	ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
>   	ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
> +	ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
>   end:
>   	if (srv_client_fd != -1)
>   		close(srv_client_fd);
> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> index 84e0fd479794..2722441850cc 100644
> --- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
> @@ -28,6 +28,7 @@ __be16 sport = 0;
>   __be32 daddr = 0;
>   __be16 dport = 0;
>   int test_exist_lookup = -ENOENT;
> +u32 test_exist_lookup_mark = 0;
>   
>   struct nf_conn;
>   
> @@ -174,6 +175,8 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
>   		       sizeof(opts_def));
>   	if (ct) {
>   		test_exist_lookup = 0;
> +		if (ct->mark == 42)
> +			test_exist_lookup_mark = 43;

Looks like CI failed here:

   [...]
   progs/test_bpf_nf.c:178:11: error: no member named 'mark' in 'struct nf_conn'
                   if (ct->mark == 42)
                       ~~  ^
   1 error generated.
   make: *** [Makefile:521: /tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf/test_bpf_nf.o] Error 1
   make: *** Waiting for unfinished jobs....
   Error: Process completed with exit code 2.

Likely due to missing CONFIG_NF_CONNTRACK_MARK for the CI instance.

>   		bpf_ct_release(ct);
>   	} else {
>   		test_exist_lookup = opts_def.error;
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next v2 2/2] selftests/bpf: Add connmark read test
  2022-08-09 21:14   ` Daniel Borkmann
@ 2022-08-10  0:20     ` Daniel Xu
  0 siblings, 0 replies; 5+ messages in thread
From: Daniel Xu @ 2022-08-10  0:20 UTC (permalink / raw)
  To: Daniel Borkmann, bpf, Alexei Starovoitov, Andrii Nakryiko,
	Kumar Kartikeya Dwivedi
  Cc: linux-kernel

Hi Daniel,

On Tue, Aug 9, 2022, at 3:14 PM, Daniel Borkmann wrote:
> Hi Daniel,
>
> On 8/9/22 6:34 PM, Daniel Xu wrote:
>> Test that the prog can read from the connection mark. This test is nice
>> because it ensures progs can interact with netfilter subsystem
>> correctly.
>> 
>> Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
>> ---
>>   tools/testing/selftests/bpf/prog_tests/bpf_nf.c | 3 ++-
>>   tools/testing/selftests/bpf/progs/test_bpf_nf.c | 3 +++
>>   2 files changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
>> index 317978cac029..7232f6dcd252 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_nf.c
>> @@ -44,7 +44,7 @@ static int connect_to_server(int srv_fd)
>>   
>>   static void test_bpf_nf_ct(int mode)
>>   {
>> -	const char *iptables = "iptables -t raw %s PREROUTING -j CT";
>> +	const char *iptables = "iptables -t raw %s PREROUTING -j CONNMARK --set-mark 42/0";
>>   	int srv_fd = -1, client_fd = -1, srv_client_fd = -1;
>>   	struct sockaddr_in peer_addr = {};
>>   	struct test_bpf_nf *skel;
>> @@ -114,6 +114,7 @@ static void test_bpf_nf_ct(int mode)
>>   	/* expected status is IPS_SEEN_REPLY */
>>   	ASSERT_EQ(skel->bss->test_status, 2, "Test for ct status update ");
>>   	ASSERT_EQ(skel->data->test_exist_lookup, 0, "Test existing connection lookup");
>> +	ASSERT_EQ(skel->bss->test_exist_lookup_mark, 43, "Test existing connection lookup ctmark");
>>   end:
>>   	if (srv_client_fd != -1)
>>   		close(srv_client_fd);
>> diff --git a/tools/testing/selftests/bpf/progs/test_bpf_nf.c b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
>> index 84e0fd479794..2722441850cc 100644
>> --- a/tools/testing/selftests/bpf/progs/test_bpf_nf.c
>> +++ b/tools/testing/selftests/bpf/progs/test_bpf_nf.c
>> @@ -28,6 +28,7 @@ __be16 sport = 0;
>>   __be32 daddr = 0;
>>   __be16 dport = 0;
>>   int test_exist_lookup = -ENOENT;
>> +u32 test_exist_lookup_mark = 0;
>>   
>>   struct nf_conn;
>>   
>> @@ -174,6 +175,8 @@ nf_ct_test(struct nf_conn *(*lookup_fn)(void *, struct bpf_sock_tuple *, u32,
>>   		       sizeof(opts_def));
>>   	if (ct) {
>>   		test_exist_lookup = 0;
>> +		if (ct->mark == 42)
>> +			test_exist_lookup_mark = 43;
>
> Looks like CI failed here:
>
>    [...]
>    progs/test_bpf_nf.c:178:11: error: no member named 'mark' in 'struct 
> nf_conn'
>                    if (ct->mark == 42)
>                        ~~  ^
>    1 error generated.
>    make: *** [Makefile:521: 
> /tmp/runner/work/bpf/bpf/tools/testing/selftests/bpf/test_bpf_nf.o] 
> Error 1
>    make: *** Waiting for unfinished jobs....
>    Error: Process completed with exit code 2.
>
> Likely due to missing CONFIG_NF_CONNTRACK_MARK for the CI instance.

Originally (as stated in the cover letter) I thought the CI kconfig was hosted
somewhere else. Looking closer I see the kconfigs are checked into the
selftest tree.

I think the following should fix the CI. I'll send out a v3 tomorrow morning:

diff --git a/tools/testing/selftests/bpf/config b/tools/testing/selftests/bpf/config
index fabf0c014349..3fc46f9cfb22 100644
--- a/tools/testing/selftests/bpf/config
+++ b/tools/testing/selftests/bpf/config
@@ -50,9 +50,11 @@ CONFIG_NET_SCHED=y
 CONFIG_NETDEVSIM=m
 CONFIG_NETFILTER=y
 CONFIG_NETFILTER_SYNPROXY=y
+CONFIG_NETFILTER_XT_CONNMARK=y
 CONFIG_NETFILTER_XT_MATCH_STATE=y
 CONFIG_NETFILTER_XT_TARGET_CT=y
 CONFIG_NF_CONNTRACK=y
+CONFIG_NF_CONNTRACK_MARK=y
 CONFIG_NF_DEFRAG_IPV4=y
 CONFIG_NF_DEFRAG_IPV6=y
 CONFIG_RC_CORE=y

[...]

Thanks,
Daniel

^ permalink raw reply related	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-10  0:20 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-09 16:34 [PATCH bpf-next v2 0/2] Add more bpf_*_ct_lookup() selftests Daniel Xu
2022-08-09 16:34 ` [PATCH bpf-next v2 1/2] selftests/bpf: Add existing connection bpf_*_ct_lookup() test Daniel Xu
2022-08-09 16:34 ` [PATCH bpf-next v2 2/2] selftests/bpf: Add connmark read test Daniel Xu
2022-08-09 21:14   ` Daniel Borkmann
2022-08-10  0:20     ` Daniel Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.