Re: [PATCH net-next v6 1/5] selftests/bpf: Fix XDP feature test setup and teardown

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The `xdp_attached` guard only records that an attach succeeded at
  some point.

Pre-existing issues:
- [Medium] test_xdp_features.sh uses only the tester's exit status, and
  tester_run() returns 0 whether or not the capability is DETECTED or…
- [Low] In tester_run(), the settimeo() and connect() failure paths
  return directly without close(sockfd), which leaks the control socket.
- [Low] recv_msg() accepts payloads shorter than val_size and copies only
  the bytes it received.

--

selftests/bpf: Fix XDP feature test setup and teardown

This commit fixes several setup and teardown problems in the xdp_features
selftest tool. It initializes the DUT state and socket address lengths,
hands the echo socket back to the caller, and stops and joins the echo
thread only if it was started. It also samples the tester counters before
teardown, and it detaches XDP programs only after a successful attach.

> diff --git a/tools/testing/selftests/bpf/xdp_features.c b/tools/testing/selftests/bpf/xdp_features.c
> index a27ed663967c4..3076fdda61fa4 100644
> --- a/tools/testing/selftests/bpf/xdp_features.c
> +++ b/tools/testing/selftests/bpf/xdp_features.c

[ ... ]

> @@ -341,10 +343,14 @@ static int recv_msg(int sockfd, void *buf, size_t bufsize, void *val,
>  		    size_t val_size)
>  {
>  	struct tlv_hdr *tlv = (struct tlv_hdr *)buf;
> -	size_t len;
> +	ssize_t len;
>  
>  	len = recv(sockfd, buf, bufsize, 0);
> -	if (len != ntohs(tlv->len) || len < sizeof(*tlv))
> +	if (len < 0)
> +		return -errno;
> +	if (!len)
> +		return -ECONNRESET;
> +	if (len < sizeof(*tlv) || len != ntohs(tlv->len))
>  		return -EINVAL;
>  
>  	if (val) {

[Severity: Low]
This isn't a bug introduced by this patch, but recv_msg() only rejects
payloads that are larger than val_size:

	if (val) {
		len -= sizeof(*tlv);
		if (len > val_size)
			return -ENOMEM;

		memcpy(val, tlv->data, len);
	}

Can a short payload leave part of the destination unwritten?

In tester_run(), stats and advertised_feature are both declared without
an initial value. Leftover stack bytes could then affect the feature
result that gets printed.

A later patch in this series changes the check to:

	if (len != val_size)
		return -ENOMEM;

That rejects short payloads before the memcpy().

[ ... ]

> @@ -403,9 +412,12 @@ static int dut_run(struct xdp_features *skel)
>  			if (err)
>  				goto out;
>  
> +			xdp_attached = true;
> +
>  			err = dut_run_echo_thread(&dut_thread, &echo_sockfd);

[ ... ]

> @@ -422,6 +434,7 @@ static int dut_run(struct xdp_features *skel)
>  
>  			exiting = true;
>  			bpf_xdp_detach(env.ifindex, flags, NULL);
> +			xdp_attached = false;

[Severity: Low]
The xdp_attached flag records that bpf_xdp_attach() succeeded at some
point. However, bpf_xdp_detach(env.ifindex, flags, NULL) removes
whatever program is attached to the device at the time it runs.

If another privileged user replaces the program between the attach and
the detach, would this still remove that other program?

The out: label in tester_run() uses the same pattern:

	if (xdp_attached)
		bpf_xdp_detach(env.ifindex, flags, NULL);

A later patch in this series appears to fix this. The DUT attaches with
bpf_link_create() and cleans up with close(link_fd). The tester no
longer attaches an XDP program at all, because it uses AF_PACKET.

>  
>  			tlv->type = htons(CMD_ACK);
>  			tlv->len = htons(sizeof(*tlv));

[ ... ]

> @@ -578,7 +596,7 @@ static int tester_run(struct xdp_features *skel)
>  	struct bpf_program *prog;
>  	unsigned int stats;
>  	int i, err, sockfd;
> -	bool detected_cap;
> +	bool detected_cap, xdp_attached = false;
>  
>  	sockfd = socket(AF_INET6, SOCK_STREAM, 0);
>  	if (sockfd < 0) {

[Severity: Low]
This is a pre-existing issue, but the settimeo() and connect() failure
paths just below this return without closing sockfd:

	if (settimeo(sockfd, 1000) < 0)
		return -EINVAL;

	err = connect(sockfd, (struct sockaddr *)&env.dut_ctrl_addr,
		      sizeof(env.dut_ctrl_addr));
	if (err) {
		...
		return -errno;
	}

Does this leak the control socket? Now that xdp_attached starts out
false, could these paths go to the out: label instead?

A later patch in this series changes them to "err = -errno; goto out;".

[ ... ]

> @@ -636,19 +655,21 @@ static int tester_run(struct xdp_features *skel)
>  	if (err)
>  		goto out;
>  
> +	/* Sample before teardown, which can produce additional echo traffic. */
> +	detected_cap = tester_collect_detected_cap(skel, ntohl(stats));
> +
>  	/* stop the test */

[ ... ]

>  	fprintf(stdout, "Feature %s: [%s][%s]\n", get_xdp_feature_str(),
>  		detected_cap ? GREEN("DETECTED") : RED("NOT DETECTED"),
>  		env.feature.drv_feature & advertised_feature ? GREEN("ADVERTISED")
>  							     : RED("NOT ADVERTISED"));
>  out:
> -	bpf_xdp_detach(env.ifindex, flags, NULL);
> +	if (xdp_attached)
> +		bpf_xdp_detach(env.ifindex, flags, NULL);
>  	close(sockfd);
>  	return err < 0 ? err : 0;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but detected_cap and
advertised_feature are only printed here. tester_run() returns 0
whatever their values are.

test_xdp_features.sh checks only the exit status of the tester:

	ip netns exec ${NS} ./xdp_features -t -f XDP_PASS \
					   -D $V1_IP6 -C $V1_IP6 \
					   -T $V0_IP6 v0
	[ $? -ne 0 ] && exit

Could a broken or unadvertised XDP action still pass the selftest?

A later patch in this series ("selftests/bpf: Track XDP test processes
and isolate namespaces") adds:

	if (!err && detected_cap != !!(env.feature.drv_feature & advertised_feature))
		err = -EINVAL;

The same series also replaces the shell script with
drivers/net/hw/xdp_features.py.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261018-xdp-v5-send-20261008-v6-0-7336e1282ca3%40gmail.com