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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] selftests: mptcp: broken native MPTCP and mptcpize checks in mptcp_nbd.sh
- [Medium] selftests: mptcp: weak and non-portable subflow check in run_io_test()
- [Medium] selftests: mptcp: mptcp_nbd.sh disconnects active host /dev/nbd0 in cleanup()
--

commit 7951aabe0d1917726a0a08d246614408762a837e
Author: Geliang Tang <geliang@kernel.org>

selftests: mptcp: add mptcp nbd tests

This commit adds mptcp_nbd.sh to test NBD over TCP and MPTCP using four
rate-limited virtual interfaces between two network namespaces, verifying
data integrity with dd/cmp and measuring I/O with fio.

> 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..29998be71a982
> --- /dev/null
> +++ b/tools/testing/selftests/net/mptcp/mptcp_nbd.sh

[ ... ]

> +# This function is used in the cleanup trap
> +#shellcheck disable=SC2317,SC2329
> +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
> +		ip netns exec "$ns1" kill -9 "${server_pid}" 2>/dev/null || true
> +	fi
> +
> +	# Clean up temp files
> +	if [ -n "${temp_file}" ]; then
> +		ip netns exec "$ns1" rm -f "${temp_file}" 2>/dev/null || true
> +	fi
> +	if [ -n "${conf_file}" ]; then
> +		ip netns exec "$ns1" rm -f "${conf_file}" 2>/dev/null || true
> +	fi
> +	if [ -n "${pid_file}" ]; then
> +		ip netns exec "$ns1" rm -f "${pid_file}" 2>/dev/null || true
> +	fi
> +
> +	mptcp_lib_ns_exit "$ns1" "$ns2"
> +
> +	unset -v trtype port
> +	unset -v temp_file server_pid
> +}

[Severity: Medium]
Can cleanup() disconnect an active /dev/nbd0 device on the host if /dev/nbd0
was already in use before the test started?

Before registering the cleanup trap, the script only checks whether
/dev/nbd0 is a block device node:

	# Check for /dev/nbd0
	if [ ! -b /dev/nbd0 ]; then
		mptcp_lib_pr_skip "/dev/nbd0 not found"
		exit ${KSFT_SKIP}
	fi

Because NBD devices in drivers/block/nbd.c are global across network
namespaces (nbd_index_idr is global and nbd_genl_family sets .netnsok = 1),
if /dev/nbd0 is already connected on the host, run_client() will fail to
attach it (or run_server() may fail earlier) and cleanup() will still execute
nbd-client -d /dev/nbd0, disconnecting the host's active block device. Should
the script check whether /dev/nbd0 is already connected via nbd-client -c
/dev/nbd0 before setting up the trap, or only disconnect /dev/nbd0 if
run_client() actually connected it?

Also, in run_io_test(), two 10 MiB temporary files are created inside the
child subshell:

	test_data=$(mktemp /tmp/nbd_test_XXXXXX.bin)
	read_data=$(mktemp /tmp/nbd_read_XXXXXX.bin)

If the test is interrupted or killed by a timeout during dd or cmp, will
test_data and read_data be leaked in /tmp since they are not tracked by
cleanup()? And in cleanup(), since temp_file, conf_file, and pid_file are
created on the host filesystem in init(), is ip netns exec "$ns1" needed to
remove them?

[ ... ]

> +run_server()
> +{
> +	# Create backing file
> +	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
> +		else
> +			mptcp_opt="mptcp = true"
> +			echo "INFO: nbd-server has native MPTCP support"
> +		fi
> +	fi

[ ... ]

> +run_client()
> +{
> +	local extra_args=()
> +	local nbd_dev="/dev/nbd0"
> +	local server_addr="10.1.1.1"
> +
> +	local use_mptcpize=""
> +	if [ "${trtype}" = "mptcp" ]; then
> +		if nbd-client 2>&1 | grep -qE "\-M|\-\-mptcp"; then
> +			extra_args+=("-M")
> +			echo "INFO: nbd-client has native MPTCP support (-M)"
> +		elif command -v mptcpize &> /dev/null; then
> +			use_mptcpize="mptcpize run -d"
> +			echo "INFO: nbd-client lacks native MPTCP," \
> +				"using mptcpize"
> +		else
> +			echo "ERROR: no MPTCP support in nbd-client"
> +			return 1
> +		fi
> +	fi

[Severity: Medium]
Does the native MPTCP detection in run_server() and run_client() work as
intended when the top-level script unconditionally skips if mptcpize is
missing?

Later in the script, before run_test() is called, there is an unconditional
check for mptcpize:

	# Check for mptcpize if using MPTCP transport
	if [ "${trtype}" = "mptcp" ]; then
		if ! command -v mptcpize &> /dev/null; then
			mptcp_lib_pr_skip "mptcpize tool not found"
			exit ${KSFT_SKIP}
		fi
	fi

Because that check exits with KSFT_SKIP whenever mptcpize is not installed,
systems where nbd-server and nbd-client have native MPTCP support will still
skip the test if mptcpize is absent, and the else branches in run_server()
and run_client() can never be reached.

In addition, in run_server():

	if ! grep -q '"mptcp"' /usr/bin/nbd-server 2>/dev/null && \
	   ! nbd-server --help 2>&1 | grep -qi mptcp; then

Can grep -q '"mptcp"' /usr/bin/nbd-server ever match a compiled ELF binary?
In a compiled C binary, string literals in .rodata do not include the
surrounding C source double quotes, and /usr/bin/nbd-server is also
hardcoded rather than resolved via command -v nbd-server (for example when
installed in /usr/sbin). Since mptcp = true is a configuration file directive
rather than a command-line option printed by nbd-server --help, won't
run_server() always fall back to mptcpize even when nbd-server supports
native MPTCP?

[ ... ]

> +	# 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}"
> +			return 1
> +		elif [ -n "${sf_total}" ]; then
> +			echo "PASS: MPTCP subflows_total:${sf_total}" \
> +				"subflows_max:${sf_max}" \
> +				"tcp_subflows:${tcp_conns}"
> +		else
> +			echo "FAIL: No MPTCP subflows info found"
> +			return 1
> +		fi
> +	fi

[Severity: Medium]
In run_io_test(), is checking [ -n "${sf_total}" ] sufficient to verify that
the additional MPTCP subflows were established?

In net/mptcp/sockopt.c:mptcp_diag_fill_info():

	info->mptcpi_subflows_total = info->mptcpi_extra_subflows +
		__mptcp_has_initial_subflow(msk);

Even if all extra subflows fail to establish across the four interfaces
configured in init(), subflows_total is still 1 for the initial subflow, so
elif [ -n "${sf_total}" ] will pass with only a single subflow.

Also, on systems where ss supports MPTCP but predates the subflows_total
field, sf_total will be empty and this check will fail with "No MPTCP
subflows info found" even though tcp_conns was computed right above. Could
this use mptcp_lib_get_info_value() instead of grep -oP, fall back to
tcp_conns when subflows_total is unavailable (similar to chk_subflows_total()
in mptcp_join.sh), and verify that multiple subflows were actually created?

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