[PATCH mptcp-net 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts

[PATCH mptcp-net 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts

[Matthieu Baerts (NGI0)]

This is linked to a discussion we had a few weeks ago: not supporting
TCP_KEEP* socket options is preventing MPTCP to be used in some apps or
libraries like it was the case in GoLang recently.

Supporting them is not difficult, it should have probably done before,
when SO_KEEPALIVE support has been added. Supporting them is easy and
isolated from the rest, it sounds safe enough to add a Fixes tag, and
check with the stable team to backport them. I understand it can be a
bit controversial, but it would unnecessary delay (1 or 2 years?) some
deployments, for something that simple.

While at it, getsockopt(SO_KEEPALIVE) is now returning the expected
value.

The manipulation of these different socket options can be verified with
this simple packetdrill test:

  --tolerance_usecs=100000
  `../common/defaults.sh`

  0.0    socket(..., SOCK_STREAM, IPPROTO_MPTCP) = 3
  +0     setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
  +0     bind(3, ..., ...) = 0
  +0     listen(3, 1) = 0

  +0       <  S   0:0(0)         win 8000  <mss 1024, sackOK, nop, nop, nop, wscale 0, mpcapable v1 flags[flag_h] nokey>
  +0       >  S.  0:0(0)  ack 1            <mss 1460, nop, nop, sackOK, nop, wscale 8, mpcapable v1 flags[flag_h] key[skey]>
  +0.01    <   .  1:1(0)  ack 1  win 8000                                             <mpcapable v1 flags[flag_h] key[ckey=2, skey]>
  +0     accept(3, ..., ...) = 4

  // Set and check the different Keep-Alive options.
  +0     getsockopt(4, SOL_SOCKET, SO_KEEPALIVE, [0], [4]) = 0
  +0     setsockopt(4, SOL_SOCKET, SO_KEEPALIVE, [1], 4) = 0
  +0     getsockopt(4, SOL_SOCKET, SO_KEEPALIVE, [1], [4]) = 0

  +0     getsockopt(4, SOL_TCP, TCP_KEEPIDLE, [7200], [4]) = 0
  +0     setsockopt(4, SOL_TCP, TCP_KEEPIDLE, [7200], 4) = 0
  +0     getsockopt(4, SOL_TCP, TCP_KEEPIDLE, [7200], [4]) = 0

  +0     getsockopt(4, SOL_TCP, TCP_KEEPINTVL, [75], [4]) = 0
  +0     setsockopt(4, SOL_TCP, TCP_KEEPINTVL, [10], 4) = 0
  +0     getsockopt(4, SOL_TCP, TCP_KEEPINTVL, [10], [4]) = 0

  +0     getsockopt(4, SOL_TCP, TCP_KEEPCNT, [9], [4]) = 0
  +0     setsockopt(4, SOL_TCP, TCP_KEEPCNT, [2], 4) = 0
  +0     getsockopt(4, SOL_TCP, TCP_KEEPCNT, [2], [4]) = 0

Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Matthieu Baerts (NGI0) (2):
      mptcp: SO_KEEPALIVE: fix getsockopt support
      mptcp: fix full TCP keep-alive support

 net/mptcp/protocol.h |  3 +++
 net/mptcp/sockopt.c  | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 2 deletions(-)
---
base-commit: a64044939fd33676d7098eb08397e65e83d278ed
change-id: 20240507-mptcp-tcp-keepalive-sockopts-546d3b1f3256

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

[PATCH mptcp-net 1/2] mptcp: SO_KEEPALIVE: fix getsockopt support

[Matthieu Baerts (NGI0)]

SO_KEEPALIVE support has to be set on each subflow: on each TCP socket,
where sk_prot->keepalive is defined. Technically, nothing has to be done
on the MPTCP socket. That's why mptcp_sol_socket_sync_intval() was
called instead of mptcp_sol_socket_intval().

Except that when nothing is done on the MPTCP socket, the
getsockopt(SO_KEEPALIVE), handled in net/core/sock.c:sk_getsockopt(),
will not know if SO_KEEPALIVE has been set on the different subflows or
not.

The fix is simple: simply call mptcp_sol_socket_intval() which will end
up calling net/core/sock.c:sk_setsockopt() where the SOCK_KEEPOPEN flag
will be set, the one used in sk_getsockopt().

So now, getsockopt(SO_KEEPALIVE) on an MPTCP socket will return the same
value as the one previously set with setsockopt(SO_KEEPALIVE).

Fixes: 1b3e7ede1365 ("mptcp: setsockopt: handle SO_KEEPALIVE and SO_PRIORITY")
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
 net/mptcp/sockopt.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index 2edaf1a16005..d9f0d36e24ad 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -181,8 +181,6 @@ static int mptcp_setsockopt_sol_socket_int(struct mptcp_sock *msk, int optname,
 
 	switch (optname) {
 	case SO_KEEPALIVE:
-		mptcp_sol_socket_sync_intval(msk, optname, val);
-		return 0;
 	case SO_DEBUG:
 	case SO_MARK:
 	case SO_PRIORITY:

-- 
2.43.0

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

[Matthieu Baerts (NGI0)]

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;
 	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;
+
+	*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);
+		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,
+						 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);
+	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));

-- 
2.43.0

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

[Paolo Abeni]

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));
> 

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

[Matthieu Baerts]

Hi Paolo,

Thank you for the review!

On 08/05/2024 17:44, Paolo Abeni wrote:
> 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...

Yes, I agree, but I don't see how else we could do that :-/

(Or relying on what has been set in the first subflow? But that's not
what we are currently doing, and should be part of a separated
refactoring I think, see below.)

>>  	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.

Good idea!

>> +	*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;

Yes, sounds cleaner and similar to what we did with TCP_CONGESTION, thanks!

>> +		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'

Argh, good catch! I checked the limits (1 < val < MAX_TCP_KEEPCNT), and
I missed the overflowing later, sorry :-/
Easier to store the original value, as an 'int' then.

> 
>> +						 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));

I initially did that -- by calling mptcp_getsockopt_first_sf_only() --
like it is done for TCP_FASTOPEN*, etc.

But then I changed that because TCP_FASTOPEN* and similar only make
sense on the first subflow, while the other ones in the list (TCP_INQ,
TCP_CORK, etc.) are set on all subflows. So I thought not to make an
exception here. (Also, socket options could be modified per subflow with
BPF, best not to have exceptions here).

But I still think it would make sense to do that if we also do the same
with the other socket options: I do think the way we handle the socket
options should be simplified, e.g. what I proposed a while ago, and I
failed to update so far:

  https://patchwork.kernel.org/project/mptcp/list/?series=763505&state=7

We could rely on the first subflow for the get as you suggested, but
also use {ip,ipv6,tcp,sock}_[gs]etsockopt(), instead of setting
variables manually. And maybe refactoring the 'sync' to "duplicate"
options set on the first subflow, and not to have to store TCP specific
options that have been set.

Would it be OK to look at that later, and keep this way here?

>> +	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));
>>
> 

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

Re: [PATCH mptcp-net 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf (only bpftest_all): Success! ✅
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/8999961874

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/852e82e9b555
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=851490


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)