Re: [PATCH mptcp-net 2/2] mptcp: fix full TCP keep-alive support

[Paolo Abeni <pabeni@redhat.com>]

On Wed, 2024-05-08 at 11:44 +0200, Matthieu Baerts (NGI0) wrote:
> SO_KEEPALIVE support has been added a while ago, as part of a series
> "adding SOL_SOCKET" support. To have a full control of this keep-alive
> feature, it is important to also support TCP_KEEP* socket options at the
> SOL_TCP level.
> 
> Supporting them on the setsockopt() part is easy, it is just a matter of
> remembering each value in the MPTCP sock structure, and calling
> tcp_sock_set_keep*() helpers on each subflow. If the value is not
> modified (0), calling these helpers will not do anything. For the
> getsockopt() part, the corresponding value from the MPTCP sock structure
> or the default one is simply returned.
> 
> It looks important for kernels supporting SO_KEEPALIVE, to also support
> TCP_KEEP* options as well: some apps seem to (wrongly) consider that if
> the former is supported, the latter ones will be supported as well. But
> also, not having this simple and isolated change is preventing MPTCP
> support in some apps, and libraries like GoLang [1]. This is why this
> patch is seen as a fix.
> 
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/383
> Fixes: 1b3e7ede1365 ("mptcp: setsockopt: handle SO_KEEPALIVE and SO_PRIORITY")
> Link: https://github.com/golang/go/issues/56539 [1]
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
>  net/mptcp/protocol.h |  3 +++
>  net/mptcp/sockopt.c  | 57 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index c09357e04f23..8c7ab1d50c75 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -309,6 +309,9 @@ struct mptcp_sock {
>  			in_accept_queue:1,
>  			free_first:1,
>  			rcvspace_init:1;
> +	u8		keepalive_cnt;
> +	unsigned int	keepalive_idle;
> +	unsigned int	keepalive_intvl;

It's a bit a pity we have to duplicate such values in all the subflows
_and_ in the msk...

>  	u32		notsent_lowat;
>  	struct work_struct work;
>  	struct sk_buff  *ooo_last_skb;
> diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
> index d9f0d36e24ad..794d8b63024a 100644
> --- a/net/mptcp/sockopt.c
> +++ b/net/mptcp/sockopt.c
> @@ -622,6 +622,30 @@ static int mptcp_setsockopt_sol_tcp_congestion(struct mptcp_sock *msk, sockptr_t
>  	return ret;
>  }
>  
> +static int __mptcp_setsockopt_set_val(struct mptcp_sock *msk, int max,
> +				      int (*set_val)(struct sock *, int),
> +				      unsigned int *msk_val, int val)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	int ret = 0;
> +
> +	if (val < 1 || val > max)
> +		return -EINVAL;

I think it would be better just skip this check, to avoid copying more
logic from TCP, and do the store and sockopt_seq_inc() only when the
operation is successful on all the subflows.

> +
> +	*msk_val = val;
> +	sockopt_seq_inc(msk);
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		lock_sock(ssk);
> +		ret |= set_val(ssk, val);

the above produces 'reasonable' return code only if all 'set_val' calls
will return the same error code.

That expectation should be respected by the existing code, but what
about keeping the first error code only? e.g:

	int err, ret;
	// ...

		ret = set_val(ssk, val);
		err = err ?: ret;

	// ...
	return err;

> +		release_sock(ssk);
> +	}
> +
> +	return ret;
> +}
> +
>  static int __mptcp_setsockopt_sol_tcp_cork(struct mptcp_sock *msk, int val)
>  {
>  	struct mptcp_subflow_context *subflow;
> @@ -818,6 +842,22 @@ static int mptcp_setsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  	case TCP_NODELAY:
>  		ret = __mptcp_setsockopt_sol_tcp_nodelay(msk, val);
>  		break;
> +	case TCP_KEEPIDLE:
> +		ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPIDLE,
> +						 &tcp_sock_set_keepidle_locked,
> +						 &msk->keepalive_idle, val);
> +		break;
> +	case TCP_KEEPINTVL:
> +		ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPINTVL,
> +						 &tcp_sock_set_keepintvl,
> +						 &msk->keepalive_intvl, val);
> +		break;
> +	case TCP_KEEPCNT:
> +		ret = __mptcp_setsockopt_set_val(msk, MAX_TCP_KEEPCNT,
> +						 &tcp_sock_set_keepcnt,
> +						 (unsigned int *)&msk->keepalive_cnt,

keepalive_cnt is 'u8' so the above will be technically a buffer
overflow ;) If we keep 'keepalive_cnt' in msk, it has to become
an 'unsigned int'

> +						 val);
> +		break;
>  	default:
>  		ret = -ENOPROTOOPT;
>  	}
> @@ -1332,6 +1372,8 @@ static int mptcp_put_int_option(struct mptcp_sock *msk, char __user *optval,
>  static int mptcp_getsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  				    char __user *optval, int __user *optlen)
>  {
> +	struct sock *sk = (void *)msk;
> +
>  	switch (optname) {
>  	case TCP_ULP:
>  	case TCP_CONGESTION:
> @@ -1352,6 +1394,18 @@ static int mptcp_getsockopt_sol_tcp(struct mptcp_sock *msk, int optname,
>  		return mptcp_put_int_option(msk, optval, optlen, msk->nodelay);
>  	case TCP_NOTSENT_LOWAT:
>  		return mptcp_put_int_option(msk, optval, optlen, msk->notsent_lowat);
> +	case TCP_KEEPIDLE:
> +		return mptcp_put_int_option(msk, optval, optlen,
> +					    msk->keepalive_idle ? :
> +					    READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_keepalive_time) / HZ);

If we force the first subflow to be present/allocated at setsockopt
time, then here we could:

	case TCP_KEEPIDLE:
	case TCP_KEEPIDLE:
	case TCP_KEEPCNT:
		return do_tcp_getsockopt(msk->first, SOL_TCP,
optname, 
					USER_SOCKPTR(optval),
					USER_SOCKPTR(optlen));

> +	case TCP_KEEPINTVL:
> +		return mptcp_put_int_option(msk, optval, optlen,
> +					    msk->keepalive_intvl ? :
> +					    READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_keepalive_intvl) / HZ);
> +	case TCP_KEEPCNT:
> +		return mptcp_put_int_option(msk, optval, optlen,
> +					    msk->keepalive_cnt ? :
> +					    READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_keepalive_probes));
>  	}
>  	return -EOPNOTSUPP;
>  }
> @@ -1467,6 +1521,9 @@ static void sync_socket_options(struct mptcp_sock *msk, struct sock *ssk)
>  		tcp_set_congestion_control(ssk, msk->ca_name, false, true);
>  	__tcp_sock_set_cork(ssk, !!msk->cork);
>  	__tcp_sock_set_nodelay(ssk, !!msk->nodelay);
> +	tcp_sock_set_keepidle_locked(ssk, msk->keepalive_idle);
> +	tcp_sock_set_keepintvl(ssk, msk->keepalive_intvl);
> +	tcp_sock_set_keepcnt(ssk, msk->keepalive_cnt);
>  
>  	inet_assign_bit(TRANSPARENT, ssk, inet_test_bit(TRANSPARENT, sk));
>  	inet_assign_bit(FREEBIND, ssk, inet_test_bit(FREEBIND, sk));
>