Re: [PATCH mptcp-next v5 4/7] mptcp: pm: in-kernel: add 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 MPTCP_PM_ATTR_SUBFLOW_RETRIES netlink attribute is
  validated only by open-coded C, and its range is not declared in the…

--

mptcp: pm: in-kernel: add the subflow-retries limit

This adds a new in-kernel PM limit, subflow-retries, which is set and read
with the other limits through MPTCP_PM_CMD_SET_LIMITS and
MPTCP_PM_CMD_GET_LIMITS. It is the number of SYN retransmissions for a
subflow re-created after an error. The default is 15 and the maximum is
MAX_TCP_SYNCNT. The value is also exposed in struct mptcp_info, replacing
the reserved field.

> diff --git a/net/mptcp/pm_kernel.c b/net/mptcp/pm_kernel.c
> index b662eb914958c..f4e23deda1403 100644
> --- a/net/mptcp/pm_kernel.c
> +++ b/net/mptcp/pm_kernel.c

[ ... ]

> @@ -1435,8 +1444,15 @@ int mptcp_pm_nl_set_limits_doit(struct sk_buff *skb, struct genl_info *info)
>  	if (ret)
>  		goto unlock;
>  
> +	retries = pernet->limit_subflow_retries;
> +	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOW_RETRIES, &retries,
> +			  MAX_TCP_SYNCNT);
> +	if (ret)
> +		goto unlock;

[Severity: Low]
Could this range be declared in the netlink policy, instead of being
checked only here?

In mptcp_pm_nl_set_limits_doit(), this parse_limit() call is the only
place that enforces the 0..127 (MAX_TCP_SYNCNT) range. The spec entry
added earlier in the series, in Documentation/netlink/specs/mptcp_pm.yaml,
is a plain u32 with no max check:

        name: subflow-retries
        type: u32
        doc: |
          SYN retransmissions of a subflow re-created after an error, 0 to
          not re-create it.

So the generated mptcp_pm_set_limits_nl_policy in net/mptcp/mptcp_pm_gen.c
becomes:

	[MPTCP_PM_ATTR_SUBFLOW_RETRIES] = { .type = NLA_U32, },

Policy introspection (genl ctrl getpolicy) and spec-driven clients such
as YNL would then report the full u32 range as valid. The kernel still
returns -EINVAL for any value above 127.

Would adding "checks: max: 127" to the spec entry help here? It would
generate NLA_POLICY_MAX(NLA_U32, 127), so the spec, the policy dump and
the kernel would all agree. The netlink guidelines prefer a declared
NLA_POLICY_* over open-coded validation.

The existing subflows and rcv-add-addrs limits have the same gap. This
attribute is new, though, and its maximum is different from theirs
(MAX_TCP_SYNCNT vs MPTCP_PM_SUBFLOWS_MAX).

The spec doc also doesn't give the maximum or the default of 15. Could
those be added too?

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