linux-kernel - Re: [PATCH 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: <e42a3622-c0a4-4cfd-994e-2261662c1cfd@bootlin.com>
Date: Sat, 27 Jul 2024 09:58:35 +0200
From: Alexis Lothoré <alexis.lothore@...tlin.com>
To: Stanislav Fomichev <sdf@...ichev.me>
Cc: 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>,
 Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
 Mykola Lysenko <mykolal@...com>, Shuah Khan <shuah@...nel.org>,
 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 2/3] selftests/bpf: convert test_dev_cgroup to test_progs

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