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

[PATCH mptcp-net v2 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>
---
Changes in v2:
- Addressed Paolo's comments in patch 2/2, see the individual changelog.
- Link to v1: https://lore.kernel.org/r/20240508-mptcp-tcp-keepalive-sockopts-v1-0-fdf7e03e14c4@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  | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 61 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 v2 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 v2 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. All of this is very similar to
other TCP_* socket options supported by MPTCP.

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>
---
Notes:
 - v1 -> v2: Addressing Paolo's comments (thanks!)
   - Store all keepalive_* variables as 'int' in the struct mptcp_sock.
   - Set these var (+ sync) only if there were no errors on the subflows.
   - Return the first error in case of multiple and different ones.
   - *Not* changed: getsockopt() part, so not looking at the 1st subflow.
---
 net/mptcp/protocol.h |  3 +++
 net/mptcp/sockopt.c  | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index c09357e04f23..b5fb26ece2b8 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -310,6 +310,9 @@ struct mptcp_sock {
 			free_first:1,
 			rcvspace_init:1;
 	u32		notsent_lowat;
+	int		keepalive_cnt;
+	int		keepalive_idle;
+	int		keepalive_intvl;
 	struct work_struct work;
 	struct sk_buff  *ooo_last_skb;
 	struct rb_root  out_of_order_queue;
diff --git a/net/mptcp/sockopt.c b/net/mptcp/sockopt.c
index d9f0d36e24ad..8cc30c9007b0 100644
--- a/net/mptcp/sockopt.c
+++ b/net/mptcp/sockopt.c
@@ -622,6 +622,31 @@ 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),
+				      int *msk_val, int val)
+{
+	struct mptcp_subflow_context *subflow;
+	int err = 0;
+
+	mptcp_for_each_subflow(msk, subflow) {
+		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
+		int ret;
+
+		lock_sock(ssk);
+		ret = set_val(ssk, val);
+		err = err ? : ret;
+		release_sock(ssk);
+	}
+
+	if (!err) {
+		*msk_val = val;
+		sockopt_seq_inc(msk);
+	}
+
+	return err;
+}
+
 static int __mptcp_setsockopt_sol_tcp_cork(struct mptcp_sock *msk, int val)
 {
 	struct mptcp_subflow_context *subflow;
@@ -818,6 +843,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,
+						 &msk->keepalive_cnt,
+						 val);
+		break;
 	default:
 		ret = -ENOPROTOOPT;
 	}
@@ -1332,6 +1373,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 +1395,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 +1522,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 v2 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/9016301142

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


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)

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

[Paolo Abeni]

On Thu, 2024-05-09 at 12:48 +0200, Matthieu Baerts (NGI0) wrote:
> 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>

LGTM, thanks!

Acked-by: Paolo Abeni <pabeni@redhat.com>

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

[Matthieu Baerts]

Hi Paolo,

On 10/05/2024 10:32, Paolo Abeni wrote:
> On Thu, 2024-05-09 at 12:48 +0200, Matthieu Baerts (NGI0) wrote:
>> 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.

(...)

>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> 
> LGTM, thanks!
> 
> Acked-by: Paolo Abeni <pabeni@redhat.com>

Thank you for the review!

Now in our tree (fixes for net-next, in preparation for the next batch):

New patches for t/upstream:
- 1f3df7457803: mptcp: SO_KEEPALIVE: fix getsockopt support
- f246f7a50963: mptcp: fix full TCP keep-alive support
- Results: 18d3f4f7094a..ea351593813d (export)

Tests are now in progress:

- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/2f64d8457b9c03ff3d78f3efb8da42f94ce7e277/checks

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