[PATCH 0/3] selftests/bpf: convert test_xdp_veth to test_progs framework

[PATCH 0/3] selftests/bpf: convert test_xdp_veth to test_progs framework

[Alexis Lothoré (eBPF Foundation)]

Hello everyone,

this small series is a first step in a larger effort aiming to help improve
eBPF selftests and the testing coverage in CI. It focuses for now on
test_xdp_veth.sh, a small test which is not integrated yet in test_progs.
The series is mostly about a rewrite of test_xdp_veth.sh to make it able to
run under test_progs, relying on libbpf to manipulate bpf programs involved
in the test.

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
Alexis Lothoré (eBPF Foundation) (3):
      selftests/bpf: update xdp_redirect_map prog sections for libbpf
      selftests/bpf: integrate test_xdp_veth into test_progs
      bpf/selftests: drop old version of test_xdp_veth.sh

 tools/testing/selftests/bpf/Makefile               |   1 -
 .../selftests/bpf/prog_tests/test_xdp_veth.c       | 234 +++++++++++++++++++++
 .../testing/selftests/bpf/progs/xdp_redirect_map.c |   6 +-
 tools/testing/selftests/bpf/test_xdp_veth.sh       | 121 -----------
 4 files changed, 237 insertions(+), 125 deletions(-)
---
base-commit: 4837cbaa1365cdb213b58577197c5b10f6e2aa81
change-id: 20240710-convert_test_xdp_veth-04cc05f5557d

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

[PATCH 1/3] selftests/bpf: update xdp_redirect_map prog sections for libbpf

[Alexis Lothoré (eBPF Foundation)]

xdp_redirect_map.c is a bpf program used by test_xdp_veth.sh, which is not
handled by the generic test runner (test_progs). To allow converting this
test to test_progs, the corresponding program must be updated to allow
handling it through skeletons generated by bpftool and libbpf.

Update programs section names to allow to manipulate those with libbpf.

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 tools/testing/selftests/bpf/progs/xdp_redirect_map.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/bpf/progs/xdp_redirect_map.c b/tools/testing/selftests/bpf/progs/xdp_redirect_map.c
index d037262c8937..682dda8dabbc 100644
--- a/tools/testing/selftests/bpf/progs/xdp_redirect_map.c
+++ b/tools/testing/selftests/bpf/progs/xdp_redirect_map.c
@@ -10,19 +10,19 @@ struct {
 	__uint(value_size, sizeof(int));
 } tx_port SEC(".maps");
 
-SEC("redirect_map_0")
+SEC("xdp")
 int xdp_redirect_map_0(struct xdp_md *xdp)
 {
 	return bpf_redirect_map(&tx_port, 0, 0);
 }
 
-SEC("redirect_map_1")
+SEC("xdp")
 int xdp_redirect_map_1(struct xdp_md *xdp)
 {
 	return bpf_redirect_map(&tx_port, 1, 0);
 }
 
-SEC("redirect_map_2")
+SEC("xdp")
 int xdp_redirect_map_2(struct xdp_md *xdp)
 {
 	return bpf_redirect_map(&tx_port, 2, 0);

-- 
2.45.2

[PATCH 2/3] selftests/bpf: integrate test_xdp_veth into test_progs

[Alexis Lothoré (eBPF Foundation)]

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@bootlin.com>
---
The new code has been tested in an aarch64 qemu instance:
Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED

I have also checked that some minor alterations in the network
configuration (altering the redirect map, or not loading one of the xdp
programs) make the test fail.

On my testing setup, the test takes a bit more than 3 seconds to run on
average.
---
 .../selftests/bpf/prog_tests/test_xdp_veth.c       | 234 +++++++++++++++++++++
 1 file changed, 234 insertions(+)

diff --git a/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c b/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
new file mode 100644
index 000000000000..40d85aece984
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
@@ -0,0 +1,234 @@
+// SPDX-License-Identifier: GPL-2.0
+/**
+ * Create 3 namespaces with 3 veth peers, and
+ * forward packets in-between using native XDP
+ *
+ *                      XDP_TX
+ * NS1(veth11)        NS2(veth22)        NS3(veth33)
+ *      |                  |                  |
+ *      |                  |                  |
+ *   (veth1,            (veth2,            (veth3,
+ *   id:111)            id:122)            id:133)
+ *     ^ |                ^ |                ^ |
+ *     | |  XDP_REDIRECT  | |  XDP_REDIRECT  | |
+ *     | ------------------ ------------------ |
+ *     -----------------------------------------
+ *                    XDP_REDIRECT
+ */
+
+#define _GNU_SOURCE
+#include <net/if.h>
+#include "test_progs.h"
+#include "network_helpers.h"
+#include "xdp_dummy.skel.h"
+#include "xdp_redirect_map.skel.h"
+#include "xdp_tx.skel.h"
+
+#define VETH_PAIRS_COUNT	3
+#define NS_NAME_MAX_LEN		16
+#define NS_SUFFIX_LEN		6
+#define VETH_NAME_MAX_LEN	16
+#define IP_SRC				"10.1.1.11"
+#define IP_DST				"10.1.1.33"
+#define IP_CMD_MAX_LEN		128
+
+struct skeletons {
+	struct xdp_dummy *xdp_dummy;
+	struct xdp_tx *xdp_tx;
+	struct xdp_redirect_map *xdp_redirect_maps;
+};
+
+struct veth_configuration {
+	char local_veth[VETH_NAME_MAX_LEN]; /* Interface in main namespace */
+	char remote_veth[VETH_NAME_MAX_LEN]; /* Peer interface in dedicated namespace*/
+	char namespace[NS_NAME_MAX_LEN]; /* Namespace for the remote veth */
+	char next_veth[VETH_NAME_MAX_LEN]; /* Local interface to redirect traffic to */
+	char *remote_addr; /* IP address of the remote veth */
+};
+
+static struct veth_configuration config[VETH_PAIRS_COUNT] = {
+	{
+		.local_veth = "veth1",
+		.remote_veth = "veth11",
+		.next_veth = "veth2",
+		.remote_addr = IP_SRC
+	},
+	{
+		.local_veth = "veth2",
+		.remote_veth = "veth22",
+		.next_veth = "veth3",
+		.remote_addr = NULL
+	},
+	{
+		.local_veth = "veth3",
+		.remote_veth = "veth33",
+		.next_veth = "veth1",
+		.remote_addr = IP_DST
+	}
+};
+
+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;
+}
+
+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) {
+		case 0:
+			local_prog = skeletons->xdp_redirect_maps->progs.xdp_redirect_map_0;
+			local_link = &skeletons->xdp_redirect_maps->links.xdp_redirect_map_0;
+			remote_prog = skeletons->xdp_dummy->progs.xdp_dummy_prog;
+			remote_link = &skeletons->xdp_dummy->links.xdp_dummy_prog;
+			break;
+		case 1:
+			local_prog = skeletons->xdp_redirect_maps->progs.xdp_redirect_map_1;
+			local_link = &skeletons->xdp_redirect_maps->links.xdp_redirect_map_1;
+			remote_prog = skeletons->xdp_tx->progs.xdp_tx;
+			remote_link = &skeletons->xdp_tx->links.xdp_tx;
+			break;
+		case 2:
+			local_prog = skeletons->xdp_redirect_maps->progs.xdp_redirect_map_2;
+			local_link = &skeletons->xdp_redirect_maps->links.xdp_redirect_map_2;
+			remote_prog = skeletons->xdp_dummy->progs.xdp_dummy_prog;
+			remote_link = &skeletons->xdp_dummy->links.xdp_dummy_prog;
+			break;
+	}
+	interface = if_nametoindex(config[index].local_veth);
+	if(!ASSERT_NEQ(interface, 0, "non zero interface index"))
+		return -1;
+	link = bpf_program__attach_xdp(local_prog, interface);
+	if (!ASSERT_OK_PTR(link, "attach xdp program to local veth"))
+		return -1;
+	*local_link = link;
+	nstoken = open_netns(config[index].namespace);
+	if (!ASSERT_OK_PTR(nstoken, "switch to remote veth namespace"))
+		return -1;
+	interface = if_nametoindex(config[index].remote_veth);
+	if(!ASSERT_NEQ(interface, 0, "non zero interface index"))
+		return -1;
+	link = bpf_program__attach_xdp(remote_prog, interface);
+	*remote_link = link;
+	close_netns(nstoken);
+	if (!ASSERT_OK_PTR(link, "attach xdp program to remote veth"))
+		return -1;
+
+	return 0;
+}
+
+static int configure_network(struct skeletons *skeletons) {
+	int interface_id;
+	int map_fd;
+	int err;
+	int i=0;
+
+	/* First create and configure all interfaces */
+	for(i=0; i<VETH_PAIRS_COUNT; i++) {
+		generate_random_ns_name(i+1, config[i].namespace);
+
+		SYS(fail, "ip netns add %s", config[i].namespace);
+		SYS(fail, "ip link add %s type veth peer name %s netns %s",
+				config[i].local_veth,
+				config[i].remote_veth,
+				config[i].namespace);
+		SYS(fail, "ip link set dev %s up", config[i].local_veth);
+		if (config[i].remote_addr)
+			SYS(fail, "ip -n %s addr add %s/24 dev %s",
+					   config[i].namespace, config[i].remote_addr, config[i].remote_veth);
+		SYS(fail, "ip -n %s link set dev %s up",
+				   config[i].namespace, config[i].remote_veth);
+	}
+
+	/* Then configure the redirect map and attach programs to interfaces */
+	map_fd = bpf_map__fd(skeletons->xdp_redirect_maps->maps.tx_port);
+	if (!ASSERT_GE(map_fd, 0, "open redirect map"))
+		goto fail;
+	for (i=0; i<VETH_PAIRS_COUNT; i++) {
+		interface_id = if_nametoindex(config[i].next_veth);
+		if(!ASSERT_NEQ(interface_id, 0, "non zero interface index"))
+			goto fail;
+		err = bpf_map_update_elem(map_fd, &i, &interface_id, BPF_ANY);
+		if (!ASSERT_OK(err, "configure interface redirection through map"))
+			goto fail;
+		if(attach_programs_to_veth_pair(skeletons, i))
+			goto fail;
+	}
+
+	return 0;
+
+fail:
+	return -1;
+}
+
+static void cleanup_network()
+{
+	char cmd[IP_CMD_MAX_LEN];
+	int i;
+
+	/* Deleting namespaces is enough to automatically remove veth pairs as well
+	 */
+	for(i=0; i<VETH_PAIRS_COUNT; i++) {
+		if(config[i].namespace[0] == 0)
+			continue;
+		snprintf(cmd, IP_CMD_MAX_LEN, "ip netns del %s", config[i].namespace);
+		system(cmd);
+	}
+}
+
+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);
+}
+
+void test_xdp_veth_redirect()
+{
+	struct skeletons skeletons = {};
+
+	skeletons.xdp_dummy = xdp_dummy__open_and_load();
+	if (!ASSERT_OK_PTR(skeletons.xdp_dummy, "xdp_dummy__open_and_load"))
+		return;
+
+	skeletons.xdp_tx = xdp_tx__open_and_load();
+	if (!ASSERT_OK_PTR(skeletons.xdp_tx, "xdp_tx__open_and_load"))
+		goto destroy_xdp_dummy;
+
+	skeletons.xdp_redirect_maps = xdp_redirect_map__open_and_load();
+	if (!ASSERT_OK_PTR(skeletons.xdp_redirect_maps, "xdp_redirect_map__open_and_load"))
+		goto destroy_xdp_tx;
+
+	if(configure_network(&skeletons))
+		goto destroy_xdp_redirect_map;
+
+	ASSERT_OK(check_ping(&skeletons), "ping");
+
+
+destroy_xdp_redirect_map:
+	xdp_redirect_map__destroy(skeletons.xdp_redirect_maps);
+destroy_xdp_tx:
+	xdp_tx__destroy(skeletons.xdp_tx);
+destroy_xdp_dummy:
+	xdp_dummy__destroy(skeletons.xdp_dummy);
+
+	cleanup_network();
+}

-- 
2.45.2

Re: [PATCH 2/3] selftests/bpf: integrate test_xdp_veth into test_progs

[Stanislav Fomichev]

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@bootlin.com>
> ---
> The new code has been tested in an aarch64 qemu instance:
> Summary: 1/0 PASSED, 0 SKIPPED, 0 FAILED
> 
> I have also checked that some minor alterations in the network
> configuration (altering the redirect map, or not loading one of the xdp
> programs) make the test fail.
> 
> On my testing setup, the test takes a bit more than 3 seconds to run on
> average.
> ---
>  .../selftests/bpf/prog_tests/test_xdp_veth.c       | 234 +++++++++++++++++++++
>  1 file changed, 234 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c b/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
> new file mode 100644
> index 000000000000..40d85aece984
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_xdp_veth.c
> @@ -0,0 +1,234 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * Create 3 namespaces with 3 veth peers, and
> + * forward packets in-between using native XDP
> + *
> + *                      XDP_TX
> + * NS1(veth11)        NS2(veth22)        NS3(veth33)
> + *      |                  |                  |
> + *      |                  |                  |
> + *   (veth1,            (veth2,            (veth3,
> + *   id:111)            id:122)            id:133)
> + *     ^ |                ^ |                ^ |
> + *     | |  XDP_REDIRECT  | |  XDP_REDIRECT  | |
> + *     | ------------------ ------------------ |
> + *     -----------------------------------------
> + *                    XDP_REDIRECT
> + */
> +
> +#define _GNU_SOURCE
> +#include <net/if.h>
> +#include "test_progs.h"
> +#include "network_helpers.h"
> +#include "xdp_dummy.skel.h"
> +#include "xdp_redirect_map.skel.h"
> +#include "xdp_tx.skel.h"
> +
> +#define VETH_PAIRS_COUNT	3
> +#define NS_NAME_MAX_LEN		16
> +#define NS_SUFFIX_LEN		6
> +#define VETH_NAME_MAX_LEN	16
> +#define IP_SRC				"10.1.1.11"
> +#define IP_DST				"10.1.1.33"
> +#define IP_CMD_MAX_LEN		128
> +
> +struct skeletons {
> +	struct xdp_dummy *xdp_dummy;
> +	struct xdp_tx *xdp_tx;
> +	struct xdp_redirect_map *xdp_redirect_maps;
> +};
> +
> +struct veth_configuration {
> +	char local_veth[VETH_NAME_MAX_LEN]; /* Interface in main namespace */
> +	char remote_veth[VETH_NAME_MAX_LEN]; /* Peer interface in dedicated namespace*/
> +	char namespace[NS_NAME_MAX_LEN]; /* Namespace for the remote veth */
> +	char next_veth[VETH_NAME_MAX_LEN]; /* Local interface to redirect traffic to */
> +	char *remote_addr; /* IP address of the remote veth */
> +};
> +
> +static struct veth_configuration config[VETH_PAIRS_COUNT] = {
> +	{
> +		.local_veth = "veth1",
> +		.remote_veth = "veth11",
> +		.next_veth = "veth2",
> +		.remote_addr = IP_SRC
> +	},
> +	{
> +		.local_veth = "veth2",
> +		.remote_veth = "veth22",
> +		.next_veth = "veth3",
> +		.remote_addr = NULL
> +	},
> +	{
> +		.local_veth = "veth3",
> +		.remote_veth = "veth33",
> +		.next_veth = "veth1",
> +		.remote_addr = IP_DST
> +	}
> +};

[..]

> +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? 

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

> +		case 0:
> +			local_prog = skeletons->xdp_redirect_maps->progs.xdp_redirect_map_0;
> +			local_link = &skeletons->xdp_redirect_maps->links.xdp_redirect_map_0;
> +			remote_prog = skeletons->xdp_dummy->progs.xdp_dummy_prog;
> +			remote_link = &skeletons->xdp_dummy->links.xdp_dummy_prog;
> +			break;
> +		case 1:
> +			local_prog = skeletons->xdp_redirect_maps->progs.xdp_redirect_map_1;
> +			local_link = &skeletons->xdp_redirect_maps->links.xdp_redirect_map_1;
> +			remote_prog = skeletons->xdp_tx->progs.xdp_tx;
> +			remote_link = &skeletons->xdp_tx->links.xdp_tx;
> +			break;
> +		case 2:
> +			local_prog = skeletons->xdp_redirect_maps->progs.xdp_redirect_map_2;
> +			local_link = &skeletons->xdp_redirect_maps->links.xdp_redirect_map_2;
> +			remote_prog = skeletons->xdp_dummy->progs.xdp_dummy_prog;
> +			remote_link = &skeletons->xdp_dummy->links.xdp_dummy_prog;
> +			break;
> +	}
> +	interface = if_nametoindex(config[index].local_veth);
> +	if(!ASSERT_NEQ(interface, 0, "non zero interface index"))
> +		return -1;
> +	link = bpf_program__attach_xdp(local_prog, interface);
> +	if (!ASSERT_OK_PTR(link, "attach xdp program to local veth"))
> +		return -1;
> +	*local_link = link;
> +	nstoken = open_netns(config[index].namespace);
> +	if (!ASSERT_OK_PTR(nstoken, "switch to remote veth namespace"))
> +		return -1;
> +	interface = if_nametoindex(config[index].remote_veth);
> +	if(!ASSERT_NEQ(interface, 0, "non zero interface index"))
> +		return -1;
> +	link = bpf_program__attach_xdp(remote_prog, interface);
> +	*remote_link = link;
> +	close_netns(nstoken);
> +	if (!ASSERT_OK_PTR(link, "attach xdp program to remote veth"))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int configure_network(struct skeletons *skeletons) {
> +	int interface_id;
> +	int map_fd;
> +	int err;
> +	int i=0;
> +
> +	/* First create and configure all interfaces */
> +	for(i=0; i<VETH_PAIRS_COUNT; i++) {
> +		generate_random_ns_name(i+1, config[i].namespace);
> +
> +		SYS(fail, "ip netns add %s", config[i].namespace);
> +		SYS(fail, "ip link add %s type veth peer name %s netns %s",
> +				config[i].local_veth,
> +				config[i].remote_veth,
> +				config[i].namespace);
> +		SYS(fail, "ip link set dev %s up", config[i].local_veth);
> +		if (config[i].remote_addr)
> +			SYS(fail, "ip -n %s addr add %s/24 dev %s",
> +					   config[i].namespace, config[i].remote_addr, config[i].remote_veth);
> +		SYS(fail, "ip -n %s link set dev %s up",
> +				   config[i].namespace, config[i].remote_veth);
> +	}
> +
> +	/* Then configure the redirect map and attach programs to interfaces */
> +	map_fd = bpf_map__fd(skeletons->xdp_redirect_maps->maps.tx_port);
> +	if (!ASSERT_GE(map_fd, 0, "open redirect map"))
> +		goto fail;
> +	for (i=0; i<VETH_PAIRS_COUNT; i++) {
> +		interface_id = if_nametoindex(config[i].next_veth);
> +		if(!ASSERT_NEQ(interface_id, 0, "non zero interface index"))
> +			goto fail;
> +		err = bpf_map_update_elem(map_fd, &i, &interface_id, BPF_ANY);
> +		if (!ASSERT_OK(err, "configure interface redirection through map"))
> +			goto fail;
> +		if(attach_programs_to_veth_pair(skeletons, i))
> +			goto fail;
> +	}
> +
> +	return 0;
> +
> +fail:
> +	return -1;
> +}
> +
> +static void cleanup_network()
> +{
> +	char cmd[IP_CMD_MAX_LEN];
> +	int i;
> +
> +	/* Deleting namespaces is enough to automatically remove veth pairs as well
> +	 */
> +	for(i=0; i<VETH_PAIRS_COUNT; i++) {
> +		if(config[i].namespace[0] == 0)
> +			continue;

[..]

> +		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?


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

Re: [PATCH 2/3] selftests/bpf: integrate test_xdp_veth into test_progs

[Alexis Lothoré]

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

[PATCH 3/3] bpf/selftests: drop old version of test_xdp_veth.sh

[Alexis Lothoré (eBPF Foundation)]

Now that test_xdp_veth has been rewritten to fit in the test_progs runner,
drop the shell script version

Signed-off-by: Alexis Lothoré <alexis.lothore@bootlin.com>
---
 tools/testing/selftests/bpf/Makefile         |   1 -
 tools/testing/selftests/bpf/test_xdp_veth.sh | 121 ---------------------------
 2 files changed, 122 deletions(-)

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index a7932bead77d..2864a0dc04d5 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -117,7 +117,6 @@ TEST_PROGS := test_kmod.sh \
 	test_xdp_redirect.sh \
 	test_xdp_redirect_multi.sh \
 	test_xdp_meta.sh \
-	test_xdp_veth.sh \
 	test_tunnel.sh \
 	test_lwt_seg6local.sh \
 	test_lirc_mode2.sh \
diff --git a/tools/testing/selftests/bpf/test_xdp_veth.sh b/tools/testing/selftests/bpf/test_xdp_veth.sh
deleted file mode 100755
index 5211ca9a0239..000000000000
--- a/tools/testing/selftests/bpf/test_xdp_veth.sh
+++ /dev/null
@@ -1,121 +0,0 @@
-#!/bin/sh
-# SPDX-License-Identifier: GPL-2.0
-#
-# Create 3 namespaces with 3 veth peers, and
-# forward packets in-between using native XDP
-#
-#                      XDP_TX
-# NS1(veth11)        NS2(veth22)        NS3(veth33)
-#      |                  |                  |
-#      |                  |                  |
-#   (veth1,            (veth2,            (veth3,
-#   id:111)            id:122)            id:133)
-#     ^ |                ^ |                ^ |
-#     | |  XDP_REDIRECT  | |  XDP_REDIRECT  | |
-#     | ------------------ ------------------ |
-#     -----------------------------------------
-#                    XDP_REDIRECT
-
-# Kselftest framework requirement - SKIP code is 4.
-ksft_skip=4
-
-TESTNAME=xdp_veth
-BPF_FS=$(awk '$3 == "bpf" {print $2; exit}' /proc/mounts)
-BPF_DIR=$BPF_FS/test_$TESTNAME
-readonly NS1="ns1-$(mktemp -u XXXXXX)"
-readonly NS2="ns2-$(mktemp -u XXXXXX)"
-readonly NS3="ns3-$(mktemp -u XXXXXX)"
-
-_cleanup()
-{
-	set +e
-	ip link del veth1 2> /dev/null
-	ip link del veth2 2> /dev/null
-	ip link del veth3 2> /dev/null
-	ip netns del ${NS1} 2> /dev/null
-	ip netns del ${NS2} 2> /dev/null
-	ip netns del ${NS3} 2> /dev/null
-	rm -rf $BPF_DIR 2> /dev/null
-}
-
-cleanup_skip()
-{
-	echo "selftests: $TESTNAME [SKIP]"
-	_cleanup
-
-	exit $ksft_skip
-}
-
-cleanup()
-{
-	if [ "$?" = 0 ]; then
-		echo "selftests: $TESTNAME [PASS]"
-	else
-		echo "selftests: $TESTNAME [FAILED]"
-	fi
-	_cleanup
-}
-
-if [ $(id -u) -ne 0 ]; then
-	echo "selftests: $TESTNAME [SKIP] Need root privileges"
-	exit $ksft_skip
-fi
-
-if ! ip link set dev lo xdp off > /dev/null 2>&1; then
-	echo "selftests: $TESTNAME [SKIP] Could not run test without the ip xdp support"
-	exit $ksft_skip
-fi
-
-if [ -z "$BPF_FS" ]; then
-	echo "selftests: $TESTNAME [SKIP] Could not run test without bpffs mounted"
-	exit $ksft_skip
-fi
-
-if ! bpftool version > /dev/null 2>&1; then
-	echo "selftests: $TESTNAME [SKIP] Could not run test without bpftool"
-	exit $ksft_skip
-fi
-
-set -e
-
-trap cleanup_skip EXIT
-
-ip netns add ${NS1}
-ip netns add ${NS2}
-ip netns add ${NS3}
-
-ip link add veth1 index 111 type veth peer name veth11 netns ${NS1}
-ip link add veth2 index 122 type veth peer name veth22 netns ${NS2}
-ip link add veth3 index 133 type veth peer name veth33 netns ${NS3}
-
-ip link set veth1 up
-ip link set veth2 up
-ip link set veth3 up
-
-ip -n ${NS1} addr add 10.1.1.11/24 dev veth11
-ip -n ${NS3} addr add 10.1.1.33/24 dev veth33
-
-ip -n ${NS1} link set dev veth11 up
-ip -n ${NS2} link set dev veth22 up
-ip -n ${NS3} link set dev veth33 up
-
-mkdir $BPF_DIR
-bpftool prog loadall \
-	xdp_redirect_map.bpf.o $BPF_DIR/progs type xdp \
-	pinmaps $BPF_DIR/maps
-bpftool map update pinned $BPF_DIR/maps/tx_port key 0 0 0 0 value 122 0 0 0
-bpftool map update pinned $BPF_DIR/maps/tx_port key 1 0 0 0 value 133 0 0 0
-bpftool map update pinned $BPF_DIR/maps/tx_port key 2 0 0 0 value 111 0 0 0
-ip link set dev veth1 xdp pinned $BPF_DIR/progs/xdp_redirect_map_0
-ip link set dev veth2 xdp pinned $BPF_DIR/progs/xdp_redirect_map_1
-ip link set dev veth3 xdp pinned $BPF_DIR/progs/xdp_redirect_map_2
-
-ip -n ${NS1} link set dev veth11 xdp obj xdp_dummy.bpf.o sec xdp
-ip -n ${NS2} link set dev veth22 xdp obj xdp_tx.bpf.o sec xdp
-ip -n ${NS3} link set dev veth33 xdp obj xdp_dummy.bpf.o sec xdp
-
-trap cleanup EXIT
-
-ip netns exec ${NS1} ping -c 1 -W 1 10.1.1.33
-
-exit 0

-- 
2.45.2