[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87d70267-0305-4f4d-a7e2-7d1f8855e14c@bootlin.com>
Date: Mon, 29 Jul 2024 19:47:05 +0200
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: Alan Maguire <alan.maguire@...cle.com>,
Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann <daniel@...earbox.net>,
Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau
<martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>,
Song Liu <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>,
John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, Mykola Lysenko <mykolal@...com>,
Shuah Khan <shuah@...nel.org>
Cc: ebpf@...uxfoundation.org, Thomas Petazzoni
<thomas.petazzoni@...tlin.com>, bpf@...r.kernel.org,
linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 2/3] selftests/bpf: convert test_dev_cgroup to
test_progs
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@...tlin.com>
>
> A few small suggestions but looks great!
>
> Reviewed-by: Alan Maguire <alan.maguire@...cle.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
--
Alexis Lothoré, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Powered by blists - more mailing lists