Re: [PATCH bpf-next] selftests/bpf: convert test_xdp_features.sh to test_progs

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi Alexis,

On Mon, 09 Sep 2024 22:02:07 +0200
Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com> wrote:

> test_xdp_features.sh is a shell script allowing to test that xdp features
> advertised by an interface are indeed delivered. The test works by starting
> two instance of the same program, both attaching specific xdp programs to
> each side of a veth link, and then make those programs manage packets and
> collect stats to check whether tested XDP feature is indeed delivered or
> not. However this test is not integrated in test_progs framework and so can
> not run automatically in CI.
> 
> Rewrite test_xdp_features to integrate it in test_progs so it can run
> automatically in CI. The main changes brought by the rewrite are the
> following:
> - instead of running to separated processes (each one managing either the
>   tester veth or the DUT vet), run a single process
> - slightly change testing direction (v0 is the tester in local namespace,
>   v1 is the Device Under Test in remote namespace)
> - group all tests previously managed by test_xdp_features as subtests (one
>   per tested XDP feature). As a consequence, run only once some steps
>   instead of once per subtest (eg: starting/stopping the udp server). On
>   the contrary, make sure that each subtest properly cleans up its state
>   (ie detach xdp programs, reset test stats, etc)
> - since there is now a single process, get rid of the "control" tcp channel
>   used to configure DUT. Configuring the DUT now only consists in switching
>   to DUT network namespace and run the relevant commands
> - since there is no more control channel, get rid of TLVs, keep only the
>   CMD_ECHO packet type, and set it as a magic
> - simplify network setup: use only ipv6 instead of both ipv4 and ipv6,
>   force static neighbours instead of waiting for autoconfiguration, do not
>   force gro (fetch xdp features only once xdp programs are loaded instead)
> 
> The existing XDP programs are reused, with some minor changes:
> - tester and dut stats maps are converted to global variables for easier
>   usage
> - programs do not use TLV struct anymore but the magic replacing the echo
>   command
> - avoid to accidentally make tests pass: drop packets instead of forwarding
>   them to userspace when they do not match the expected payload
> 
> Signed-off-by: Alexis Lothoré (eBPF Foundation) <alexis.lothore@bootlin.com>

I'm far for having reviewed the whole patch, but I do have one comment
below :)

[...]

> +static void *run_dut_echo_thread(void *arg)
> +{
> +	struct test_data *t = (struct test_data *)arg;
> +	__u32 magic;
> +
> +	while (!t->quit_dut_echo_thread) {
> +		struct sockaddr_storage addr;
> +		socklen_t addrlen;
> +		size_t n;
> +
> +		n = recvfrom(t->echo_server_sock, &magic, sizeof(magic),
> +			     MSG_WAITALL, (struct sockaddr *)&addr, &addrlen);
> +		if (n != sizeof(magic)) {
> +			usleep(LOOP_DELAY_US);
> +			continue;
> +		}
> +
> +		if (htonl(magic) != CMD_ECHO)
> +			continue;

Shouldn't it be ntohl here ? The former code used the ntohs helper for
that command, and you're sending the magic in send_echo_msg with a
htonl() so I guess here you might want to convert the value back to
host endianness.

Thanks,

Maxime