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

[Martin KaFai Lau <martin.lau@linux.dev>]

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();
> +}