Re: [PATCH mptcp-next v8 5/6] selftests: mptcp: pm_nl_ctl: support the subflow-retries limit

[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 'subflow retries above hard limit' subtest in
  tools/testing/selftests/net/mptcp/pm_netlink.sh cannot show what the…

--

selftests: mptcp: pm_nl_ctl: support the subflow-retries limit

With this patch, 'pm_nl_ctl limits' can set the subflow-retries limit
through an optional third argument, and it prints the limit when the
kernel reports it. pm_netlink.sh is extended to check the default value,
that a value above the maximum is refused, and that the value can be set.

> diff --git a/tools/testing/selftests/net/mptcp/pm_netlink.sh b/tools/testing/selftests/net/mptcp/pm_netlink.sh
> index 21bfe1311f118..99072ad8052d9 100755
> --- a/tools/testing/selftests/net/mptcp/pm_netlink.sh
> +++ b/tools/testing/selftests/net/mptcp/pm_netlink.sh

[ ... ]

> @@ -75,6 +80,22 @@ get_limits_nb() {
>  	fi
>  }
>  
> +# The subflow retries limit is only set and shown with pm_nl_ctl for now.
> +# Prints its default value if it is supported.
> +get_default_retries() {
> +	if mptcp_lib_is_ip_mptcp; then
> +		return
> +	fi
> +
> +	if ip netns exec "${ns1}" ./pm_nl_ctl limits |
> +	   grep -q "^subflow_retries"; then
> +		echo 127
> +	elif mptcp_lib_expect_all_features; then
> +		# missing feature: the defaults limits check will fail
> +		echo 127
> +	fi
> +}

[ ... ]

> @@ -224,7 +247,27 @@ check "get_limits" "${default_limits}" "subflows above hard limit"
>  
>  set_limits ${max_limits} ${max_limits}
>  flush_endpoint  ## to make sure it doesn't affect the limits
> -check "get_limits" "$(format_limits ${max_limits} ${max_limits})" "set limits"
> +max_limits_out="$(format_limits ${max_limits} ${max_limits} \
> +		 "${default_retries}")"
> +check "get_limits" "${max_limits_out}" "set limits"
> +
> +if [ -n "${default_retries}" ]; then
> +	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} \
> +		128 2>/dev/null
> +	check "get_limits" "${max_limits_out}" \
> +		"subflow retries above hard limit"

[Severity: Low]
Can this subtest tell a refused value apart from a clamped one?

The commit message says pm_netlink.sh checks "that a value above the
maximum is refused". Since v8, the per-netns default is the same value
as the policy maximum.

In net/mptcp/pm_kernel.c:pm_nl_init_net():

	pernet->limit_subflow_retries = 127;

In net/mptcp/mptcp_pm_gen.c, mptcp_pm_set_limits_nl_policy has:

	[MPTCP_PM_ATTR_SUBFLOW_RETRIES] = NLA_POLICY_MAX(NLA_U32, 127),

get_default_retries() also hardcodes 127, so max_limits_out already
contains "subflow_retries 127" when the request with 128 is sent. The
exit status and stderr of pm_nl_ctl are thrown away.

If the kernel refuses 128, the readback shows 127. If 128 were accepted
and clamped to 127, the readback would also show 127. Both outcomes pass
this check, so it only fails if 128 is stored as-is.

The earlier "rcv addrs above hard limit" and "subflows above hard limit"
checks work because the stored value differs from what the bad request
would set.

Could the 128 attempt be moved after the "set subflow retries" step, so
the check can verify the value stays at 0? Checking that pm_nl_ctl exits
non-zero for the 128 request would also work.

> +
> +	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} 0
> +	check "get_limits" "$(format_limits ${max_limits} ${max_limits} 0)" \
> +		"set subflow retries"

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261022151130.2285198-1-anilkaushikwireless%40gmail.com