Hello, this small series aims to integrate test_dev_cgroup in test_progs so it could be run automatically in CI. The new version brings a few differences with the current one: - test now uses directly syscalls instead of wrapping commandline tools into system() calls - test_progs manipulates /dev/null (eg: redirecting test logs into it), so disabling access to it in the bpf program confuses the tests. To fix this, the first commit modifies the bpf program to allow access to char devices 1:3 (/dev/null), and disable access to char devices 1:5 (/dev/zero) - once test is converted, add a small subtest to also check for device type interpretation (char or block) - paths used in mknod tests are now in /dev instead of /tmp: due to the CI runner organisation and mountpoints manipulations, trying to create nodes in /tmp leads to errors unrelated to the test (ie, mknod calls refused by kernel, not the bpf program). I don't understand exactly the root cause at the deepest point (all I see in CI is an -ENXIO error on mknod when trying to create the node in tmp, and I can not make sense out of it neither replicate it locally), so I would gladly take inputs from anyone more educated than me about this.
The new test_progs part has been tested in a local qemu environment as well as in upstream CI:
./test_progs -a cgroup_dev 47/1 cgroup_dev/deny-mknod:OK 47/2 cgroup_dev/allow-mknod:OK 47/3 cgroup_dev/deny-mknod-wrong-type:OK 47/4 cgroup_dev/allow-read:OK 47/5 cgroup_dev/allow-write:OK 47/6 cgroup_dev/deny-read:OK 47/7 cgroup_dev/deny-write:OK 47 cgroup_dev:OK Summary: 1/7 PASSED, 0 SKIPPED, 0 FAILED
--- Changes in v2: - directly pass expected ret code to subtests instead of boolean pass/not pass - fix faulty fd check in subtest expected to fail on open - fix wrong subtest name - pass test buffer and corresponding size to read/write subtests - use correct series prefix - Link to v1: https://lore.kernel.org/r/20240725-convert_dev_cgroup-v1-0-2c8cbd487c44@boot...
--- Alexis Lothoré (eBPF Foundation) (3): selftests/bpf: do not disable /dev/null device access in cgroup dev test selftests/bpf: convert test_dev_cgroup to test_progs selftests/bpf: add wrong type test to cgroup dev
tools/testing/selftests/bpf/.gitignore | 1 - tools/testing/selftests/bpf/Makefile | 2 - .../testing/selftests/bpf/prog_tests/cgroup_dev.c | 113 +++++++++++++++++++++ tools/testing/selftests/bpf/progs/dev_cgroup.c | 4 +- tools/testing/selftests/bpf/test_dev_cgroup.c | 85 ---------------- 5 files changed, 115 insertions(+), 90 deletions(-) --- base-commit: 3e9fef7751a84a7d02bbe14a67d3a5d301cbd156 change-id: 20240723-convert_dev_cgroup-6464b0d37f1a
Best regards,
test_dev_cgroup currently loads a small bpf program allowing any access on urandom and zero devices, disabling access to any other device. It makes migrating this test to test_progs impossible, since this one manipulates extensively /dev/null.
Allow /dev/null manipulation in dev_cgroup program to make its usage in test_progs framework possible. Update test_dev_cgroup.c as well to match this change while it has not been removed.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com --- tools/testing/selftests/bpf/progs/dev_cgroup.c | 4 ++-- tools/testing/selftests/bpf/test_dev_cgroup.c | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/dev_cgroup.c b/tools/testing/selftests/bpf/progs/dev_cgroup.c index 79b54a4fa244..c1dfbd2b56fc 100644 --- a/tools/testing/selftests/bpf/progs/dev_cgroup.c +++ b/tools/testing/selftests/bpf/progs/dev_cgroup.c @@ -41,14 +41,14 @@ int bpf_prog1(struct bpf_cgroup_dev_ctx *ctx) bpf_trace_printk(fmt, sizeof(fmt), ctx->major, ctx->minor); #endif
- /* Allow access to /dev/zero and /dev/random. + /* Allow access to /dev/null and /dev/urandom. * Forbid everything else. */ if (ctx->major != 1 || type != BPF_DEVCG_DEV_CHAR) return 0;
switch (ctx->minor) { - case 5: /* 1:5 /dev/zero */ + case 3: /* 1:3 /dev/null */ case 9: /* 1:9 /dev/urandom */ return 1; } diff --git a/tools/testing/selftests/bpf/test_dev_cgroup.c b/tools/testing/selftests/bpf/test_dev_cgroup.c index adeaf63cb6fa..33f544f0005a 100644 --- a/tools/testing/selftests/bpf/test_dev_cgroup.c +++ b/tools/testing/selftests/bpf/test_dev_cgroup.c @@ -54,25 +54,25 @@ int main(int argc, char **argv) goto err; }
- /* All operations with /dev/zero and and /dev/urandom are allowed, + /* All operations with /dev/null and /dev/urandom are allowed, * everything else is forbidden. */ - assert(system("rm -f /tmp/test_dev_cgroup_null") == 0); - assert(system("mknod /tmp/test_dev_cgroup_null c 1 3")); - assert(system("rm -f /tmp/test_dev_cgroup_null") == 0); - - /* /dev/zero is whitelisted */ assert(system("rm -f /tmp/test_dev_cgroup_zero") == 0); - assert(system("mknod /tmp/test_dev_cgroup_zero c 1 5") == 0); + assert(system("mknod /tmp/test_dev_cgroup_zero c 1 5")); assert(system("rm -f /tmp/test_dev_cgroup_zero") == 0);
- assert(system("dd if=/dev/urandom of=/dev/zero count=64") == 0); + /* /dev/null is whitelisted */ + assert(system("rm -f /tmp/test_dev_cgroup_null") == 0); + assert(system("mknod /tmp/test_dev_cgroup_null c 1 3") == 0); + assert(system("rm -f /tmp/test_dev_cgroup_null") == 0); + + assert(system("dd if=/dev/urandom of=/dev/null count=64") == 0);
/* src is allowed, target is forbidden */ assert(system("dd if=/dev/urandom of=/dev/full count=64"));
/* src is forbidden, target is allowed */ - assert(system("dd if=/dev/random of=/dev/zero count=64")); + assert(system("dd if=/dev/random of=/dev/null count=64"));
error = 0; printf("test_dev_cgroup:PASS\n");
On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
test_dev_cgroup currently loads a small bpf program allowing any access on urandom and zero devices, disabling access to any other device. It makes migrating this test to test_progs impossible, since this one manipulates extensively /dev/null.
Allow /dev/null manipulation in dev_cgroup program to make its usage in test_progs framework possible. Update test_dev_cgroup.c as well to match this change while it has not been removed.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
tools/testing/selftests/bpf/progs/dev_cgroup.c | 4 ++-- tools/testing/selftests/bpf/test_dev_cgroup.c | 18 +++++++++---------
Not a big deal, but I found it a bit confusing that this file was modified then deleted in patch 2. Would it work having patch 1 stop building the standalone test/remove it and .gitignore entry, patch 2 updating progs/dev_cgroup.c to allow /dev/zero, /dev/urandom access, patch 3 add cgroup_dev.c test support, and patch 4 add the device type subtest? Or are there issues with doing things that way? Thanks!
Alan
Hello Alan,
On 7/29/24 18:59, Alan Maguire wrote:
On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
test_dev_cgroup currently loads a small bpf program allowing any access on urandom and zero devices, disabling access to any other device. It makes migrating this test to test_progs impossible, since this one manipulates extensively /dev/null.
Allow /dev/null manipulation in dev_cgroup program to make its usage in test_progs framework possible. Update test_dev_cgroup.c as well to match this change while it has not been removed.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
tools/testing/selftests/bpf/progs/dev_cgroup.c | 4 ++-- tools/testing/selftests/bpf/test_dev_cgroup.c | 18 +++++++++---------
Not a big deal, but I found it a bit confusing that this file was modified then deleted in patch 2. Would it work having patch 1 stop building the standalone test/remove it and .gitignore entry, patch 2 updating progs/dev_cgroup.c to allow /dev/zero, /dev/urandom access, patch 3 add cgroup_dev.c test support, and patch 4 add the device type subtest? Or are there issues with doing things that way? Thanks!
I've done this to make sure that at any point in the git history, there is one working test for the targeted feature, either the old or the new one. I've done it this way because the old test also helped me validate the new one while developing it, but also because if at some point there is a (major) issue with the new test, reverting only the relevant commit brings back the old test while disabling the new one.
But maybe this concern is not worth the trouble (especially since the old tests are not run automatically) ? If that's indeed the case, I can do it the way you are suggesting :)
Thanks,
Alexis
On 29/07/2024 18:30, Alexis Lothoré wrote:
Hello Alan,
On 7/29/24 18:59, Alan Maguire wrote:
On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
test_dev_cgroup currently loads a small bpf program allowing any access on urandom and zero devices, disabling access to any other device. It makes migrating this test to test_progs impossible, since this one manipulates extensively /dev/null.
Allow /dev/null manipulation in dev_cgroup program to make its usage in test_progs framework possible. Update test_dev_cgroup.c as well to match this change while it has not been removed.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
tools/testing/selftests/bpf/progs/dev_cgroup.c | 4 ++-- tools/testing/selftests/bpf/test_dev_cgroup.c | 18 +++++++++---------
Not a big deal, but I found it a bit confusing that this file was modified then deleted in patch 2. Would it work having patch 1 stop building the standalone test/remove it and .gitignore entry, patch 2 updating progs/dev_cgroup.c to allow /dev/zero, /dev/urandom access, patch 3 add cgroup_dev.c test support, and patch 4 add the device type subtest? Or are there issues with doing things that way? Thanks!
I've done this to make sure that at any point in the git history, there is one working test for the targeted feature, either the old or the new one. I've done it this way because the old test also helped me validate the new one while developing it, but also because if at some point there is a (major) issue with the new test, reverting only the relevant commit brings back the old test while disabling the new one.
But maybe this concern is not worth the trouble (especially since the old tests are not run automatically) ? If that's indeed the case, I can do it the way you are suggesting :)
If no-one complains, it seems fine to me to stick with the way you've constructed the series the next respin. Thanks!
Thanks,
Alexis
On 7/30/24 10:16, Alan Maguire wrote:
On 29/07/2024 18:30, Alexis Lothoré wrote:
Hello Alan,
[...]
Not a big deal, but I found it a bit confusing that this file was modified then deleted in patch 2. Would it work having patch 1 stop building the standalone test/remove it and .gitignore entry, patch 2 updating progs/dev_cgroup.c to allow /dev/zero, /dev/urandom access, patch 3 add cgroup_dev.c test support, and patch 4 add the device type subtest? Or are there issues with doing things that way? Thanks!
I've done this to make sure that at any point in the git history, there is one working test for the targeted feature, either the old or the new one. I've done it this way because the old test also helped me validate the new one while developing it, but also because if at some point there is a (major) issue with the new test, reverting only the relevant commit brings back the old test while disabling the new one.
But maybe this concern is not worth the trouble (especially since the old tests are not run automatically) ? If that's indeed the case, I can do it the way you are suggesting :)
If no-one complains, it seems fine to me to stick with the way you've constructed the series the next respin. Thanks!
ACK, thanks, I'll keep it that way then.
For the record, I am accumulating a few other converted tests that I will send soon, and those follow the same logic (keeping one working test at any point of time, and pushing it to the point where I start by fixing broken tests before converting those), so if anyone has an opinion in favor of this or rather in favor of Alan's suggestion, do not hesitate to share it, so I can adjust before sending.
Thanks,
Thanks,
Alexis
test_dev_cgroup is defined as a standalone test program, and so is not executed in CI.
Convert it to test_progs framework so it is tested automatically in CI, and remove the old test. In order to be able to run it in test_progs, /dev/null must remain usable, so change the new test to test operations on devices 1:3 as valid, and operations on devices 1:5 (/dev/zero) as invalid.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com --- Changes in v2: - pass expected return code to subtest function instead of boolean pass/not pass - also pass buffer and buffer size for read/write subtests - fix faulty fd check on read/write tests expected to fail --- tools/testing/selftests/bpf/.gitignore | 1 - tools/testing/selftests/bpf/Makefile | 2 - .../testing/selftests/bpf/prog_tests/cgroup_dev.c | 110 +++++++++++++++++++++ tools/testing/selftests/bpf/test_dev_cgroup.c | 85 ---------------- 4 files changed, 110 insertions(+), 88 deletions(-)
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index 4e4aae8aa7ec..8f14d8faeb0b 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -9,7 +9,6 @@ test_lpm_map test_tag FEATURE-DUMP.libbpf fixdep -test_dev_cgroup /test_progs /test_progs-no_alu32 /test_progs-bpf_gcc diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index aeada478e37a..2a9ba2246f80 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -69,7 +69,6 @@ endif
# Order correspond to 'make run_tests' order TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \ - test_dev_cgroup \ test_sock test_sockmap get_cgroup_id_user \ test_cgroup_storage \ test_tcpnotify_user test_sysctl \ @@ -295,7 +294,6 @@ JSON_WRITER := $(OUTPUT)/json_writer.o CAP_HELPERS := $(OUTPUT)/cap_helpers.o NETWORK_HELPERS := $(OUTPUT)/network_helpers.o
-$(OUTPUT)/test_dev_cgroup: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(OUTPUT)/test_skb_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(OUTPUT)/test_sock: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS) diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c new file mode 100644 index 000000000000..af0b70086c21 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <sys/stat.h> +#include <sys/sysmacros.h> +#include "test_progs.h" +#include "cgroup_helpers.h" +#include "dev_cgroup.skel.h" + +#define TEST_CGROUP "/test-bpf-based-device-cgroup/" +#define TEST_BUFFER_SIZE 64 + +static void test_mknod(const char *path, mode_t mode, int dev_major, + int dev_minor, int expected_ret) +{ + int ret; + + unlink(path); + ret = mknod(path, mode, makedev(dev_major, dev_minor)); + ASSERT_EQ(ret, expected_ret, "mknod"); + unlink(path); +} + +static void test_read(const char *path, char *buf, int buf_size, int expected_ret) +{ + int ret, fd; + + fd = open(path, O_RDONLY); + + /* A bare open on unauthorized device should fail */ + if (expected_ret < 0) { + ASSERT_EQ(fd, expected_ret, "open file for read"); + if (fd >= 0) + close(fd); + return; + } + + if (!ASSERT_OK_FD(fd, "open file for read")) + return; + + ret = read(fd, buf, buf_size); + ASSERT_EQ(ret, expected_ret, "read"); + + close(fd); +} + +static void test_write(const char *path, char *buf, int buf_size, int expected_ret) +{ + int ret, fd; + + fd = open(path, O_WRONLY); + + /* A bare open on unauthorized device should fail */ + if (expected_ret < 0) { + ASSERT_EQ(fd, expected_ret, "open file for write"); + if (fd >= 0) + close(fd); + return; + } + + if (!ASSERT_OK_FD(fd, "open file for write")) + return; + + ret = write(fd, buf, buf_size); + ASSERT_EQ(ret, expected_ret, "write"); + + close(fd); +} + +void test_cgroup_dev(void) +{ + char buf[TEST_BUFFER_SIZE] = "some random test data"; + struct dev_cgroup *skel; + int cgroup_fd; + + cgroup_fd = cgroup_setup_and_join(TEST_CGROUP); + if (!ASSERT_OK_FD(cgroup_fd, "cgroup switch")) + return; + + skel = dev_cgroup__open_and_load(); + if (!ASSERT_OK_PTR(skel, "load program")) + goto cleanup_cgroup; + + if (!ASSERT_OK(bpf_prog_attach(bpf_program__fd(skel->progs.bpf_prog1), + cgroup_fd, BPF_CGROUP_DEVICE, 0), + "attach_program")) + goto cleanup_progs; + + if (test__start_subtest("deny-mknod")) + test_mknod("/dev/test_dev_cgroup_zero", S_IFCHR, 1, 5, -EPERM); + + if (test__start_subtest("allow-mknod")) + test_mknod("/dev/test_dev_cgroup_null", S_IFCHR, 1, 3, 0); + + if (test__start_subtest("allow-read")) + test_read("/dev/urandom", buf, TEST_BUFFER_SIZE, TEST_BUFFER_SIZE); + + if (test__start_subtest("allow-write")) + test_write("/dev/null", buf, TEST_BUFFER_SIZE, TEST_BUFFER_SIZE); + + if (test__start_subtest("deny-read")) + test_read("/dev/random", buf, TEST_BUFFER_SIZE, -EPERM); + + if (test__start_subtest("deny-write")) + test_write("/dev/zero", buf, TEST_BUFFER_SIZE, -EPERM); + +cleanup_progs: + dev_cgroup__destroy(skel); +cleanup_cgroup: + cleanup_cgroup_environment(); +} diff --git a/tools/testing/selftests/bpf/test_dev_cgroup.c b/tools/testing/selftests/bpf/test_dev_cgroup.c deleted file mode 100644 index 33f544f0005a..000000000000 --- a/tools/testing/selftests/bpf/test_dev_cgroup.c +++ /dev/null @@ -1,85 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* Copyright (c) 2017 Facebook - */ - -#include <stdio.h> -#include <stdlib.h> -#include <string.h> -#include <errno.h> -#include <assert.h> -#include <sys/time.h> - -#include <linux/bpf.h> -#include <bpf/bpf.h> -#include <bpf/libbpf.h> - -#include "cgroup_helpers.h" -#include "testing_helpers.h" - -#define DEV_CGROUP_PROG "./dev_cgroup.bpf.o" - -#define TEST_CGROUP "/test-bpf-based-device-cgroup/" - -int main(int argc, char **argv) -{ - struct bpf_object *obj; - int error = EXIT_FAILURE; - int prog_fd, cgroup_fd; - __u32 prog_cnt; - - /* Use libbpf 1.0 API mode */ - libbpf_set_strict_mode(LIBBPF_STRICT_ALL); - - if (bpf_prog_test_load(DEV_CGROUP_PROG, BPF_PROG_TYPE_CGROUP_DEVICE, - &obj, &prog_fd)) { - printf("Failed to load DEV_CGROUP program\n"); - goto out; - } - - cgroup_fd = cgroup_setup_and_join(TEST_CGROUP); - if (cgroup_fd < 0) { - printf("Failed to create test cgroup\n"); - goto out; - } - - /* Attach bpf program */ - if (bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_DEVICE, 0)) { - printf("Failed to attach DEV_CGROUP program"); - goto err; - } - - if (bpf_prog_query(cgroup_fd, BPF_CGROUP_DEVICE, 0, NULL, NULL, - &prog_cnt)) { - printf("Failed to query attached programs"); - goto err; - } - - /* All operations with /dev/null and /dev/urandom are allowed, - * everything else is forbidden. - */ - assert(system("rm -f /tmp/test_dev_cgroup_zero") == 0); - assert(system("mknod /tmp/test_dev_cgroup_zero c 1 5")); - assert(system("rm -f /tmp/test_dev_cgroup_zero") == 0); - - /* /dev/null is whitelisted */ - assert(system("rm -f /tmp/test_dev_cgroup_null") == 0); - assert(system("mknod /tmp/test_dev_cgroup_null c 1 3") == 0); - assert(system("rm -f /tmp/test_dev_cgroup_null") == 0); - - assert(system("dd if=/dev/urandom of=/dev/null count=64") == 0); - - /* src is allowed, target is forbidden */ - assert(system("dd if=/dev/urandom of=/dev/full count=64")); - - /* src is forbidden, target is allowed */ - assert(system("dd if=/dev/random of=/dev/null count=64")); - - error = 0; - printf("test_dev_cgroup:PASS\n"); - -err: - cleanup_cgroup_environment(); - -out: - return error; -}
On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
test_dev_cgroup is defined as a standalone test program, and so is not executed in CI.
Convert it to test_progs framework so it is tested automatically in CI, and remove the old test. In order to be able to run it in test_progs, /dev/null must remain usable, so change the new test to test operations on devices 1:3 as valid, and operations on devices 1:5 (/dev/zero) as invalid.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
A few small suggestions but looks great!
Reviewed-by: Alan Maguire alan.maguire@oracle.com
Changes in v2:
- pass expected return code to subtest function instead of boolean pass/not pass
- also pass buffer and buffer size for read/write subtests
- fix faulty fd check on read/write tests expected to fail
tools/testing/selftests/bpf/.gitignore | 1 - tools/testing/selftests/bpf/Makefile | 2 - .../testing/selftests/bpf/prog_tests/cgroup_dev.c | 110 +++++++++++++++++++++ tools/testing/selftests/bpf/test_dev_cgroup.c | 85 ---------------- 4 files changed, 110 insertions(+), 88 deletions(-)
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore index 4e4aae8aa7ec..8f14d8faeb0b 100644 --- a/tools/testing/selftests/bpf/.gitignore +++ b/tools/testing/selftests/bpf/.gitignore @@ -9,7 +9,6 @@ test_lpm_map test_tag FEATURE-DUMP.libbpf fixdep -test_dev_cgroup /test_progs /test_progs-no_alu32 /test_progs-bpf_gcc diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile index aeada478e37a..2a9ba2246f80 100644 --- a/tools/testing/selftests/bpf/Makefile +++ b/tools/testing/selftests/bpf/Makefile @@ -69,7 +69,6 @@ endif # Order correspond to 'make run_tests' order TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test_progs \
- test_dev_cgroup \ test_sock test_sockmap get_cgroup_id_user \ test_cgroup_storage \ test_tcpnotify_user test_sysctl \
@@ -295,7 +294,6 @@ JSON_WRITER := $(OUTPUT)/json_writer.o CAP_HELPERS := $(OUTPUT)/cap_helpers.o NETWORK_HELPERS := $(OUTPUT)/network_helpers.o -$(OUTPUT)/test_dev_cgroup: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(OUTPUT)/test_skb_cgroup_id_user: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(OUTPUT)/test_sock: $(CGROUP_HELPERS) $(TESTING_HELPERS) $(OUTPUT)/test_sockmap: $(CGROUP_HELPERS) $(TESTING_HELPERS) diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c new file mode 100644 index 000000000000..af0b70086c21 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c @@ -0,0 +1,110 @@ +// SPDX-License-Identifier: GPL-2.0
+#include <sys/stat.h> +#include <sys/sysmacros.h> +#include "test_progs.h" +#include "cgroup_helpers.h" +#include "dev_cgroup.skel.h"
+#define TEST_CGROUP "/test-bpf-based-device-cgroup/" +#define TEST_BUFFER_SIZE 64
+static void test_mknod(const char *path, mode_t mode, int dev_major,
int dev_minor, int expected_ret)+{
- int ret;
- unlink(path);
- ret = mknod(path, mode, makedev(dev_major, dev_minor));
- ASSERT_EQ(ret, expected_ret, "mknod");
no need to unlink unless "if (!ret)"
- unlink(path);
+}
+static void test_read(const char *path, char *buf, int buf_size, int expected_ret) +{
- int ret, fd;
- fd = open(path, O_RDONLY);
- /* A bare open on unauthorized device should fail */
- if (expected_ret < 0) {
ASSERT_EQ(fd, expected_ret, "open file for read");if (fd >= 0)close(fd);return;- }
- if (!ASSERT_OK_FD(fd, "open file for read"))
return;- ret = read(fd, buf, buf_size);
- ASSERT_EQ(ret, expected_ret, "read");
- close(fd);
+}
+static void test_write(const char *path, char *buf, int buf_size, int expected_ret) +{
- int ret, fd;
- fd = open(path, O_WRONLY);
- /* A bare open on unauthorized device should fail */
- if (expected_ret < 0) {
ASSERT_EQ(fd, expected_ret, "open file for write");if (fd >= 0)close(fd);return;- }
- if (!ASSERT_OK_FD(fd, "open file for write"))
return;- ret = write(fd, buf, buf_size);
- ASSERT_EQ(ret, expected_ret, "write");
- close(fd);
+}
+void test_cgroup_dev(void) +{
- char buf[TEST_BUFFER_SIZE] = "some random test data";
- struct dev_cgroup *skel;
- int cgroup_fd;
- cgroup_fd = cgroup_setup_and_join(TEST_CGROUP);
- if (!ASSERT_OK_FD(cgroup_fd, "cgroup switch"))
return;- skel = dev_cgroup__open_and_load();
- if (!ASSERT_OK_PTR(skel, "load program"))
goto cleanup_cgroup;- if (!ASSERT_OK(bpf_prog_attach(bpf_program__fd(skel->progs.bpf_prog1),
cgroup_fd, BPF_CGROUP_DEVICE, 0),"attach_program"))
I'd suggest using bpf_program__attach_cgroup() here as you can assign the link in the skeleton; see prog_tests/cgroup_v1v2.c.
goto cleanup_progs;- if (test__start_subtest("deny-mknod"))
test_mknod("/dev/test_dev_cgroup_zero", S_IFCHR, 1, 5, -EPERM);
nit: group with other deny subtests.
- if (test__start_subtest("allow-mknod"))
test_mknod("/dev/test_dev_cgroup_null", S_IFCHR, 1, 3, 0);- if (test__start_subtest("allow-read"))
test_read("/dev/urandom", buf, TEST_BUFFER_SIZE, TEST_BUFFER_SIZE);
Nit: should we have a separate garbage buffer for the successful /dev/urandom read? We're not validating buffer contents anywhere but we will overwrite our test string I think and it'll end up non-null terminated.
Alan
Hello Alan, thanks for the review
On 7/29/24 19:29, Alan Maguire wrote:
On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
test_dev_cgroup is defined as a standalone test program, and so is not executed in CI.
Convert it to test_progs framework so it is tested automatically in CI, and remove the old test. In order to be able to run it in test_progs, /dev/null must remain usable, so change the new test to test operations on devices 1:3 as valid, and operations on devices 1:5 (/dev/zero) as invalid.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
A few small suggestions but looks great!
Reviewed-by: Alan Maguire alan.maguire@oracle.com
[...]
- unlink(path);
- ret = mknod(path, mode, makedev(dev_major, dev_minor));
- ASSERT_EQ(ret, expected_ret, "mknod");
no need to unlink unless "if (!ret)"
Indeed, you are right.
[...]
- skel = dev_cgroup__open_and_load();
- if (!ASSERT_OK_PTR(skel, "load program"))
goto cleanup_cgroup;- if (!ASSERT_OK(bpf_prog_attach(bpf_program__fd(skel->progs.bpf_prog1),
cgroup_fd, BPF_CGROUP_DEVICE, 0),"attach_program"))I'd suggest using bpf_program__attach_cgroup() here as you can assign the link in the skeleton; see prog_tests/cgroup_v1v2.c.
Ah yes, thanks for the hint !
goto cleanup_progs;- if (test__start_subtest("deny-mknod"))
test_mknod("/dev/test_dev_cgroup_zero", S_IFCHR, 1, 5, -EPERM);nit: group with other deny subtests.
ACK
- if (test__start_subtest("allow-mknod"))
test_mknod("/dev/test_dev_cgroup_null", S_IFCHR, 1, 3, 0);- if (test__start_subtest("allow-read"))
test_read("/dev/urandom", buf, TEST_BUFFER_SIZE, TEST_BUFFER_SIZE);Nit: should we have a separate garbage buffer for the successful /dev/urandom read? We're not validating buffer contents anywhere but we will overwrite our test string I think and it'll end up non-null terminated.
True, but since the tests aren't performing any string operation on it, is it really a big deal ? I can even switch the string to a byte array, if it can prevent any mistake.
If that's ok for you, I can bring all the suggestions discussed here in a new revision and keep your review tag.
Thanks,
Alexis
Alan
On 29/07/2024 18:47, Alexis Lothoré wrote:
Hello Alan, thanks for the review
On 7/29/24 19:29, Alan Maguire wrote:
On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
test_dev_cgroup is defined as a standalone test program, and so is not executed in CI.
Convert it to test_progs framework so it is tested automatically in CI, and remove the old test. In order to be able to run it in test_progs, /dev/null must remain usable, so change the new test to test operations on devices 1:3 as valid, and operations on devices 1:5 (/dev/zero) as invalid.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
A few small suggestions but looks great!
Reviewed-by: Alan Maguire alan.maguire@oracle.com
[...]
- unlink(path);
- ret = mknod(path, mode, makedev(dev_major, dev_minor));
- ASSERT_EQ(ret, expected_ret, "mknod");
no need to unlink unless "if (!ret)"
Indeed, you are right.
[...]
- skel = dev_cgroup__open_and_load();
- if (!ASSERT_OK_PTR(skel, "load program"))
goto cleanup_cgroup;- if (!ASSERT_OK(bpf_prog_attach(bpf_program__fd(skel->progs.bpf_prog1),
cgroup_fd, BPF_CGROUP_DEVICE, 0),"attach_program"))I'd suggest using bpf_program__attach_cgroup() here as you can assign the link in the skeleton; see prog_tests/cgroup_v1v2.c.
Ah yes, thanks for the hint !
goto cleanup_progs;- if (test__start_subtest("deny-mknod"))
test_mknod("/dev/test_dev_cgroup_zero", S_IFCHR, 1, 5, -EPERM);nit: group with other deny subtests.
ACK
- if (test__start_subtest("allow-mknod"))
test_mknod("/dev/test_dev_cgroup_null", S_IFCHR, 1, 3, 0);- if (test__start_subtest("allow-read"))
test_read("/dev/urandom", buf, TEST_BUFFER_SIZE, TEST_BUFFER_SIZE);Nit: should we have a separate garbage buffer for the successful /dev/urandom read? We're not validating buffer contents anywhere but we will overwrite our test string I think and it'll end up non-null terminated.
True, but since the tests aren't performing any string operation on it, is it really a big deal ? I can even switch the string to a byte array, if it can prevent any mistake.
There's no need, don't worry. As long as the size limits ensure we don't overrun the buffer, we're good.
If that's ok for you, I can bring all the suggestions discussed here in a new revision and keep your review tag.
Sounds great, thanks!
Alan
Current cgroup_dev test mostly tests that device operation is accepted or refused base on passed major/minor (and so, any operation performed during test involves only char device)
Add a small subtest ensuring that the device type passed to bpf program allows it to take decisions as well.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com --- Changes in v2: - change test name ("null" block device does not make sense) - use updated subtest API for this new subtest --- tools/testing/selftests/bpf/prog_tests/cgroup_dev.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c index af0b70086c21..a840973c87b1 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c @@ -91,6 +91,9 @@ void test_cgroup_dev(void) if (test__start_subtest("allow-mknod")) test_mknod("/dev/test_dev_cgroup_null", S_IFCHR, 1, 3, 0);
+ if (test__start_subtest("deny-mknod-wrong-type")) + test_mknod("/dev/test_dev_cgroup_block", S_IFBLK, 1, 3, -EPERM); + if (test__start_subtest("allow-read")) test_read("/dev/urandom", buf, TEST_BUFFER_SIZE, TEST_BUFFER_SIZE);
On 29/07/2024 09:20, Alexis Lothoré (eBPF Foundation) wrote:
Current cgroup_dev test mostly tests that device operation is accepted or refused base on passed major/minor (and so, any operation performed during test involves only char device)
Add a small subtest ensuring that the device type passed to bpf program allows it to take decisions as well.
Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com
Reviewed-by: Alan Maguire alan.maguire@oracle.com
Changes in v2:
- change test name ("null" block device does not make sense)
- use updated subtest API for this new subtest
tools/testing/selftests/bpf/prog_tests/cgroup_dev.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c index af0b70086c21..a840973c87b1 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c @@ -91,6 +91,9 @@ void test_cgroup_dev(void) if (test__start_subtest("allow-mknod")) test_mknod("/dev/test_dev_cgroup_null", S_IFCHR, 1, 3, 0);
- if (test__start_subtest("deny-mknod-wrong-type"))
test_mknod("/dev/test_dev_cgroup_block", S_IFBLK, 1, 3, -EPERM);- if (test__start_subtest("allow-read")) test_read("/dev/urandom", buf, TEST_BUFFER_SIZE, TEST_BUFFER_SIZE);
On 07/29, Alexis Lothoré (eBPF Foundation) wrote:
Hello, this small series aims to integrate test_dev_cgroup in test_progs so it could be run automatically in CI. The new version brings a few differences with the current one:
- test now uses directly syscalls instead of wrapping commandline tools into system() calls
- test_progs manipulates /dev/null (eg: redirecting test logs into it), so disabling access to it in the bpf program confuses the tests. To fix this, the first commit modifies the bpf program to allow access to char devices 1:3 (/dev/null), and disable access to char devices 1:5 (/dev/zero)
- once test is converted, add a small subtest to also check for device type interpretation (char or block)
- paths used in mknod tests are now in /dev instead of /tmp: due to the CI runner organisation and mountpoints manipulations, trying to create nodes in /tmp leads to errors unrelated to the test (ie, mknod calls refused by kernel, not the bpf program). I don't understand exactly the root cause at the deepest point (all I see in CI is an -ENXIO error on mknod when trying to create the node in tmp, and I can not make sense out of it neither replicate it locally), so I would gladly take inputs from anyone more educated than me about this.
The new test_progs part has been tested in a local qemu environment as well as in upstream CI:
./test_progs -a cgroup_dev 47/1 cgroup_dev/deny-mknod:OK 47/2 cgroup_dev/allow-mknod:OK 47/3 cgroup_dev/deny-mknod-wrong-type:OK 47/4 cgroup_dev/allow-read:OK 47/5 cgroup_dev/allow-write:OK 47/6 cgroup_dev/deny-read:OK 47/7 cgroup_dev/deny-write:OK 47 cgroup_dev:OK Summary: 1/7 PASSED, 0 SKIPPED, 0 FAILED
Changes in v2:
- directly pass expected ret code to subtests instead of boolean pass/not pass
- fix faulty fd check in subtest expected to fail on open
- fix wrong subtest name
- pass test buffer and corresponding size to read/write subtests
- use correct series prefix
- Link to v1: https://lore.kernel.org/r/20240725-convert_dev_cgroup-v1-0-2c8cbd487c44@boot...
For the next respin (after addressing Alan's comment about bpf_program__attach_cgroup):
Acked-by: Stanislav Fomichev sdf@fomichev.me
linux-kselftest-mirror@lists.linaro.org
-
Alan Maguire -
Alexis Lothoré -
Alexis Lothoré (eBPF Foundation)
-
Stanislav Fomichev