linux-kernel - Re: [PATCH bpf-next 2/4] selftests/bpf: convert test_cgroup_storage 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: <40683bcd-14df-40b6-9110-ce5c61e543af@bootlin.com>
Date: Thu, 1 Aug 2024 11:21:11 +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>, linux-kernel@...r.kernel.org,
 bpf@...r.kernel.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next 2/4] selftests/bpf: convert test_cgroup_storage
 to test_progs

On 8/1/24 10:27, Alan Maguire wrote:
> On 31/07/2024 11:38, Alexis Lothoré (eBPF Foundation) wrote:
>> test_cgroup_storage is currently a standalone program which is not run
>> when executing test_progs.
>>
>> Convert it to the test_progs framework so it can be automatically executed
>> in CI. The conversion led to the following changes:
>> - converted the raw bpf program in the userspace test file into a dedicated
>>   test program in progs/ dir
>> - reduced the scope of cgroup_storage test: the content from this test
>>   overlaps with some other tests already present in test_progs, most
>>   notably netcnt and cgroup_storage_multi*. Those tests already check
>>   extensively local storage, per-cpu local storage, cgroups interaction,
>>   etc. So the new test only keep the part testing that the program return
>>   code (based on map content) properly leads to packet being passed or
>>   dropped.
>>
>> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@...tlin.com>
> 
> Two small things below, but
> 
> Reviewed-by: Alan Maguire <alan.maguire@...cle.com>

[...]

>> +#define PING_CMD "ping localhost -c 1 -W 1 -q"
> 
> other tests seem to redirect ping stdout output to /dev/null ; might be
> worth doing that too.

That's in fact performed automatically by SYS_NOFAIL :)

#define SYS_NOFAIL(fmt, ...)						\
	({								\
		char cmd[1024];						\
		int n;							\
		n = snprintf(cmd, sizeof(cmd), fmt, ##__VA_ARGS__);	\
		if (n < sizeof(cmd) && sizeof(cmd) - n >= sizeof(ALL_TO_DEV_NULL)) \
			strcat(cmd, ALL_TO_DEV_NULL);			\
		system(cmd);						\
	})

[...]

>> +{
>> +	__u64 *counter;
>> +
>> +	counter = bpf_get_local_storage(&cgroup_storage, 0);
> 
> don't we need a NULL check for counter here? Or does the verifier know
> bpf_get_local_storage never fails?

Good question. Since the verifier accepted the prog during my tests, I indeed
assume that the returned pointer is always valid. Amongst all calls to this
function in progs involved in selftests, I found only one performing a check
before using the value (lsm_cgroup.c). So I guess it is fine ?

Thanks for the review !

Alexis

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