linux-kernel - Re: [PATCH bpf-next v2 2/3] selftests/bpf: convert test_dev_cgroup to test

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <178ff26a-7068-41f1-a0db-dbd0749aa82f@oracle.com>
Date: Mon, 29 Jul 2024 19:15:04 +0100
From: Alan Maguire <alan.maguire@...cle.com>
To: Alexis Lothoré <alexis.lothore@...tlin.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

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@...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.
>

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