| Message ID | 20241113-flow_dissector-v1-7-27c4df0592dc@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/bpf: migrate test_flow_dissector.sh to test_progs | expand |
On 11/13/24 14:53, Alexis Lothoré (eBPF Foundation) wrote: [...] > + ns = open_netns(TEST_NS); > + bpf_prog_detach2(prog_fd, 0, BPF_FLOW_DISSECTOR); > + close_netns(ns); I would like to mention that I initially planned to directly delete the namespace to perform the test cleanup, assuming it would be enough to consider any non-root namespace flow_dissector to be removed. However I observed that it made other tests dealing with flow_dissector starting to fail with -EEXIST, despite all those tests being marked as "serial". I started examining this, suspecting a real issue (a race between namespace deletion and flow dissector attachment check, or a ns refcount issue) but before going further: is my assumption right ? Should a mere namespace deletion be indeed enough to remove the corresponding bpf flow dissector ? Or am I missing something ? If so I'll keep examining this. > +out_clean_ns: > + remove_netns(TEST_NS); > +out_destroy_skel: > + bpf_flow__destroy(skel); > +} > + > static int create_tap(const char *ifname) > { > struct ifreq ifr = { >
On 11/13, Alexis Lothoré wrote: > On 11/13/24 14:53, Alexis Lothoré (eBPF Foundation) wrote: > > [...] > > > + ns = open_netns(TEST_NS); > > + bpf_prog_detach2(prog_fd, 0, BPF_FLOW_DISSECTOR); > > + close_netns(ns); > > I would like to mention that I initially planned to directly delete the > namespace to perform the test cleanup, assuming it would be enough to consider > any non-root namespace flow_dissector to be removed. However I observed that it > made other tests dealing with flow_dissector starting to fail with -EEXIST, > despite all those tests being marked as "serial". I started examining this, > suspecting a real issue (a race between namespace deletion and flow dissector > attachment check, or a ns refcount issue) but before going further: is my > assumption right ? Should a mere namespace deletion be indeed enough to remove > the corresponding bpf flow dissector ? Or am I missing something ? If so I'll > keep examining this. Don't think that's expected. Removing a namespace with an attached dissector program should (in theory) clearly detach it.
On 11/13/24 18:50, Stanislav Fomichev wrote: > On 11/13, Alexis Lothoré wrote: >> On 11/13/24 14:53, Alexis Lothoré (eBPF Foundation) wrote: >> >> [...] >> >>> + ns = open_netns(TEST_NS); >>> + bpf_prog_detach2(prog_fd, 0, BPF_FLOW_DISSECTOR); >>> + close_netns(ns); >> >> I would like to mention that I initially planned to directly delete the >> namespace to perform the test cleanup, assuming it would be enough to consider >> any non-root namespace flow_dissector to be removed. However I observed that it >> made other tests dealing with flow_dissector starting to fail with -EEXIST, >> despite all those tests being marked as "serial". I started examining this, >> suspecting a real issue (a race between namespace deletion and flow dissector >> attachment check, or a ns refcount issue) but before going further: is my >> assumption right ? Should a mere namespace deletion be indeed enough to remove >> the corresponding bpf flow dissector ? Or am I missing something ? If so I'll >> keep examining this. > > Don't think that's expected. Removing a namespace with an attached > dissector program should (in theory) clearly detach it. ACK, thanks, I'll keep investigating this then.
diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c index 05f3953c6266efdedeb74a81969ccfdabf009ccd..216f89070144f5fd19d602d0691f0aa6eed10a8e 100644 --- a/tools/testing/selftests/bpf/prog_tests/flow_dissector.c +++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector.c @@ -7,6 +7,7 @@ #include "bpf_flow.skel.h" +#define TEST_NS "flow_dissector_ns" #define FLOW_CONTINUE_SADDR 0x7f00007f /* 127.0.0.127 */ #define TEST_NAME_MAX_LEN 64 @@ -495,6 +496,67 @@ struct test tests[] = { }, }; +void serial_test_flow_dissector_namespace(void) +{ + struct bpf_flow *skel; + struct nstoken *ns; + int err, prog_fd; + + skel = bpf_flow__open_and_load(); + if (!ASSERT_OK_PTR(skel, "open/load skeleton")) + return; + + prog_fd = bpf_program__fd(skel->progs._dissect); + if (!ASSERT_OK_FD(prog_fd, "get dissector fd")) + goto out_destroy_skel; + + /* We must be able to attach a flow dissector to root namespace */ + err = bpf_prog_attach(prog_fd, 0, BPF_FLOW_DISSECTOR, 0); + if (!ASSERT_OK(err, "attach on root namespace ok")) + goto out_destroy_skel; + + err = make_netns(TEST_NS); + if (!ASSERT_OK(err, "create non-root net namespace")) + goto out_destroy_skel; + + /* We must not be able to additionally attach a flow dissector to a + * non-root net namespace + */ + ns = open_netns(TEST_NS); + if (!ASSERT_OK_PTR(ns, "enter non-root net namespace")) + goto out_clean_ns; + + err = bpf_prog_attach(prog_fd, 0, BPF_FLOW_DISSECTOR, 0); + close_netns(ns); + ASSERT_ERR(err, "refuse new flow dissector in non-root net namespace"); + ASSERT_EQ(errno, EEXIST, "refused because of already attached prog"); + + /* If no flow dissector is attached to the root namespace, we must + * be able to attach one to a non-root net namespace + */ + bpf_prog_detach2(prog_fd, 0, BPF_FLOW_DISSECTOR); + ns = open_netns(TEST_NS); + ASSERT_OK_PTR(ns, "enter non-root net namespace"); + err = bpf_prog_attach(prog_fd, 0, BPF_FLOW_DISSECTOR, 0); + close_netns(ns); + ASSERT_OK(err, "accept new flow dissector in non-root net namespace"); + + /* If a flow dissector is attached to non-root net namespace, attaching + * a flow dissector to root namespace must fail + */ + err = bpf_prog_attach(prog_fd, 0, BPF_FLOW_DISSECTOR, 0); + ASSERT_ERR(err, "refuse new flow dissector on root namespace"); + ASSERT_EQ(errno, EEXIST, "refused because of already attached prog"); + + ns = open_netns(TEST_NS); + bpf_prog_detach2(prog_fd, 0, BPF_FLOW_DISSECTOR); + close_netns(ns); +out_clean_ns: + remove_netns(TEST_NS); +out_destroy_skel: + bpf_flow__destroy(skel); +} + static int create_tap(const char *ifname) { struct ifreq ifr = {
Commit a11c397c43d5 ("bpf/flow_dissector: add mode to enforce global BPF flow dissector") is currently tested in test_flow_dissector.sh, which is not part of test_progs. Add the corresponding test to flow_dissector.c, which is part of test_progs. The new test reproduces the behavior implemented in its shell script counterpart: - attach a flow dissector program to the root net namespace, ensure that we can not attach another flow dissector in any non-root net namespace - attach a flow dissector program to a non-root net namespace, ensure that we can not attach another flow dissector in root namespace Since the new test is performing operations in the root net namespace, make sure to set it as a "serial" test to make sure not to conflict with any other test. Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> --- .../selftests/bpf/prog_tests/flow_dissector.c | 62 ++++++++++++++++++++++ 1 file changed, 62 insertions(+)