Patch 1 corrects the logic for MP_JOIN tests where 0 RSTs are expected.
Patch 2 ensures MPTCP packets are not incorrectly coalesced in the TCP backlog queue.
Patch 3 avoids a zero-window probe and associated WARN_ON_ONCE() in an expected MPTCP reinjection scenario.
Patches 4 & 5 allow an initial MPTCP subflow to be closed cleanly instead of always sending RST. Associated selftest is updated.
Signed-off-by: Mat Martineau martineau@kernel.org --- Geliang Tang (1): mptcp: avoid sending RST when closing the initial subflow
Matthieu Baerts (2): selftests: mptcp: join: correctly check for no RST selftests: mptcp: join: no RST when rm subflow/addr
Paolo Abeni (2): tcp: check mptcp-level constraints for backlog coalescing mptcp: more conservative check for zero probes
net/ipv4/tcp_ipv4.c | 1 + net/mptcp/protocol.c | 36 ++++++++++++++++--------- tools/testing/selftests/net/mptcp/mptcp_join.sh | 21 +++++++++++++-- 3 files changed, 43 insertions(+), 15 deletions(-) --- base-commit: 2915240eddba96b37de4c7e9a3d0ac6f9548454b change-id: 20231018-send-net-20231018-ac6b38df05e2
Best regards,
From: Matthieu Baerts matttbe@kernel.org
The commit mentioned below was more tolerant with the number of RST seen during a test because in some uncontrollable situations, multiple RST can be generated.
But it was not taking into account the case where no RST are expected: this validation was then no longer reporting issues for the 0 RST case because it is not possible to have less than 0 RST in the counter. This patch fixes the issue by adding a specific condition.
Fixes: 6bf41020b72b ("selftests: mptcp: update and extend fastclose test-cases") Cc: stable@vger.kernel.org Reviewed-by: Mat Martineau martineau@kernel.org Signed-off-by: Matthieu Baerts matttbe@kernel.org Signed-off-by: Mat Martineau martineau@kernel.org --- tools/testing/selftests/net/mptcp/mptcp_join.sh | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh index ee1f89a872b3..27953670206e 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh @@ -1432,7 +1432,9 @@ chk_rst_nr() count=$(get_counter ${ns_tx} "MPTcpExtMPRstTx") if [ -z "$count" ]; then print_skip - elif [ $count -lt $rst_tx ]; then + # accept more rst than expected except if we don't expect any + elif { [ $rst_tx -ne 0 ] && [ $count -lt $rst_tx ]; } || + { [ $rst_tx -eq 0 ] && [ $count -ne 0 ]; }; then fail_test "got $count MP_RST[s] TX expected $rst_tx" else print_ok @@ -1442,7 +1444,9 @@ chk_rst_nr() count=$(get_counter ${ns_rx} "MPTcpExtMPRstRx") if [ -z "$count" ]; then print_skip - elif [ "$count" -lt "$rst_rx" ]; then + # accept more rst than expected except if we don't expect any + elif { [ $rst_rx -ne 0 ] && [ $count -lt $rst_rx ]; } || + { [ $rst_rx -eq 0 ] && [ $count -ne 0 ]; }; then fail_test "got $count MP_RST[s] RX expected $rst_rx" else print_ok
From: Paolo Abeni pabeni@redhat.com
The MPTCP protocol can acquire the subflow-level socket lock and cause the tcp backlog usage. When inserting new skbs into the backlog, the stack will try to coalesce them.
Currently, we have no check in place to ensure that such coalescing will respect the MPTCP-level DSS, and that may cause data stream corruption, as reported by Christoph.
Address the issue by adding the relevant admission check for coalescing in tcp_add_backlog().
Note the issue is not easy to reproduce, as the MPTCP protocol tries hard to avoid acquiring the subflow-level socket lock.
Fixes: 648ef4b88673 ("mptcp: Implement MPTCP receive path") Cc: stable@vger.kernel.org Reported-by: Christoph Paasch cpaasch@apple.com Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/420 Reviewed-by: Mat Martineau martineau@kernel.org Signed-off-by: Paolo Abeni pabeni@redhat.com Signed-off-by: Mat Martineau martineau@kernel.org --- net/ipv4/tcp_ipv4.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 27140e5cdc06..4167e8a48b60 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -1869,6 +1869,7 @@ bool tcp_add_backlog(struct sock *sk, struct sk_buff *skb, #ifdef CONFIG_TLS_DEVICE tail->decrypted != skb->decrypted || #endif + !mptcp_skb_can_collapse(tail, skb) || thtail->doff != th->doff || memcmp(thtail + 1, th + 1, hdrlen - sizeof(*th))) goto no_coalesce;
From: Paolo Abeni pabeni@redhat.com
Christoph reported that the MPTCP protocol can find the subflow-level write queue unexpectedly not empty while crafting a zero-window probe, hitting a warning:
------------[ cut here ]------------ WARNING: CPU: 0 PID: 188 at net/mptcp/protocol.c:1312 mptcp_sendmsg_frag+0xc06/0xe70 Modules linked in: CPU: 0 PID: 188 Comm: kworker/0:2 Not tainted 6.6.0-rc2-g1176aa719d7a #47 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.11.0-2.el7 04/01/2014 Workqueue: events mptcp_worker RIP: 0010:mptcp_sendmsg_frag+0xc06/0xe70 net/mptcp/protocol.c:1312 RAX: 47d0530de347ff6a RBX: 47d0530de347ff6b RCX: ffff8881015d3c00 RDX: ffff8881015d3c00 RSI: 47d0530de347ff6b RDI: 47d0530de347ff6b RBP: 47d0530de347ff6b R08: ffffffff8243c6a8 R09: ffffffff82042d9c R10: 0000000000000002 R11: ffffffff82056850 R12: ffff88812a13d580 R13: 0000000000000001 R14: ffff88812b375e50 R15: ffff88812bbf3200 FS: 0000000000000000(0000) GS:ffff88813bc00000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000695118 CR3: 0000000115dfc001 CR4: 0000000000170ef0 Call Trace: <TASK> __subflow_push_pending+0xa4/0x420 net/mptcp/protocol.c:1545 __mptcp_push_pending+0x128/0x3b0 net/mptcp/protocol.c:1614 mptcp_release_cb+0x218/0x5b0 net/mptcp/protocol.c:3391 release_sock+0xf6/0x100 net/core/sock.c:3521 mptcp_worker+0x6e8/0x8f0 net/mptcp/protocol.c:2746 process_scheduled_works+0x341/0x690 kernel/workqueue.c:2630 worker_thread+0x3a7/0x610 kernel/workqueue.c:2784 kthread+0x143/0x180 kernel/kthread.c:388 ret_from_fork+0x4d/0x60 arch/x86/kernel/process.c:147 ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:304 </TASK>
The root cause of the issue is that expectations are wrong: e.g. due to MPTCP-level re-injection we can hit the critical condition.
Explicitly avoid the zero-window probe when the subflow write queue is not empty and drop the related warnings.
Reported-by: Christoph Paasch cpaasch@apple.com Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/444 Fixes: f70cad1085d1 ("mptcp: stop relying on tcp_tx_skb_cache") Cc: stable@vger.kernel.org Reviewed-by: Mat Martineau martineau@kernel.org Signed-off-by: Paolo Abeni pabeni@redhat.com Signed-off-by: Mat Martineau martineau@kernel.org --- net/mptcp/protocol.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index d1902373c974..4e30e5ba3795 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -1298,7 +1298,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk, if (copy == 0) { u64 snd_una = READ_ONCE(msk->snd_una);
- if (snd_una != msk->snd_nxt) { + if (snd_una != msk->snd_nxt || tcp_write_queue_tail(ssk)) { tcp_remove_empty_skb(ssk); return 0; } @@ -1306,11 +1306,6 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk, zero_window_probe = true; data_seq = snd_una - 1; copy = 1; - - /* all mptcp-level data is acked, no skbs should be present into the - * ssk write queue - */ - WARN_ON_ONCE(reuse_skb); }
copy = min_t(size_t, copy, info->limit - info->sent); @@ -1339,7 +1334,6 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk, if (reuse_skb) { TCP_SKB_CB(skb)->tcp_flags &= ~TCPHDR_PSH; mpext->data_len += copy; - WARN_ON_ONCE(zero_window_probe); goto out; }
From: Geliang Tang geliang.tang@suse.com
When closing the first subflow, the MPTCP protocol unconditionally calls tcp_disconnect(), which in turn generates a reset if the subflow is established.
That is unexpected and different from what MPTCP does with MPJ subflows, where resets are generated only on FASTCLOSE and other edge scenarios.
We can't reuse for the first subflow the same code in place for MPJ subflows, as MPTCP clean them up completely via a tcp_close() call, while must keep the first subflow socket alive for later re-usage, due to implementation constraints.
This patch adds a new helper __mptcp_subflow_disconnect() that encapsulates, a logic similar to tcp_close, issuing a reset only when the MPTCP_CF_FASTCLOSE flag is set, and performing a clean shutdown otherwise.
Fixes: c2b2ae3925b6 ("mptcp: handle correctly disconnect() failures") Cc: stable@vger.kernel.org Reviewed-by: Matthieu Baerts matttbe@kernel.org Co-developed-by: Paolo Abeni pabeni@redhat.com Signed-off-by: Paolo Abeni pabeni@redhat.com Signed-off-by: Geliang Tang geliang.tang@suse.com Signed-off-by: Mat Martineau martineau@kernel.org --- net/mptcp/protocol.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 4e30e5ba3795..886ab689a8ae 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -2348,6 +2348,26 @@ bool __mptcp_retransmit_pending_data(struct sock *sk) #define MPTCP_CF_PUSH BIT(1) #define MPTCP_CF_FASTCLOSE BIT(2)
+/* be sure to send a reset only if the caller asked for it, also + * clean completely the subflow status when the subflow reaches + * TCP_CLOSE state + */ +static void __mptcp_subflow_disconnect(struct sock *ssk, + struct mptcp_subflow_context *subflow, + unsigned int flags) +{ + if (((1 << ssk->sk_state) & (TCPF_CLOSE | TCPF_LISTEN)) || + (flags & MPTCP_CF_FASTCLOSE)) { + /* The MPTCP code never wait on the subflow sockets, TCP-level + * disconnect should never fail + */ + WARN_ON_ONCE(tcp_disconnect(ssk, 0)); + mptcp_subflow_ctx_reset(subflow); + } else { + tcp_shutdown(ssk, SEND_SHUTDOWN); + } +} + /* subflow sockets can be either outgoing (connect) or incoming * (accept). * @@ -2385,7 +2405,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk, lock_sock_nested(ssk, SINGLE_DEPTH_NESTING);
if ((flags & MPTCP_CF_FASTCLOSE) && !__mptcp_check_fallback(msk)) { - /* be sure to force the tcp_disconnect() path, + /* be sure to force the tcp_close path * to generate the egress reset */ ssk->sk_lingertime = 0; @@ -2395,11 +2415,7 @@ static void __mptcp_close_ssk(struct sock *sk, struct sock *ssk,
need_push = (flags & MPTCP_CF_PUSH) && __mptcp_retransmit_pending_data(sk); if (!dispose_it) { - /* The MPTCP code never wait on the subflow sockets, TCP-level - * disconnect should never fail - */ - WARN_ON_ONCE(tcp_disconnect(ssk, 0)); - mptcp_subflow_ctx_reset(subflow); + __mptcp_subflow_disconnect(ssk, subflow, flags); release_sock(ssk);
goto out;
From: Matthieu Baerts matttbe@kernel.org
Recently, we noticed that some RST were wrongly generated when removing the initial subflow.
This patch makes sure RST are not sent when removing any subflows or any addresses.
Fixes: c2b2ae3925b6 ("mptcp: handle correctly disconnect() failures") Cc: stable@vger.kernel.org Acked-by: Paolo Abeni pabeni@redhat.com Signed-off-by: Matthieu Baerts matttbe@kernel.org Signed-off-by: Mat Martineau martineau@kernel.org --- tools/testing/selftests/net/mptcp/mptcp_join.sh | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh index 27953670206e..dc895b7b94e1 100755 --- a/tools/testing/selftests/net/mptcp/mptcp_join.sh +++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh @@ -2309,6 +2309,7 @@ remove_tests() chk_join_nr 1 1 1 chk_rm_tx_nr 1 chk_rm_nr 1 1 + chk_rst_nr 0 0 fi
# multiple subflows, remove @@ -2321,6 +2322,7 @@ remove_tests() run_tests $ns1 $ns2 10.0.1.1 chk_join_nr 2 2 2 chk_rm_nr 2 2 + chk_rst_nr 0 0 fi
# single address, remove @@ -2333,6 +2335,7 @@ remove_tests() chk_join_nr 1 1 1 chk_add_nr 1 1 chk_rm_nr 1 1 invert + chk_rst_nr 0 0 fi
# subflow and signal, remove @@ -2346,6 +2349,7 @@ remove_tests() chk_join_nr 2 2 2 chk_add_nr 1 1 chk_rm_nr 1 1 + chk_rst_nr 0 0 fi
# subflows and signal, remove @@ -2360,6 +2364,7 @@ remove_tests() chk_join_nr 3 3 3 chk_add_nr 1 1 chk_rm_nr 2 2 + chk_rst_nr 0 0 fi
# addresses remove @@ -2374,6 +2379,7 @@ remove_tests() chk_join_nr 3 3 3 chk_add_nr 3 3 chk_rm_nr 3 3 invert + chk_rst_nr 0 0 fi
# invalid addresses remove @@ -2388,6 +2394,7 @@ remove_tests() chk_join_nr 1 1 1 chk_add_nr 3 3 chk_rm_nr 3 1 invert + chk_rst_nr 0 0 fi
# subflows and signal, flush @@ -2402,6 +2409,7 @@ remove_tests() chk_join_nr 3 3 3 chk_add_nr 1 1 chk_rm_nr 1 3 invert simult + chk_rst_nr 0 0 fi
# subflows flush @@ -2421,6 +2429,7 @@ remove_tests() else chk_rm_nr 3 3 fi + chk_rst_nr 0 0 fi
# addresses flush @@ -2435,6 +2444,7 @@ remove_tests() chk_join_nr 3 3 3 chk_add_nr 3 3 chk_rm_nr 3 3 invert simult + chk_rst_nr 0 0 fi
# invalid addresses flush @@ -2449,6 +2459,7 @@ remove_tests() chk_join_nr 1 1 1 chk_add_nr 3 3 chk_rm_nr 3 1 invert + chk_rst_nr 0 0 fi
# remove id 0 subflow @@ -2460,6 +2471,7 @@ remove_tests() run_tests $ns1 $ns2 10.0.1.1 chk_join_nr 1 1 1 chk_rm_nr 1 1 + chk_rst_nr 0 0 fi
# remove id 0 address @@ -2472,6 +2484,7 @@ remove_tests() chk_join_nr 1 1 1 chk_add_nr 1 1 chk_rm_nr 1 1 invert + chk_rst_nr 0 0 invert fi }
Hello:
This series was applied to netdev/net.git (main) by Jakub Kicinski kuba@kernel.org:
On Wed, 18 Oct 2023 11:23:51 -0700 you wrote:
Patch 1 corrects the logic for MP_JOIN tests where 0 RSTs are expected.
Patch 2 ensures MPTCP packets are not incorrectly coalesced in the TCP backlog queue.
Patch 3 avoids a zero-window probe and associated WARN_ON_ONCE() in an expected MPTCP reinjection scenario.
[...]
Here is the summary with links: - [net,1/5] selftests: mptcp: join: correctly check for no RST https://git.kernel.org/netdev/net/c/b134a5805455 - [net,2/5] tcp: check mptcp-level constraints for backlog coalescing https://git.kernel.org/netdev/net/c/6db8a37dfc54 - [net,3/5] mptcp: more conservative check for zero probes https://git.kernel.org/netdev/net/c/72377ab2d671 - [net,4/5] mptcp: avoid sending RST when closing the initial subflow https://git.kernel.org/netdev/net/c/14c56686a64c - [net,5/5] selftests: mptcp: join: no RST when rm subflow/addr https://git.kernel.org/netdev/net/c/2cfaa8b3b7ae
You are awesome, thank you!
linux-stable-mirror@lists.linaro.org
-
Mat Martineau -
patchwork-bot+netdevbpf＠kernel.org