Re: [PATCH mptcp-next v9 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] At this commit, MPTCP_PM_ATTR_SUBFLOW_RETRIES has been added to
  include/uapi/linux/mptcp_pm.h and…

--

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

This commit adds the subflow-retries attribute to the mptcp_pm netlink
spec and the uapi header. It also adds the attribute to the set-limits
and get-limits commands, and regenerates the policies with ynl-regen.

> diff --git a/Documentation/netlink/specs/mptcp_pm.yaml b/Documentation/netlink/specs/mptcp_pm.yaml
> index 39f3facc38e55..690d3c5dfdd21 100644
> --- a/Documentation/netlink/specs/mptcp_pm.yaml
> +++ b/Documentation/netlink/specs/mptcp_pm.yaml
> @@ -196,6 +196,16 @@ attribute-sets:
>          name: addr-remote
>          type: nest
>          nested-attributes: address
> +      -
> +        name: subflow-retries
> +        type: u32
> +        checks:
> +          max: 127
> +        doc: |
> +          SYN retries of a subflow re-created after a timeout, like the
> +          TCP_SYNCNT socket option, so with tcp_syn_linear_timeouts on top.
> +          The default is the maximum, MAX_TCP_SYNCNT (127), about 4 hours
> +          with the default RTO settings. 0 to not re-create it.

[ ... ]

> 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]
After this change genl accepts and validates the attribute. At this commit,
though, mptcp_pm_nl_set_limits_doit() and mptcp_pm_nl_get_limits_doit() in
net/mptcp/pm_kernel.c only handle MPTCP_PM_ATTR_RCV_ADD_ADDRS and
MPTCP_PM_ATTR_SUBFLOWS.

Does that mean set-limits takes a subflow-retries value from 0 to 127,
returns success and then discards it? It also looks like the get-limits
reply leaves the attribute out, although the spec lists it as a reply
attribute.

The documented default (127) and the "0 to not re-create it" behaviour also
don't exist yet at this point in the series.

Because .maxattr is raised to MPTCP_PM_ATTR_SUBFLOW_RETRIES, a set-limits
or get-limits request with subflow-retries above 127 now fails with -ERANGE.
Before this change, attribute type 7 was above maxattr and was ignored
under GENL_DONT_VALIDATE_STRICT. Is that change intended at this commit?

Later in the series, "mptcp: pm: in-kernel: add the subflow-retries limit"
adds the missing pieces. mptcp_pm_nl_set_limits_doit() parses the
attribute:

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

mptcp_pm_nl_get_limits_doit() emits it, and pm_nl_init_net() sets the
default to MAX_TCP_SYNCNT. Then "mptcp: pm: in-kernel: retry subflows
closed after an error" uses the value and treats 0 as "do not re-create".
So the gap is only visible when bisecting.

The commit message says the attribute "will be used by the in-kernel PM",
so the split seems deliberate. Would it be worth folding the uapi and spec
change into the implementation patch, so that no single commit exposes
the attribute without handling it?

[ ... ]

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