[PATCH bpf-next] selftests/bpf: fix test_dev_cgroup

[PATCH bpf-next] selftests/bpf: fix test_dev_cgroup

[Alexei Starovoitov]

The test incorrectly doing
mkdir /mnt/cgroup-test-work-dirtest-bpf-based-device-cgroup
instead of
mkdir /mnt/cgroup-test-work-dir/test-bpf-based-device-cgroup

somehow such mkdir succeeds and new directory appears:
/mnt/cgroup-test-work-dir/cgroup-test-work-dirtest-bpf-based-device-cgroup

Later cleanup via nftw("/mnt/cgroup-test-work-dir", ...);
doesn't walk this directory.
"rmdir /mnt/cgroup-test-work-dir" succeeds, but bpf program and
dangling cgroup stays in memory.
That's a separate issue on a cgroup side.
For now fix the test.

Fixes: 37f1ba0909df ("selftests/bpf: add a test for device cgroup controller")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 tools/testing/selftests/bpf/test_dev_cgroup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/test_dev_cgroup.c b/tools/testing/selftests/bpf/test_dev_cgroup.c
index c1535b34f14f..3489cc283433 100644
--- a/tools/testing/selftests/bpf/test_dev_cgroup.c
+++ b/tools/testing/selftests/bpf/test_dev_cgroup.c
@@ -21,7 +21,7 @@
 
 #define DEV_CGROUP_PROG "./dev_cgroup.o"
 
-#define TEST_CGROUP "test-bpf-based-device-cgroup/"
+#define TEST_CGROUP "/test-bpf-based-device-cgroup/"
 
 int main(int argc, char **argv)
 {
-- 
2.9.5

Re: [PATCH bpf-next] selftests/bpf: fix test_dev_cgroup

[Daniel Borkmann]

On 01/23/2018 05:48 AM, Alexei Starovoitov wrote:
> The test incorrectly doing
> mkdir /mnt/cgroup-test-work-dirtest-bpf-based-device-cgroup
> instead of
> mkdir /mnt/cgroup-test-work-dir/test-bpf-based-device-cgroup
> 
> somehow such mkdir succeeds and new directory appears:
> /mnt/cgroup-test-work-dir/cgroup-test-work-dirtest-bpf-based-device-cgroup
> 
> Later cleanup via nftw("/mnt/cgroup-test-work-dir", ...);
> doesn't walk this directory.
> "rmdir /mnt/cgroup-test-work-dir" succeeds, but bpf program and
> dangling cgroup stays in memory.
> That's a separate issue on a cgroup side.

Purely cgroup side with regards to internal cleanup (which then as a
side effect doesn't drop ref on the BPF prog)? Is a fix taken care of?

> For now fix the test.
> 
> Fixes: 37f1ba0909df ("selftests/bpf: add a test for device cgroup controller")
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>

Applied to bpf-next, thanks Alexei!

Re: [PATCH bpf-next] selftests/bpf: fix test_dev_cgroup

[Alexei Starovoitov]

On Tue, Jan 23, 2018 at 06:50:04PM +0100, Daniel Borkmann wrote:
> On 01/23/2018 05:48 AM, Alexei Starovoitov wrote:
> > The test incorrectly doing
> > mkdir /mnt/cgroup-test-work-dirtest-bpf-based-device-cgroup
> > instead of
> > mkdir /mnt/cgroup-test-work-dir/test-bpf-based-device-cgroup
> > 
> > somehow such mkdir succeeds and new directory appears:
> > /mnt/cgroup-test-work-dir/cgroup-test-work-dirtest-bpf-based-device-cgroup
> > 
> > Later cleanup via nftw("/mnt/cgroup-test-work-dir", ...);
> > doesn't walk this directory.
> > "rmdir /mnt/cgroup-test-work-dir" succeeds, but bpf program and
> > dangling cgroup stays in memory.
> > That's a separate issue on a cgroup side.
> 
> Purely cgroup side with regards to internal cleanup (which then as a
> side effect doesn't drop ref on the BPF prog)? Is a fix taken care of?

we're arguing whether it's a bug in the first place.
When cgroup_helpers.c were written my understanding was that
the sequence:
unshare(CLONE_NEWNS);
mount("none", CGROUP_MOUNT_PATH, "cgroup2", 0, NULL)
will give us fresh cgroup2 mount where the tests can run without affecting
other cgroup2s.
Turned out that cgroup2 hierarchy is still global here.
When newns is destroyed the cgroup2 fs gets unmounted, but internal
hierarchy stays around in arguably hidden form (though cgroup2 is no longer
mounted anywhere).

The next time we execute test_dev_cgroup it picks up the same tmp test
directory. New bpf program is loaded and attached to old cgroup dir.
That attach operation overrides old program and now new program
becomes hidden when newns is destroyed second time.

I thought the following diff:
-       if (unshare(CLONE_NEWNS)) {
+       if (unshare(CLONE_NEWNS | CLONE_NEWCGROUP)) {
should make cgroup behave like fresh hierarchy, but it didn't work either.