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/[email protected]/
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
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 <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
> ---
> 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;
>
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 <[email protected]>
>> ---
>> 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