* [PATCH bpf-next] selftests/bpf: Restore the netns after flow dissector reattach test
@ 2019-10-17 8:37 Jakub Sitnicki
2019-10-17 18:18 ` Martin Lau
0 siblings, 1 reply; 4+ messages in thread
From: Jakub Sitnicki @ 2019-10-17 8:37 UTC (permalink / raw)
To: bpf; +Cc: netdev, kernel-team, Alexei Starovoitov, Andrii Nakryiko
flow_dissector_reattach test changes the netns we run in but does not
restore it to the one we started in when finished. This interferes with
tests that run after it. Fix it by restoring the netns when done.
Fixes: f97eea1756f3 ("selftests/bpf: Check that flow dissector can be re-attached")
Reported-by: Alexei Starovoitov <ast@kernel.org>
Reported-by: Andrii Nakryiko <andriin@fb.com>
Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
---
.../bpf/prog_tests/flow_dissector_reattach.c | 21 +++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
index 777faffc4639..1f51ba66b98b 100644
--- a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
+++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
@@ -91,12 +91,18 @@ static void do_flow_dissector_reattach(void)
void test_flow_dissector_reattach(void)
{
- int init_net, err;
+ int init_net, self_net, err;
+
+ self_net = open("/proc/self/ns/net", O_RDONLY);
+ if (CHECK_FAIL(self_net < 0)) {
+ perror("open(/proc/self/ns/net");
+ return;
+ }
init_net = open("/proc/1/ns/net", O_RDONLY);
if (CHECK_FAIL(init_net < 0)) {
perror("open(/proc/1/ns/net)");
- return;
+ goto out_close;
}
err = setns(init_net, CLONE_NEWNET);
@@ -108,7 +114,7 @@ void test_flow_dissector_reattach(void)
if (is_attached(init_net)) {
test__skip();
printf("Can't test with flow dissector attached to init_net\n");
- return;
+ goto out_setns;
}
/* First run tests in root network namespace */
@@ -118,10 +124,17 @@ void test_flow_dissector_reattach(void)
err = unshare(CLONE_NEWNET);
if (CHECK_FAIL(err)) {
perror("unshare(CLONE_NEWNET)");
- goto out_close;
+ goto out_setns;
}
do_flow_dissector_reattach();
+out_setns:
+ /* Move back to netns we started in. */
+ err = setns(self_net, CLONE_NEWNET);
+ if (CHECK_FAIL(err))
+ perror("setns(/proc/self/ns/net)");
+
out_close:
close(init_net);
+ close(self_net);
}
--
2.20.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Restore the netns after flow dissector reattach test
2019-10-17 8:37 [PATCH bpf-next] selftests/bpf: Restore the netns after flow dissector reattach test Jakub Sitnicki
@ 2019-10-17 18:18 ` Martin Lau
2019-10-17 19:13 ` Alexei Starovoitov
2019-10-18 9:07 ` Jakub Sitnicki
0 siblings, 2 replies; 4+ messages in thread
From: Martin Lau @ 2019-10-17 18:18 UTC (permalink / raw)
To: Jakub Sitnicki
Cc: bpf, netdev, kernel-team, Alexei Starovoitov, Andrii Nakryiko
On Thu, Oct 17, 2019 at 10:37:52AM +0200, Jakub Sitnicki wrote:
> flow_dissector_reattach test changes the netns we run in but does not
> restore it to the one we started in when finished. This interferes with
> tests that run after it. Fix it by restoring the netns when done.
>
> Fixes: f97eea1756f3 ("selftests/bpf: Check that flow dissector can be re-attached")
> Reported-by: Alexei Starovoitov <ast@kernel.org>
> Reported-by: Andrii Nakryiko <andriin@fb.com>
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
> .../bpf/prog_tests/flow_dissector_reattach.c | 21 +++++++++++++++----
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> index 777faffc4639..1f51ba66b98b 100644
> --- a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> +++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> @@ -91,12 +91,18 @@ static void do_flow_dissector_reattach(void)
>
> void test_flow_dissector_reattach(void)
> {
> - int init_net, err;
> + int init_net, self_net, err;
> +
> + self_net = open("/proc/self/ns/net", O_RDONLY);
> + if (CHECK_FAIL(self_net < 0)) {
> + perror("open(/proc/self/ns/net");
> + return;
> + }
>
> init_net = open("/proc/1/ns/net", O_RDONLY);
> if (CHECK_FAIL(init_net < 0)) {
> perror("open(/proc/1/ns/net)");
> - return;
> + goto out_close;
Mostly nit. close(-1) is ok-ish... The same goes for the "out_close" in
do_flow_dissector_reattach().
Acked-by: Martin KaFai Lau <kafai@fb.com>
> }
>
> err = setns(init_net, CLONE_NEWNET);
> @@ -108,7 +114,7 @@ void test_flow_dissector_reattach(void)
> if (is_attached(init_net)) {
> test__skip();
> printf("Can't test with flow dissector attached to init_net\n");
> - return;
> + goto out_setns;
> }
>
> /* First run tests in root network namespace */
> @@ -118,10 +124,17 @@ void test_flow_dissector_reattach(void)
> err = unshare(CLONE_NEWNET);
> if (CHECK_FAIL(err)) {
> perror("unshare(CLONE_NEWNET)");
> - goto out_close;
> + goto out_setns;
> }
> do_flow_dissector_reattach();
>
> +out_setns:
> + /* Move back to netns we started in. */
> + err = setns(self_net, CLONE_NEWNET);
> + if (CHECK_FAIL(err))
> + perror("setns(/proc/self/ns/net)");
> +
> out_close:
> close(init_net);
> + close(self_net);
> }
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Restore the netns after flow dissector reattach test
2019-10-17 18:18 ` Martin Lau
@ 2019-10-17 19:13 ` Alexei Starovoitov
2019-10-18 9:07 ` Jakub Sitnicki
1 sibling, 0 replies; 4+ messages in thread
From: Alexei Starovoitov @ 2019-10-17 19:13 UTC (permalink / raw)
To: Martin Lau
Cc: Jakub Sitnicki, bpf, netdev, kernel-team, Alexei Starovoitov,
Andrii Nakryiko
On Thu, Oct 17, 2019 at 11:18 AM Martin Lau <kafai@fb.com> wrote:
>
> On Thu, Oct 17, 2019 at 10:37:52AM +0200, Jakub Sitnicki wrote:
> > flow_dissector_reattach test changes the netns we run in but does not
> > restore it to the one we started in when finished. This interferes with
> > tests that run after it. Fix it by restoring the netns when done.
> >
> > Fixes: f97eea1756f3 ("selftests/bpf: Check that flow dissector can be re-attached")
> > Reported-by: Alexei Starovoitov <ast@kernel.org>
> > Reported-by: Andrii Nakryiko <andriin@fb.com>
> > Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> > ---
> > .../bpf/prog_tests/flow_dissector_reattach.c | 21 +++++++++++++++----
> > 1 file changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> > index 777faffc4639..1f51ba66b98b 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
> > @@ -91,12 +91,18 @@ static void do_flow_dissector_reattach(void)
> >
> > void test_flow_dissector_reattach(void)
> > {
> > - int init_net, err;
> > + int init_net, self_net, err;
> > +
> > + self_net = open("/proc/self/ns/net", O_RDONLY);
> > + if (CHECK_FAIL(self_net < 0)) {
> > + perror("open(/proc/self/ns/net");
> > + return;
> > + }
> >
> > init_net = open("/proc/1/ns/net", O_RDONLY);
> > if (CHECK_FAIL(init_net < 0)) {
> > perror("open(/proc/1/ns/net)");
> > - return;
> > + goto out_close;
> Mostly nit. close(-1) is ok-ish... The same goes for the "out_close" in
> do_flow_dissector_reattach().
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
Yeah. Not ideal, but applied the fix to bpf-next
to get selftests working again.
Please follow up with any cleanups.
Thanks
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH bpf-next] selftests/bpf: Restore the netns after flow dissector reattach test
2019-10-17 18:18 ` Martin Lau
2019-10-17 19:13 ` Alexei Starovoitov
@ 2019-10-18 9:07 ` Jakub Sitnicki
1 sibling, 0 replies; 4+ messages in thread
From: Jakub Sitnicki @ 2019-10-18 9:07 UTC (permalink / raw)
To: Martin Lau; +Cc: bpf, netdev, kernel-team, Alexei Starovoitov, Andrii Nakryiko
On Thu, Oct 17, 2019 at 08:18 PM CEST, Martin Lau wrote:
> On Thu, Oct 17, 2019 at 10:37:52AM +0200, Jakub Sitnicki wrote:
[...]
>> ---
>> .../bpf/prog_tests/flow_dissector_reattach.c | 21 +++++++++++++++----
>> 1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
>> index 777faffc4639..1f51ba66b98b 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/flow_dissector_reattach.c
>> @@ -91,12 +91,18 @@ static void do_flow_dissector_reattach(void)
>>
>> void test_flow_dissector_reattach(void)
>> {
>> - int init_net, err;
>> + int init_net, self_net, err;
>> +
>> + self_net = open("/proc/self/ns/net", O_RDONLY);
>> + if (CHECK_FAIL(self_net < 0)) {
>> + perror("open(/proc/self/ns/net");
>> + return;
>> + }
>>
>> init_net = open("/proc/1/ns/net", O_RDONLY);
>> if (CHECK_FAIL(init_net < 0)) {
>> perror("open(/proc/1/ns/net)");
>> - return;
>> + goto out_close;
> Mostly nit. close(-1) is ok-ish... The same goes for the "out_close" in
> do_flow_dissector_reattach().
Happy to fix it up. Is your concern that calls to close invalid FDs
clutter strace output or something else?
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
Thanks for the review.
-Jakub
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-10-18 9:07 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-17 8:37 [PATCH bpf-next] selftests/bpf: Restore the netns after flow dissector reattach test Jakub Sitnicki
2019-10-17 18:18 ` Martin Lau
2019-10-17 19:13 ` Alexei Starovoitov
2019-10-18 9:07 ` Jakub Sitnicki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).