linux-kernel - Re: [PATCH 2/3] selftests/bpf: integrate test_xdp_veth into 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] [day] [month] [year] [list]

Message-ID: <48581ce1-f141-46ec-86ac-88092e00b967@bootlin.com>
Date: Mon, 15 Jul 2024 09:42:20 +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>, "David S. Miller"
 <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
 Jesper Dangaard Brouer <hawk@...nel.org>,
 John Fastabend <john.fastabend@...il.com>,
 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>,
 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,
 netdev@...r.kernel.org, bpf@...r.kernel.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/3] selftests/bpf: integrate test_xdp_veth into
 test_progs

Hello Stanislas, thanks for the review

On 7/12/24 03:10, Stanislav Fomichev wrote:
> On 07/11, Alexis Lothoré (eBPF Foundation) wrote:
>> test_xdp_veth.sh tests that XDP return codes work as expected, by bringing
>> up multiple veth pairs isolated in different namespaces, attaching specific
>> xdp programs to each interface, and ensuring that the whole chain allows to
>> ping one end interface from the first one. The test runs well but is
>> currently not integrated in test_progs, which prevents it from being run
>> automatically in the CI infrastructure.
>>
>> Rewrite it as a C test relying on libbpf to allow running it in the CI
>> infrastructure. The new code brings up the same network infrastructure and
>> reuses the same eBPF programs as test_xdp_veth.sh, for which skeletons are
>> already generated by the bpf tests makefile.
>>
>> Signed-off-by: Alexis Lothoré <alexis.lothore@...tlin.com>

[...]

>> +static void generate_random_ns_name(int index, char *out)
>> +{
>> +	int random, count, i;
>> +
>> +	count = snprintf(out, NS_NAME_MAX_LEN, "ns%d-", index);
>> +	for(i=0; i<NS_SUFFIX_LEN; i++) {
>> +		random=rand() % 2;
>> +		out[count++]= random ? 'a' + rand() % 26 : 'A' + rand() % 26;
>> +	}
>> +	out[count] = 0;
>> +}
> 
> It's been customary to hard-code netns names for all the tests we have, so
> maybe it's ok here as well?

I indeed wondered if it was really useful to bring this random ns name mechanism
from the shell script, but I saw that it has been brought by the dedicated
commit 9d66c9ddc9fc ("selftests/bpf/test_xdp_veth: use temp netns for testing"),
so I assumed that some real issues about static ns names were encountered and
led to this fix. Maybe it is indeed enough if I hardcode ns names but not with a
too generic prefix ?

> 
>> +static int attach_programs_to_veth_pair(struct skeletons *skeletons, int index)
>> +{
>> +	struct bpf_program *local_prog, *remote_prog;
>> +	struct bpf_link **local_link, **remote_link;
>> +	struct nstoken *nstoken;
>> +	struct bpf_link *link;
>> +	int interface;
>> +
> 
> [..]
> 
>> +	switch(index) {
> 
> Can you pls run the patch through the checkpatch.pl? The formatting
> looks wrong, should be 'switch (index)'. Applies to 'if()' elsewhere as
> well.

Crap, I forgot this very basic part, my bad, I'll fix all those small issues.

> [..]
> 
>> +		snprintf(cmd, IP_CMD_MAX_LEN, "ip netns del %s", config[i].namespace);
>> +		system(cmd);
> 
> SYS_NOFAIL to avoid separate snprintf?

[...]

>> +static int check_ping(struct skeletons *skeletons)
>> +{
>> +	char cmd[IP_CMD_MAX_LEN];
>> +
>> +	/* Test: if all interfaces are properly configured, we must be able to ping
>> +	 * veth33 from veth11
>> +	 */
>> +	snprintf(cmd, IP_CMD_MAX_LEN,
>> +			 "ip netns exec %s ping -c 1 -W 1 %s > /dev/null",
>> +			 config[0].namespace, IP_DST);
>> +	return system(cmd);
> 
> SYS_NOFAL here as well?

Thanks for the tip, I'll use this macro.
> 
> Btw, not sure it makes sense to split that work into 3 patches. After
> you first patch the test is broken anyway, so might as well just delete
> the script at that point...

I have made sure that the sh script still runs correctly even after renaming the
sections in the xdp program. But indeed, maybe I can squash the new test patch
and the shell scrip deletion patch.

Thanks,

Alexis

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