Re: [PATCH mptcp-net v5 1/2] mptcp: pm: in-kernel: fullmesh: skip closed subflows

[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 commit message gets the failure mechanism partly wrong for
  the case where the initial subflow is already in TCP_CLOSE.

--

mptcp: pm: in-kernel: fullmesh: skip closed subflows

This patch changes fill_remote_addresses_fullmesh() to skip subflows that
are closing or in TCP_CLOSE. They are skipped both when collecting the
remote IDs in use and when picking remote addresses. If no subflow is left
to provide the remote ID 0 address, the remote address of the MPTCP socket
is used instead, as fill_remote_addr() does.

> If it was already in TCP_CLOSE,
> e.g. after a reset, it is disconnected and its context is reset: the
> 'closing' flag and the remote ID are cleared, and its remote address
> and port are zeroed. It is then picked as a remote address, with the
> address 0.0.0.0, the port 0 and the ID 0, which also marks the remote
> ID 0 as unavailable.

[Severity: Low]
Is this description of the TCP_CLOSE case accurate?

>From __mptcp_close_ssk(), the initial subflow stays in the conn_list and
goes through __mptcp_subflow_disconnect():

net/mptcp/protocol.c:__mptcp_subflow_disconnect() {
    ...
	WARN_ON_ONCE(tcp_disconnect(ssk, 0));
	mptcp_subflow_ctx_reset(subflow);
    ...
}

On that path, tcp_disconnect() resets only the destination port and the
source address:

net/ipv4/tcp.c:tcp_disconnect() {
    ...
	inet->inet_dport = 0;

	inet_bhash2_reset_saddr(sk);
    ...
}

inet_bhash2_reset_saddr() ends up in inet_reset_saddr(). That clears
inet_saddr, inet_rcv_saddr and the IPv6 source addresses, but not
sk_daddr or skc_v6_daddr. mptcp_subflow_ctx_reset() clears the reset
group (closing, remote_id, ...) and sets local_id to -1. It does not
touch the socket addresses.

So in the old fill_remote_addresses_fullmesh(), mptcp_remote_address(ssk)
would return the real peer address with port 0 and remote ID 0, not
0.0.0.0:0.

The conclusion still holds, since port 0 is unusable and remote ID 0
still gets marked as unavailable. Could the changelog say that only the
destination port, the source address and the IDs are reset, and that the
peer address is kept?

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