| Message ID | abd424ee71675e3008acd4a2c1fd136cb7dbf8be.1659209738.git.dxu@dxuuu.xyz (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Add more bpf_*_ct_lookup() selftests | expand |
On Sat, 30 Jul 2022 at 21:40, Daniel Xu <dxu@dxuuu.xyz> wrote: > > Test that the prog can read/write to/from the connection mark. This > test is nice because it ensures progs can interact with netfilter > subsystem correctly. > Commit message is a bit misleading, where are you writing to ct->mark? :) The cover letter also mentions "reading and writing from nf_conn". Do you have patches whitelisting nf_conn::mark for writes? If not, drop the writing related bits from the commit log. The rest looks ok to me. > 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 >
Hi Kumar, On Mon, Aug 1, 2022, at 8:51 AM, Kumar Kartikeya Dwivedi wrote: > On Sat, 30 Jul 2022 at 21:40, Daniel Xu <dxu@dxuuu.xyz> wrote: >> >> Test that the prog can read/write to/from the connection mark. This >> test is nice because it ensures progs can interact with netfilter >> subsystem correctly. >> > > Commit message is a bit misleading, where are you writing to ct->mark? :) > The cover letter also mentions "reading and writing from nf_conn". Do > you have patches whitelisting nf_conn::mark for writes? > If not, drop the writing related bits from the commit log. The rest > looks ok to me. Ah good catch, thanks. I've neglected to actually test writing to the mark. I'll follow up with another patch to enable writing to mark and testing it. And in meantime let's just change the commit msg on this patchset. I'm in the middle of a move right now so I probably can't respin the patch for a few more days. Feel free to edit the commit msgs. Or I'll send it again when I set my computer up. [...] Thanks, Daniel
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;
Test that the prog can read/write to/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(-)