Re: [PATCH mptcp-next v5 3/7] 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] This gap exists only at this one commit; the next patch in the
  series closes it.

--

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

This adds a subflow-retries u32 attribute to the MPTCP PM netlink spec and
lists it in the set-limits request and in the get-limits request and reply.
The uapi header and the generated policy files are regenerated to match.

> diff --git a/net/mptcp/mptcp_pm_gen.c b/net/mptcp/mptcp_pm_gen.c
> index c180930a8e0e4..127a1fadaac40 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] = { .type = NLA_U32, },
>  };

[ ... ]

> @@ -127,7 +129,7 @@ const struct genl_ops mptcp_pm_nl_ops[11] = {
>  		.validate	= GENL_DONT_VALIDATE_STRICT,
>  		.doit		= mptcp_pm_nl_set_limits_doit,
>  		.policy		= mptcp_pm_set_limits_nl_policy,
> -		.maxattr	= MPTCP_PM_ATTR_SUBFLOWS,
> +		.maxattr	= MPTCP_PM_ATTR_SUBFLOW_RETRIES,
>  		.flags		= GENL_UNS_ADMIN_PERM,
>  	},

[Severity: Low]
With only this patch applied, MPTCP_PM_CMD_SET_LIMITS accepts
MPTCP_PM_ATTR_SUBFLOW_RETRIES. Is the value then silently dropped?

mptcp_pm_nl_set_limits_doit() in net/mptcp/pm_kernel.c still parses only
the two existing limits:

	ret = parse_limit(info, MPTCP_PM_ATTR_RCV_ADD_ADDRS, &rcv_addrs,
			  MPTCP_PM_SUBFLOWS_MAX);
	...
	ret = parse_limit(info, MPTCP_PM_ATTR_SUBFLOWS, &subflows,
			  MPTCP_PM_SUBFLOWS_MAX);
	...
	WRITE_ONCE(pernet->limit_add_addr_accepted, rcv_addrs);
	WRITE_ONCE(pernet->limit_extra_subflows, subflows);

A request that carries subflow-retries passes validation and returns 0,
but the value is not stored anywhere.

In the same way, mptcp_pm_nl_get_limits_doit() never adds
MPTCP_PM_ATTR_SUBFLOW_RETRIES to its reply, even though the spec lists it
in the get-limits reply.

Only a bisect build at this commit is affected. The next patch in the
series, "mptcp: pm: in-kernel: add the subflow-retries limit", adds the
parse_limit() call, the WRITE_ONCE(pernet->limit_subflow_retries, ...) and
the nla_put_u32() in the reply. At the end of the series the spec and the
implementation match.

Could this spec change be squashed into that patch, so the attribute is
never advertised without a handler?

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