[PATCH bpf-next v3 0/3] Add more bpf_*_ct_lookup() selftests

[PATCH bpf-next v3 0/3] Add more bpf_*_ct_lookup() selftests

[Daniel Xu]

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`.

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

Changes since v2:
- Add bpf-ci kconfig changes

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


Daniel Xu (3):
  selftests/bpf: Add existing connection bpf_*_ct_lookup() test
  selftests/bpf: Add connmark read test
  selftests/bpf: Update CI kconfig

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

-- 
2.37.1

[PATCH bpf-next v3 1/3] selftests/bpf: Add existing connection bpf_*_ct_lookup() test

[Daniel Xu]

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

[PATCH bpf-next v3 2/3] selftests/bpf: Add connmark read test

[Daniel Xu]

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

[PATCH bpf-next v3 3/3] selftests/bpf: Update CI kconfig

[Daniel Xu]

The previous selftest changes require two kconfig changes in bpf-ci.

Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
---
 tools/testing/selftests/bpf/config | 2 ++
 1 file changed, 2 insertions(+)

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
-- 
2.37.1

Re: [PATCH bpf-next v3 1/3] selftests/bpf: Add existing connection bpf_*_ct_lookup() test

[Kumar Kartikeya Dwivedi]

On Thu, 11 Aug 2022 at 01:16, Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> 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))) {

CHECK_FAIL is deprecated, please consider using ASSERT_*. Sorry for
not catching this before your respin...

> +               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
>

Re: [PATCH bpf-next v3 0/3] Add more bpf_*_ct_lookup() selftests

[Kumar Kartikeya Dwivedi]

On Thu, 11 Aug 2022 at 01:16, Daniel Xu <dxu@dxuuu.xyz> wrote:
>
> 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.
>

Thank you for contributing these tests. Feel free to add:
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

People often look at selftests for usage examples these days, so it's
great to have coverage + examples for more use cases.

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

Please also cc netfilter-devel, netdev, Pablo, and Florian when you send it.

I think we can directly enable stores to ct->mark, since that is what
ctnetlink is doing too, so adding another helper for this would be
unnecessary overhead.


> Past discussion:
> - v2: https://lore.kernel.org/bpf/cover.1660062725.git.dxu@dxuuu.xyz/
> - v1: https://lore.kernel.org/bpf/cover.1659209738.git.dxu@dxuuu.xyz/
>
> Changes since v2:
> - Add bpf-ci kconfig changes
>
> Changes since v1:
> - Reword commit message / cover letter to not mention connmark writing
>
>
> Daniel Xu (3):
>   selftests/bpf: Add existing connection bpf_*_ct_lookup() test
>   selftests/bpf: Add connmark read test
>   selftests/bpf: Update CI kconfig
>
>  tools/testing/selftests/bpf/config            |  2 +
>  .../testing/selftests/bpf/prog_tests/bpf_nf.c | 60 +++++++++++++++++++
>  .../testing/selftests/bpf/progs/test_bpf_nf.c | 21 +++++++
>  3 files changed, 83 insertions(+)
>
> --
> 2.37.1
>

Re: [PATCH bpf-next v3 0/3] Add more bpf_*_ct_lookup() selftests

[Daniel Xu]

Hi Kumar,

On Wed, Aug 10, 2022, at 6:25 PM, Kumar Kartikeya Dwivedi wrote:
> On Thu, 11 Aug 2022 at 01:16, Daniel Xu <dxu@dxuuu.xyz> wrote:
>>
>> 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.
>>
>
> Thank you for contributing these tests. Feel free to add:
> Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
>
> People often look at selftests for usage examples these days, so it's
> great to have coverage + examples for more use cases.

I also want this interaction to still work when I start using it later :).

>
>> I'll follow this patchset up with support for writing to `struct nf_conn`.
>>
>
> Please also cc netfilter-devel, netdev, Pablo, and Florian when you send it.
>

Ack.

> I think we can directly enable stores to ct->mark, since that is what
> ctnetlink is doing too, so adding another helper for this would be
> unnecessary overhead.

Ack.

[...]

Thanks,
Danel