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/allow-mknod:OK 47/2 cgroup_dev/allow-read:OK 47/3 cgroup_dev/allow-write:OK 47/4 cgroup_dev/deny-mknod:OK 47/5 cgroup_dev/deny-read:OK 47/6 cgroup_dev/deny-write:OK 47/7 cgroup_dev/deny-mknod-wrong-type:OK 47 cgroup_dev:OK Summary: 1/7 PASSED, 0 SKIPPED, 0 FAILED
--- Changes in v3: - delete mknod file only if it has been created - use bpf_program__attach_cgroup() instead of bpf_prog_attach - reorganize subtests order - collect review/ack tags from Alan and Stanislas - Link to v2: https://lore.kernel.org/r/20240729-convert_dev_cgroup-v2-0-4c1fc0520545@boot...
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 | 118 +++++++++++++++++++++ tools/testing/selftests/bpf/progs/dev_cgroup.c | 4 +- tools/testing/selftests/bpf/test_dev_cgroup.c | 85 --------------- 5 files changed, 120 insertions(+), 90 deletions(-) --- base-commit: 2107cb4bff1c21110ebf7a17458a918282c1a8c9 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 30/07/2024 12:59, 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
Reviewed-by: Alan Maguire alan.maguire@oracle.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(-)
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.
Reviewed-by: Alan Maguire alan.maguire@oracle.com Acked-by: Stanislav Fomichev sdf@fomichev.me Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com --- Changes in v3: - delete mknod file only when is has been created - use bpf_program__attach_cgroup instead of bpf_prog_attach - reorder subtests - collect review/ack tags
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 | 115 +++++++++++++++++++++ tools/testing/selftests/bpf/test_dev_cgroup.c | 85 --------------- 4 files changed, 115 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 564e1fdf80b6..bb02a3540d7a 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 \ @@ -294,7 +293,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..62a2ca9a0b94 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c @@ -0,0 +1,115 @@ +// 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"); + 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; + + skel->links.bpf_prog1 = + bpf_program__attach_cgroup(skel->progs.bpf_prog1, cgroup_fd); + if (!ASSERT_OK_PTR(skel->links.bpf_prog1, "attach_program")) + goto cleanup_progs; + + 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-mknod")) + test_mknod("/dev/test_dev_cgroup_zero", S_IFCHR, 1, 5, -EPERM); + + 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 7/30/24 4:59 AM, Alexis Lothoré (eBPF Foundation) wrote:
+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");
One nit. expected_ret is actually expected_errno. It just happens -EPERM is -1, so testing fd against expected_errno works here but is confusing to read. How about separating the fd and errno test in the access rejected case. First test for fd == -1 and then test for errno == expected_errno.
Please also carry Stanislav's Ack in patch 1 and 3 in the next respin.
Thanks for helping to move this test to test_progs.
pw-bot: cr
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;- skel->links.bpf_prog1 =
bpf_program__attach_cgroup(skel->progs.bpf_prog1, cgroup_fd);- if (!ASSERT_OK_PTR(skel->links.bpf_prog1, "attach_program"))
goto cleanup_progs;- 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-mknod"))
test_mknod("/dev/test_dev_cgroup_zero", S_IFCHR, 1, 5, -EPERM);- 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();
+}
Hello Martin,
On 7/31/24 02:34, Martin KaFai Lau wrote:
On 7/30/24 4:59 AM, Alexis Lothoré (eBPF Foundation) wrote:
+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");
One nit. expected_ret is actually expected_errno. It just happens -EPERM is -1, so testing fd against expected_errno works here but is confusing to read. How about separating the fd and errno test in the access rejected case. First test for fd == -1 and then test for errno == expected_errno.
Ah you are right, I mixed up things here, I'll fix it.
Please also carry Stanislav's Ack in patch 1 and 3 in the next respin.
Sure, will do.
Thanks,
Alexis
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.
Reviewed-by: Alan Maguire alan.maguire@oracle.com Signed-off-by: Alexis Lothoré (eBPF Foundation) alexis.lothore@bootlin.com --- Changes in v3: - reorganize subtests order
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 62a2ca9a0b94..49c3d09ff259 100644 --- a/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c @@ -108,6 +108,9 @@ void test_cgroup_dev(void) if (test__start_subtest("deny-write")) test_write("/dev/zero", buf, TEST_BUFFER_SIZE, -EPERM);
+ if (test__start_subtest("deny-mknod-wrong-type")) + test_mknod("/dev/test_dev_cgroup_block", S_IFBLK, 1, 3, -EPERM); + cleanup_progs: dev_cgroup__destroy(skel); cleanup_cgroup:
linux-kselftest-mirror@lists.linaro.org
-
Alan Maguire -
Alexis Lothoré -
Alexis Lothoré (eBPF Foundation)
-
Martin KaFai Lau