Re: [PATCH mptcp-next 2/2] selftests: mptcp: add mptcp nbd tests

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] selftests: mptcp: run_io_test() returns 0 on fallback or missing subflows
- [Medium] selftests: mptcp: missing prerequisite checks cause failure instead of skip
- [Medium] selftests: mptcp: temp files leaked in cleanup() due to subshell assignment
- [Medium] selftests: mptcp: host-wide pkill/pgrep and /dev/nbd0 clobber in cleanup()
--

commit 3361c31ea4cfbf255e2c3ce5c5e70d2193d46dd4
Author: Geliang Tang <geliang@kernel.org>

selftests: mptcp: add mptcp nbd tests

This commit adds the mptcp_nbd.sh selftest to verify Network Block Device
(NBD) functionality and throughput over both TCP and MPTCP across multiple
virtual network interfaces.

> diff --git a/tools/testing/selftests/net/mptcp/mptcp_nbd.sh b/tools/testing/selftests/net/mptcp/mptcp_nbd.sh
> new file mode 100755
> index 0000000000000..935dea5738aa1
> --- /dev/null
> +++ b/tools/testing/selftests/net/mptcp/mptcp_nbd.sh
> @@ -0,0 +1,309 @@
[ ... ]
> +cleanup()
> +{
> +	# Disconnect nbd device in ns2
> +	ip netns exec "$ns2" nbd-client -d /dev/nbd0 2>/dev/null || true
> +
> +	sleep 1
> +
> +	# Kill nbd-server in ns1
> +	if [ -n "${server_pid}" ]; then
> +		ip netns exec "$ns1" kill "${server_pid}" 2>/dev/null || true
> +	fi
> +	ip netns exec "$ns1" pkill -9 -f nbd-server 2>/dev/null || true

[Severity: Medium]
Since ip netns exec only switches the network namespace while sharing the
host PID namespace, won't running pkill -9 -f nbd-server in cleanup() kill
all nbd-server processes across the entire host system, and won't pgrep -f
"nbd-server" in run_test() match unrelated host processes as well?

Also, /dev/nbd0 is a host-global block device that is not isolated by
network namespaces. Could run_client(), run_io_test(), and cleanup()
clobber or disconnect /dev/nbd0 if it is already in use on the host?

> +
> +	# Clean up temp files
> +	if [ -n "${temp_file}" ]; then
> +		ip netns exec "$ns1" rm -f "${temp_file}" 2>/dev/null || true
> +	fi

[Severity: Medium]
Does cleanup() leak /tmp/nbd_backing.raw and /tmp/nbd-server.conf on every
run?

In run_test(), run_server() is executed inside a child bash process:

	if ! ip netns exec "$ns1" bash <<- EOF
		$(declare -f run_server)
		export trtype port
		run_server
		exit \$?
	EOF

Because temp_file="/tmp/nbd_backing.raw" is assigned inside that child
process, temp_file in the parent shell remains empty when the EXIT trap
invokes cleanup(). In addition, conf_file="/tmp/nbd-server.conf" is a local
variable in run_server() with no removal in cleanup(), and both use
hardcoded /tmp paths instead of mktemp.

[ ... ]
> +run_server()
> +{
> +	local conf_file="/tmp/nbd-server.conf"
> +
> +	# Create backing file
> +	temp_file="/tmp/nbd_backing.raw"
> +	dd if=/dev/zero of="${temp_file}" bs=1M count=0 seek=256 2>/dev/null
> +
> +	# Detect MPTCP support: native config or mptcpize fallback
> +	local use_mptcpize=""
> +	local mptcp_opt=""
> +	if [ "${trtype}" = "mptcp" ]; then
> +		if ! grep -q '"mptcp"' /usr/bin/nbd-server 2>/dev/null && \
> +		   ! nbd-server --help 2>&1 | grep -qi mptcp; then
> +			if command -v mptcpize &> /dev/null; then
> +				use_mptcpize="mptcpize run -d"
> +				echo "INFO: nbd-server lacks native MPTCP," \
> +					"using mptcpize"
> +			else
> +				echo "ERROR: no MPTCP support in nbd-server"
> +				return 1
> +			fi

[Severity: Medium]
Should the test skip with KSFT_SKIP instead of failing with KSFT_FAIL when
optional prerequisites are unavailable?

In run_server() and run_client(), if trtype is mptcp and neither native
MPTCP support in nbd-server/nbd-client nor mptcpize is installed, returning
1 causes run_test() to set ret=${KSFT_FAIL}. Also, run_server() hardcodes
/usr/bin/nbd-server rather than using command -v nbd-server.

Similarly, at the top level of mptcp_nbd.sh:

mptcp_lib_check_tools nbd fio
validate_params

if MPTCP is not supported by the kernel (checked via mptcp_lib_check_mptcp
in other mptcp selftests), ip/tc/ss are missing, or /dev/nbd0 is not
present (for example when CONFIG_BLK_DEV_NBD is disabled or the nbd module
is not loaded), the script will fail instead of skipping.

[ ... ]
> +	# Verify MPTCP subflows if trtype is mptcp
> +	if [ "${trtype}" = "mptcp" ]; then
> +		local sf_total sf_max tcp_conns is_fallback
> +		sf_total=$(ss -Mia | grep -oP "subflows_total:\K[0-9]+" | \
> +			head -1)
> +		sf_max=$(ss -Mia | grep -oP "subflows_max:\K[0-9]+" | head -1)
> +		tcp_conns=$(ss -tia | grep -c "tcp-ulp-mptcp" || true)
> +		is_fallback=$(ss -Mia | grep -c "fallback" || true)
> +		if [ "${is_fallback}" -gt 0 ]; then
> +			echo "FAIL: MPTCP fell back to TCP!" \
> +				"subflows_total:${sf_total}" \
> +				"tcp_subflows:${tcp_conns}"
> +		elif [ -n "${sf_total}" ]; then
> +			echo "PASS: MPTCP subflows_total:${sf_total}" \
> +				"subflows_max:${sf_max}" \
> +				"tcp_subflows:${tcp_conns}"
> +		else
> +			echo "WARNING: No MPTCP subflows info found"
> +		fi
> +	fi
> +
> +	return 0
> +}

[Severity: Medium]
In run_io_test(), when is_fallback is greater than 0 or sf_total is empty,
the function prints a FAIL or WARNING message but still falls through to
return 0, and the elif branch only checks that sf_total is non-empty rather
than verifying that multiple subflows were created. Doesn't this cause
mptcp_nbd.sh to report KSFT_PASS even when MPTCP falls back to TCP or fails
to establish subflows?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792746800.git.tanggeliang@kylinos.cn?part=2