Skip to content

Conversation

@kernel-patches-daemon-bpf-rc
Copy link

Pull request for series with
subject: selftests/bpf: convert test_tc_tunnel.sh to test_progs
version: 1
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012903

@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 48a97ff
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012903
version: 1

@kernel-patches-review-bot
Copy link

commit 4ae5a37cdd34256a1ebc8a04f8fae87778c5d921
Author: Alexis Lothoré (eBPF Foundation) <[email protected]>

selftests/bpf: integrate test_tc_tunnel.sh tests into test_progs

This commit integrates the test_tc_tunnel.sh script into the test_progs
framework by creating a new test file that validates various tunneling
mechanisms. The test checks that BPF programs can handle encapsulation and
decapsulation for multiple tunnel types.

> diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_tunnel.c b/tools/testing/selftests/bpf/prog_tests/test_tc_tunnel.c
> new file mode 100644
> index 000000000..294cf4fb1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/test_tc_tunnel.c

[ ... ]

> +static int run_server(struct subtest_cfg *cfg)
> +{
> +	struct nstoken *nstoken = open_netns(SERVER_NS);
> +	int family = cfg->ipproto == 6 ? AF_INET6 : AF_INET;
> +
> +	cfg->server_fd = start_reuseport_server(family, SOCK_STREAM,
> +						cfg->server_addr, TEST_PORT,
> +						TIMEOUT_MS, 1);
> +	close_netns(nstoken);
> +	if (!ASSERT_NEQ(cfg->server_fd, NULL, "start server"))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static void stop_server(struct subtest_cfg *cfg)
> +{
> +	struct nstoken *nstoken = open_netns(SERVER_NS);
> +
> +	close(*cfg->server_fd);
> +	cfg->server_fd = NULL;
> +	close_netns(nstoken);
> +}

Can stop_server() dereference a NULL pointer if run_server() failed?
Looking at run_test(), if run_server() returns -1, the code jumps to
the fail label which unconditionally calls stop_server(). But when
run_server() fails, cfg->server_fd is never set and remains at
whatever value it had (likely uninitialized or from a previous test).

Also, does this code leak the server_fd array? The start_reuseport_server()
function allocates an array of file descriptors using malloc() and returns
the pointer. The matching cleanup function is free_fds() which both closes
all the file descriptors and frees the allocated array. Here in
stop_server(), only close() is called on the first file descriptor, but
the array itself is never freed with free_fds().

> +static void run_test(struct subtest_cfg *cfg)
> +{
> +	if (!ASSERT_OK(run_server(cfg), "run server"))
> +		goto fail;
> +
> +	// Basic communication must work
> +	if (!ASSERT_OK(send_and_test_data(cfg, true), "connect without any encap"))
> +		goto fail;
> +
> +	// Attach encapsulation program to client, communication must fail
> +	if (!ASSERT_OK(configure_encapsulation(cfg), "configure encapsulation"))
> +		return;
> +	if (!ASSERT_OK(send_and_test_data(cfg, false), "connect with encap prog only"))
> +		goto fail;
> +
> +	/* Insert kernel decap module, connection must succeed */
> +	if (!ASSERT_OK(configure_kernel_decapsulation(cfg), "configure kernel decapsulation"))
> +		goto fail;
> +	if (!ASSERT_OK(send_and_test_data(cfg, !cfg->expect_kern_decap_failure),
> +		       "connect with encap prog and kern decap"))
> +		goto fail;
> +
> +	// Replace kernel module with BPF decap, test must pass
> +	if (!ASSERT_OK(configure_ebpf_decapsulation(cfg), "configure ebpf decapsulation"))
> +		goto fail;
> +	ASSERT_OK(send_and_test_data(cfg, true), "connect with encap and decap progs");
> +
> +fail:
> +	stop_server(cfg);
> +}

[ ... ]



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: selftests/bpf: integrate test_tc_tunnel.sh tests into test_progs
CI run summary: https://github.com/kernel-patches/bpf-rc/actions/runs/18596012432

@kernel-patches-daemon-bpf-rc
Copy link
Author

Forwarding comment 3415900283 via email
In-Reply-To: [email protected]
Patch: https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

The test_tunnel.c file  defines some convenient functions to create a qdisc
and attach bpf programs to it. It would be beneficial to new tests to
get those functions exported as helpers in a tc_helpers.h. There is
however already a tc_helpers.h file in the BPF selftests, used by a few
tests. This header is quite coupled to some BPF programs (some exposed
helpers consume a skeleton from some specific BPF programs), so rename
it to something more related to its original purpose.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <[email protected]>
The test_tunnel.c file defines small fonctions to easily attach eBPF
programs to tc hooks, either on egress, ingress or both.

Move those helpers in a dedicated file so that other tests can benefit
from it.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <[email protected]>
@kernel-patches-daemon-bpf-rc
Copy link
Author

Upstream branch: 48a97ff
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1012903
version: 1

…latforms

When trying to run bpf-based encapsulation in a s390x environment, some
parts of test_tc_tunnel.bpf.o do not encapsulate correctly the traffic,
leading to tests failures. Adding some logs shows for example that
packets about to be sent on an interface with the ip6vxlan_eth program
attached do not have the expected value 5 in the ip header ihl field,
and so are ignored by the program.

This phenomenon appears when trying to cross-compile the selftests,
rather than compiling it from a virtualized host: the selftests build
system may then wrongly pick some host headers. If <asm/byteorder.h>
ends up being picked on the host (and if the host has a endianness
different from the target one), it will then expose wrong endianness
defines (e.g __LITTLE_ENDIAN_BITFIELD instead of __BIT_ENDIAN_BITFIELD),
and it will for example mess up the iphdr structure layout used in the
ebpf program.

To prevent this, directly use the vmlinux.h header generated by the
selftests build system rather than including directly specific kernel
headers. As a consequence, add some missing definitions that are not
exposed by vmlinux.h, and adapt the bitfield manipulations to allow
building and using the program on both types of platforms.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <[email protected]>
The test_tc_tunnel.sh script checks that a large variety of tunneling
mechanisms handled by the kernel can be handled as well by eBPF
programs. While this test shares similarities with test_tunnel.c (which
is already integrated in test_progs), those are testing slightly
different things:
- test_tunnel.c creates a tunnel interface, and then get and set tunnel
  keys in packet metadata, from BPF programs.
- test_tc_tunnels.sh manually parses/crafts packets content

Bring the tests covered by test_tc_tunnel.sh into the test_progs
framework, by creating a dedicated test_tc_tunnel.sh. This new test
defines a "generic" runner which, for each test configuration:
- will bring the relevant veth pair, each of those isolated in a
  dedicated namespace
- will check that traffic will fail if there is only an encapsulating
  program attached to one veth egress
- will check that traffic succeed if we enable some decapsulation module
  on kernel side
- will check that traffic still succeeds if we replace the kernel
  decapsulation with some eBPF ingress decapsulation.

Example of the new test execution:

  # ./test_progs -a tc_tunnel
  #447/1   tc_tunnel/ipip_none:OK
  #447/2   tc_tunnel/ipip6_none:OK
  #447/3   tc_tunnel/ip6tnl_none:OK
  #447/4   tc_tunnel/sit_none:OK
  #447/5   tc_tunnel/vxlan_eth:OK
  #447/6   tc_tunnel/ip6vxlan_eth:OK
  #447/7   tc_tunnel/gre_none:OK
  #447/8   tc_tunnel/gre_eth:OK
  #447/9   tc_tunnel/gre_mpls:OK
  #447/10  tc_tunnel/ip6gre_none:OK
  #447/11  tc_tunnel/ip6gre_eth:OK
  #447/12  tc_tunnel/ip6gre_mpls:OK
  #447/13  tc_tunnel/udp_none:OK
  #447/14  tc_tunnel/udp_eth:OK
  #447/15  tc_tunnel/udp_mpls:OK
  #447/16  tc_tunnel/ip6udp_none:OK
  #447/17  tc_tunnel/ip6udp_eth:OK
  #447/18  tc_tunnel/ip6udp_mpls:OK
  #447     tc_tunnel:OK
  Summary: 1/18 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Alexis Lothoré (eBPF Foundation) <[email protected]>
Now that test_tc_tunnel.sh scope has been ported to the test_progs
framework, remove it.

Signed-off-by: Alexis Lothoré (eBPF Foundation) <[email protected]>
@kernel-patches-daemon-bpf-rc
Copy link
Author

At least one diff in series https://patchwork.kernel.org/project/netdevbpf/list/?series=1012903 expired. Closing PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants