Re: [PATCH mptcp] mptcp: sockopt: make sync_socket_options propagate SOCK_KEEPOPEN

[Geliang Tang <geliang@kernel.org>]

Hi Krister,

On Fri, 2025-09-05 at 17:43 -0700, Krister Johansen wrote:
> Users reported a scenario where MPTCP connections that were
> configured
> with SO_KEEPALIVE prior to connect would fail to enable their
> keepalives
> if MTPCP fell back to TCP mode.
> 
> After investigating, this affects keepalives for any connection where
> sync_socket_options is called on a socket that is in the closed or
> listening state.  Joins are handled properly. For connects,
> sync_socket_options is called when the socket is still in the closed
> state.  The tcp_set_keepalive() function does not act on sockets that
> are closed or listening, hence keepalive is not immediately enabled.
> Since the SO_KEEPOPEN flag is absent, it is not enabled later in the
> connect sequence via tcp_finish_connect.  Setting the keepalive via
> sockopt after connect does work, but would not address any
> subsequently
> created flows.
> 
> Fortunately, the fix here is straight-forward: set SOCK_KEEPOPEN on
> the
> subflow when calling sync_socket_options.
> 
> The fix was valdidated both by using tcpdump to observe keeplaive
> packets not being sent before the fix, and being sent after the fix. 
> It
> was also possible to observe via ss that the keepalive timer was not
> enabled on these sockets before the fix, but was enabled afterwards.
> 
> Fixes: 1b3e7ede1365 ("mptcp: setsockopt: handle SO_KEEPALIVE and
> SO_PRIORITY")
> Cc: stable@vger.kernel.org
> Signed-off-by: Krister Johansen <kjlx@templeofstupid.com>

Thanks for this fix. Good catch!

Reviewed-by: Geliang Tang <geliang@kernel.org>

-Geliang

> ---
>  net/mptcp/sockopt.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index 2c267aff95be..13108e9f982b 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -1532,13 +1532,11 @@ static void sync_socket_options(struct
> mptcp_sock *msk, struct sock *ssk)
>  {
>  	static const unsigned int tx_rx_locks = SOCK_RCVBUF_LOCK |
> SOCK_SNDBUF_LOCK;
>  	struct sock *sk = (struct sock *)msk;
> +	int kaval = !!sock_flag(sk, SOCK_KEEPOPEN);
>  
> -	if (ssk->sk_prot->keepalive) {
> -		if (sock_flag(sk, SOCK_KEEPOPEN))
> -			ssk->sk_prot->keepalive(ssk, 1);
> -		else
> -			ssk->sk_prot->keepalive(ssk, 0);
> -	}
> +	if (ssk->sk_prot->keepalive)
> +		ssk->sk_prot->keepalive(ssk, kaval);
> +	sock_valbool_flag(ssk, SOCK_KEEPOPEN, kaval);
>  
>  	ssk->sk_priority = sk->sk_priority;
>  	ssk->sk_bound_dev_if = sk->sk_bound_dev_if;
> 
> base-commit: 319f7385f22c85618235ab0169b80092fa3c7696