| Message ID | 20180123044840.3467394-1-ast@kernel.org |
|---|---|
| State | Accepted, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [bpf-next] selftests/bpf: fix test_dev_cgroup | expand |
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!
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.
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) {
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(-)