[PATCH 0/3] selftests/bpf: convert test_dev_cgroup to test_progs

[PATCH 0/3] selftests/bpf: convert test_dev_cgroup to test_progs

[Alexis Lothoré (eBPF Foundation)]

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

---
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  | 123 +++++++++++++++++++++
 tools/testing/selftests/bpf/progs/dev_cgroup.c     |   4 +-
 tools/testing/selftests/bpf/test_dev_cgroup.c      |  85 --------------
 5 files changed, 125 insertions(+), 90 deletions(-)
---
base-commit: c90e2d4a7738a24fbb3657092dbd1ca20c040ed1
change-id: 20240723-convert_dev_cgroup-6464b0d37f1a

Best regards,
-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH 1/3] selftests/bpf: do not disable /dev/null device access in cgroup dev test

[Alexis Lothoré (eBPF Foundation)]

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");

-- 
2.45.2

[PATCH 2/3] selftests/bpf: convert test_dev_cgroup to test_progs

[Alexis Lothoré (eBPF Foundation)]

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>
---
 tools/testing/selftests/bpf/.gitignore             |   1 -
 tools/testing/selftests/bpf/Makefile               |   2 -
 .../testing/selftests/bpf/prog_tests/cgroup_dev.c  | 120 +++++++++++++++++++++
 tools/testing/selftests/bpf/test_dev_cgroup.c      |  85 ---------------
 4 files changed, 120 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..5112b99213ad
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c
@@ -0,0 +1,120 @@
+// 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 should_fail)
+{
+	int ret;
+
+	unlink(path);
+	ret = mknod(path, mode, makedev(dev_major, dev_minor));
+	if (should_fail)
+		ASSERT_ERR(ret, "mknod");
+	else
+		ASSERT_OK(ret, "mknod");
+	unlink(path);
+}
+
+static void test_read(const char *path, int should_fail)
+{
+	char buf[TEST_BUFFER_SIZE];
+	int ret, fd;
+
+	fd = open(path, O_RDONLY);
+
+	/* A bare open on unauthorized device should fail */
+	if (should_fail) {
+		ASSERT_ERR(fd, "open file for read");
+		if (fd)
+			close(fd);
+		return;
+	}
+
+	if (!ASSERT_OK_FD(fd, "open file for read"))
+		return;
+
+	ret = read(fd, buf, TEST_BUFFER_SIZE);
+	if (should_fail)
+		ASSERT_ERR(ret, "read");
+	else
+		ASSERT_EQ(ret, TEST_BUFFER_SIZE, "read");
+
+	close(fd);
+}
+
+static void test_write(const char *path, int should_fail)
+{
+	char buf[] = "some random test data";
+	int ret, fd;
+
+	fd = open(path, O_WRONLY);
+
+	/* A bare open on unauthorized device should fail */
+	if (should_fail) {
+		ASSERT_ERR(fd, "open file for write");
+		if (fd)
+			close(fd);
+		return;
+	}
+
+	if (!ASSERT_OK_FD(fd, "open file for write"))
+		return;
+
+	ret = write(fd, buf, sizeof(buf));
+	if (should_fail)
+		ASSERT_ERR(ret, "write");
+	else
+		ASSERT_EQ(ret, sizeof(buf), "write");
+
+	close(fd);
+}
+
+void test_cgroup_dev(void)
+{
+	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, 1);
+
+	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", 0);
+
+	if (test__start_subtest("allow-write"))
+		test_write("/dev/null", 0);
+
+	if (test__start_subtest("deny-read"))
+		test_read("/dev/random", 1);
+
+	if (test__start_subtest("deny-write"))
+		test_write("/dev/zero", 1);
+
+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;
-}

-- 
2.45.2

[PATCH 3/3] selftests/bpf: add wrong type test to cgroup dev

[Alexis Lothoré (eBPF Foundation)]

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>
---
 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 5112b99213ad..f0b6e5d9604b 100644
--- a/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c
+++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c
@@ -101,6 +101,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_null_block", S_IFBLK, 1, 3, 1);
+
 	if (test__start_subtest("allow-read"))
 		test_read("/dev/urandom", 0);
 

-- 
2.45.2

Re: [PATCH 2/3] selftests/bpf: convert test_dev_cgroup to test_progs

[Stanislav Fomichev]

On 07/25, 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>
> ---
>  tools/testing/selftests/bpf/.gitignore             |   1 -
>  tools/testing/selftests/bpf/Makefile               |   2 -
>  .../testing/selftests/bpf/prog_tests/cgroup_dev.c  | 120 +++++++++++++++++++++
>  tools/testing/selftests/bpf/test_dev_cgroup.c      |  85 ---------------
>  4 files changed, 120 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..5112b99213ad
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/cgroup_dev.c
> @@ -0,0 +1,120 @@
> +// 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 should_fail)
> +{
> +	int ret;
> +
> +	unlink(path);
> +	ret = mknod(path, mode, makedev(dev_major, dev_minor));

[..]

> +	if (should_fail)
> +		ASSERT_ERR(ret, "mknod");
> +	else
> +		ASSERT_OK(ret, "mknod");

Optional: might be easier to use something like expected_ret instead
of should_fail and then do:

ASSERT_EQ(ret, expected_ret)

I see this part being copy-pasted in a bunch of places below.

> +	unlink(path);
> +}
> +
> +static void test_read(const char *path, int should_fail)
> +{
> +	char buf[TEST_BUFFER_SIZE];
> +	int ret, fd;
> +
> +	fd = open(path, O_RDONLY);
> +
> +	/* A bare open on unauthorized device should fail */
> +	if (should_fail) {
> +		ASSERT_ERR(fd, "open file for read");

[..]

> +		if (fd)
> +			close(fd);

nit: should this be 'if (fd >= 0)'? I'm assuming the intention is to
avoid close(-1)?

> +		return;
> +	}
> +
> +	if (!ASSERT_OK_FD(fd, "open file for read"))
> +		return;
> +
> +	ret = read(fd, buf, TEST_BUFFER_SIZE);
> +	if (should_fail)
> +		ASSERT_ERR(ret, "read");
> +	else
> +		ASSERT_EQ(ret, TEST_BUFFER_SIZE, "read");
> +
> +	close(fd);
> +}
> +
> +static void test_write(const char *path, int should_fail)
> +{
> +	char buf[] = "some random test data";
> +	int ret, fd;
> +
> +	fd = open(path, O_WRONLY);
> +
> +	/* A bare open on unauthorized device should fail */
> +	if (should_fail) {
> +		ASSERT_ERR(fd, "open file for write");
> +		if (fd)
> +			close(fd);

Same 'if (fd >= 0)'

Re: [PATCH 0/3] selftests/bpf: convert test_dev_cgroup to test_progs

[Stanislav Fomichev]

On 07/25, 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
> 
> ---
> 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  | 123 +++++++++++++++++++++
>  tools/testing/selftests/bpf/progs/dev_cgroup.c     |   4 +-
>  tools/testing/selftests/bpf/test_dev_cgroup.c      |  85 --------------
>  5 files changed, 125 insertions(+), 90 deletions(-)
> ---
> base-commit: c90e2d4a7738a24fbb3657092dbd1ca20c040ed1
> change-id: 20240723-convert_dev_cgroup-6464b0d37f1a
> 
> Best regards,
> -- 
> Alexis Lothoré, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 

Going forward, can you pls use [PATCH bpf-next] as a subject (or bpf when
targeting bpf tree)? I'm not sure whether patchworks picks up
plain [PATCH] messages..

Re: [PATCH 2/3] selftests/bpf: convert test_dev_cgroup to test_progs

[Alexis Lothoré]

Hello Stanislas, thanks for the review

On 7/27/24 00:48, Stanislav Fomichev wrote:
> On 07/25, Alexis Lothoré (eBPF Foundation) wrote:

[...]

>> +	if (should_fail)
>> +		ASSERT_ERR(ret, "mknod");
>> +	else
>> +		ASSERT_OK(ret, "mknod");
> 
> Optional: might be easier to use something like expected_ret instead
> of should_fail and then do:
> 
> ASSERT_EQ(ret, expected_ret)

Yes, you are right. I initially went with a version relying on system() to
perform the mknods/dd calls, which could return different errors codes so I used
this should_fail. But while debugging some issues in CI with this series, I
realized that the needed commands are basic enough to be replaced with direct
library calls and I forgot to update this part, which can now assert an exact
return value. I will update this accordingly.

> I see this part being copy-pasted in a bunch of places below.
> 
>> +	unlink(path);
>> +}
>> +
>> +static void test_read(const char *path, int should_fail)
>> +{
>> +	char buf[TEST_BUFFER_SIZE];
>> +	int ret, fd;
>> +
>> +	fd = open(path, O_RDONLY);
>> +
>> +	/* A bare open on unauthorized device should fail */
>> +	if (should_fail) {
>> +		ASSERT_ERR(fd, "open file for read");
> 
> [..]
> 
>> +		if (fd)
>> +			close(fd);
> 
> nit: should this be 'if (fd >= 0)'? I'm assuming the intention is to
> avoid close(-1)?

Right as well, I'll fix it (here and below) in v2

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 0/3] selftests/bpf: convert test_dev_cgroup to test_progs

[Alexis Lothoré]

On 7/27/24 00:49, Stanislav Fomichev wrote:
> On 07/25, 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.
>>

[...]

> Going forward, can you pls use [PATCH bpf-next] as a subject (or bpf when
> targeting bpf tree)? I'm not sure whether patchworks picks up
> plain [PATCH] messages..

Yes, my bad, I realized some time after sending that I may have missed some
proper patch prefix. I have just checked on patchwork and see this series and
the one I have sent before, so I guess there is no need to resend those, but
I'll make sure to apply the relevant prefix for next series.

Thanks,

Alexis

-- 
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com