Re: [PATCH mptcp-next v5 6/7] 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 new subflow-retries subtests in
  tools/testing/selftests/net/mptcp/pm_netlink.sh test only one side of…

--

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

This commit lets 'pm_nl_ctl limits' take an optional third value that
sets the subflow-retries limit. It also prints that limit from the
GET_LIMITS reply when the kernel reports it. pm_netlink.sh gets new
subtests for the default value, for an out-of-range value being refused,
and for setting a new value.

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

[ ... ]

> @@ -224,7 +247,22 @@ 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"
> +
> +	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"

[Severity: Low]
This isn't a bug, but should these subtests also check that the largest
valid value is accepted?

The values sent here are 128 (refused), 0, and then the default 15 when
the limit is restored. On the kernel side, mptcp_pm_nl_set_limits_doit()
in net/mptcp/pm_kernel.c does:

	retries = pernet->limit_subflow_retries;
	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOW_RETRIES, &retries,
			  MAX_TCP_SYNCNT);

parse_limit() only refuses the value when *limit > max, so 127 should be
accepted. Nothing in the test sends 127.

If the check broke in a way that still refuses 128 and accepts 0, these
subtests would still pass. Two examples are an off-by-one (>= instead
of >), which would refuse 127, and a smaller cap such as 15.

The earlier tests for the rcv-add-addrs and subflows limits cover both
sides of the boundary: max_limits + 1 is refused, and max_limits is
accepted in "set limits".

Could a step be added that sets 127 and checks it with format_limits
before the default is restored?

A smaller point: the out-of-range request sends rcv_addr and subflows
equal to the values already in place (max_limits). That means the test
can't show that the whole request is refused together. The current
kernel stores all three values only after every parse_limit() call
succeeds, so this is a gap in the test, not a bug.

> +	ip netns exec "${ns1}" ./pm_nl_ctl limits ${max_limits} ${max_limits} \
> +		"${default_retries}"
> +fi
>
>  flush_endpoint
>  add_endpoint 10.0.1.1

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