Re: [PATCH mptcp-next v5 11/16] mptcp: add TCP_REPAIR sockopt support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The check for repair mode in __mptcp_close() is placed after the fastclose and shutdown sequences, breaking silent teardown.
- [Medium] mptcp_setsockopt_sol_tcp() silently ignores error codes when setting TCP_REPAIR.
- [Medium] TCP_REPAIR is missing from mptcp_getsockopt_sol_tcp(), causing getsockopt() to fail with -EOPNOTSUPP.
--

commit 31eec098dfb51659558f320d2e2ec1abfdffb89c
Author: Geliang Tang <geliang@kernel.org>

mptcp: add TCP_REPAIR sockopt support

Add TCP_REPAIR socket option support for MPTCP, required for sockmap CRIU
checkpoint/restore. When TCP_REPAIR is set on the first subflow, skip the
normal connection handshake in mptcp_connect() and transition directly to
TCP_ESTABLISHED. On close, skip the shutdown sequence and jump to cleanup
when repair mode is active.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index c5be29767cb15..07132976016e3 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3560,6 +3560,11 @@ bool __mptcp_close(struct sock *sk, long timeout)
>  		__mptcp_wr_shutdown(sk);
>  	}
>  
> +	if (msk->first && tcp_sk(msk->first)->repair) {
> +		mptcp_set_state(sk, TCP_CLOSE);
> +		goto cleanup;
> +	}
> +

[Severity: High]
Does this check evaluate too late in the shutdown sequence?

Since mptcp_do_fastclose() or __mptcp_wr_shutdown() can already be invoked
just before this block, could closing an MPTCP socket in repair mode incorrectly
leak RST or FIN packets to the network instead of being silently dismantled?

>  	sk_stream_wait_close(sk, timeout);
>  
>  cleanup:

[ ... ]

> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 922f6ae5c80cb..d2d1c9d1ed1d7 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c

[ ... ]

> @@ -918,6 +919,7 @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  	case TCP_CONGESTION:
>  		return mptcp_setsockopt_sol_tcp_congestion(msk, optval, optlen);
>  	case TCP_DEFER_ACCEPT:
> +	case TCP_REPAIR:
>  		/* See tcp.c: TCP_DEFER_ACCEPT does not fail */
>  		mptcp_setsockopt_first_sf_only(msk, SOL_TCP, optname, optval, optlen);
>  		return 0;

[Severity: Medium]
Since TCP_DEFER_ACCEPT does not fail, returning 0 unconditionally is correct
for it. However, because setting TCP_REPAIR requires privileges, can the
underlying tcp_setsockopt() fail and return an error like -EPERM?

If so, does discarding the return value of mptcp_setsockopt_first_sf_only()
cause these errors to be silently swallowed while returning 0 to the
application?

[Severity: Medium]
Also, should TCP_REPAIR be added to mptcp_getsockopt_sol_tcp()?

If it is omitted from the switch statement there, will applications querying the
repair state via getsockopt(SOL_TCP, TCP_REPAIR) incorrectly receive
-EOPNOTSUPP? For example:

net/mptcp/sockopt.c:mptcp_getsockopt_sol_tcp() {
	...
	switch (optname) {
	case TCP_ULP:
	case TCP_CONGESTION:
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1789294365.git.tanggeliang@kylinos.cn?part=11