Re: [PATCH mptcp-next v6 2/6] netlink: specs: mptcp: add the subflow-retries attribute

[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 concern: commit "netlink: specs: mptcp: add the subflow-
  retries attribute" adds MPTCP_PM_ATTR_SUBFLOW_RETRIES to the uAPI…

--

netlink: specs: mptcp: add the subflow-retries attribute

This commit adds the subflow-retries attribute to the MPTCP PM netlink
spec for the set-limits and get-limits commands. ynl-regen regenerates the
uapi header and the generated policy files. The new policy limits the value
to 127 (MAX_TCP_SYNCNT).

> diff --git a/net/mptcp/mptcp_pm_gen.c b/net/mptcp/mptcp_pm_gen.c
> index c180930a8e0e4..e2d4af1bc1d5d 100644
> --- a/net/mptcp/mptcp_pm_gen.c
> +++ b/net/mptcp/mptcp_pm_gen.c
> @@ -44,15 +44,17 @@ const struct nla_policy mptcp_pm_flush_addrs_nl_policy[MPTCP_PM_ENDPOINT_ADDR +
>  };
>  
>  /* MPTCP_PM_CMD_SET_LIMITS - do */
> -const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOWS + 1] = {
> +const struct nla_policy mptcp_pm_set_limits_nl_policy[MPTCP_PM_ATTR_SUBFLOW_RETRIES + 1] = {
>  	[MPTCP_PM_ATTR_RCV_ADD_ADDRS] = { .type = NLA_U32, },
>  	[MPTCP_PM_ATTR_SUBFLOWS] = { .type = NLA_U32, },
> +	[MPTCP_PM_ATTR_SUBFLOW_RETRIES] = NLA_POLICY_MAX(NLA_U32, 127),
>  };

[Severity: Low]
With only this commit applied, does the kernel accept
MPTCP_PM_ATTR_SUBFLOW_RETRIES and then silently drop it?

The policy checks the value (max 127). But mptcp_pm_nl_set_limits_doit()
in net/mptcp/pm_kernel.c still parses only MPTCP_PM_ATTR_RCV_ADD_ADDRS and
MPTCP_PM_ATTR_SUBFLOWS. A set-limits request that carries the new attribute
returns success and the value is not stored.

The same applies to mptcp_pm_nl_get_limits_doit(): it does not put the
attribute in its reply, although the spec lists it in the get-limits reply.

The next commit in this series, "mptcp: pm: in-kernel: add the
subflow-retries limit", fixes this. In that commit, set-limits does:

	attr = info->attrs[MPTCP_PM_ATTR_SUBFLOW_RETRIES];
	retries = nla_get_u32_default(attr, pernet->limit_subflow_retries);

and get-limits does:

	if (nla_put_u32(msg, MPTCP_PM_ATTR_SUBFLOW_RETRIES,
			READ_ONCE(pernet->limit_subflow_retries)))

So this only affects a kernel built at exactly this commit, for example
while bisecting. Would it be better to fold the spec change into that
commit? Or is the usual netdev split, with the ynl-regen patch landing
before its user, acceptable here?

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