* [PATCH mptcp-net v2 1/2] mptcp: SO_KEEPALIVE: fix getsockopt support
2024-05-09 10:48 [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts Matthieu Baerts (NGI0)
@ 2024-05-09 10:48 ` Matthieu Baerts (NGI0)
2024-05-09 10:48 ` [PATCH mptcp-net v2 2/2] mptcp: fix full TCP keep-alive support Matthieu Baerts (NGI0)
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-05-09 10:48 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH mptcp-net v2 2/2] mptcp: fix full TCP keep-alive support
2024-05-09 10:48 [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts Matthieu Baerts (NGI0)
2024-05-09 10:48 ` [PATCH mptcp-net v2 1/2] mptcp: SO_KEEPALIVE: fix getsockopt support Matthieu Baerts (NGI0)
@ 2024-05-09 10:48 ` Matthieu Baerts (NGI0)
2024-05-09 11:39 ` [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts MPTCP CI
2024-05-10 8:32 ` Paolo Abeni
3 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts (NGI0) @ 2024-05-09 10:48 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts
2024-05-09 10:48 [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts Matthieu Baerts (NGI0)
2024-05-09 10:48 ` [PATCH mptcp-net v2 1/2] mptcp: SO_KEEPALIVE: fix getsockopt support Matthieu Baerts (NGI0)
2024-05-09 10:48 ` [PATCH mptcp-net v2 2/2] mptcp: fix full TCP keep-alive support Matthieu Baerts (NGI0)
@ 2024-05-09 11:39 ` MPTCP CI
2024-05-10 8:32 ` Paolo Abeni
3 siblings, 0 replies; 6+ messages in thread
From: MPTCP CI @ 2024-05-09 11:39 UTC (permalink / raw)
To: Matthieu Baerts; +Cc: mptcp
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)
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts
2024-05-09 10:48 [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts Matthieu Baerts (NGI0)
` (2 preceding siblings ...)
2024-05-09 11:39 ` [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts MPTCP CI
@ 2024-05-10 8:32 ` Paolo Abeni
2024-05-10 8:52 ` Matthieu Baerts
3 siblings, 1 reply; 6+ messages in thread
From: Paolo Abeni @ 2024-05-10 8:32 UTC (permalink / raw)
To: Matthieu Baerts (NGI0), mptcp
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>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH mptcp-net v2 0/2] mptcp: getsockopt(SO_KEEPALIVE) and TCP_KEEP* sockopts
2024-05-10 8:32 ` Paolo Abeni
@ 2024-05-10 8:52 ` Matthieu Baerts
0 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2024-05-10 8:52 UTC (permalink / raw)
To: Paolo Abeni, mptcp
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.
^ permalink raw reply [flat|nested] 6+ messages in thread