* [PATCH mptcp-net] mptcp: fix possible deadlock in subflow_error_report
@ 2023-02-20 14:27 Paolo Abeni
2023-02-20 15:32 ` mptcp: fix possible deadlock in subflow_error_report: Tests Results MPTCP CI
2023-02-20 16:05 ` [PATCH mptcp-net] mptcp: fix possible deadlock in subflow_error_report Matthieu Baerts
0 siblings, 2 replies; 3+ messages in thread
From: Paolo Abeni @ 2023-02-20 14:27 UTC (permalink / raw
To: mptcp
Christoph reported a possible deadlock while the TCP stack
destroys an unaccepted subflow due to an incoming reset: the
MPTCP socket error path tries to acquire the msk-level socket
lock while TCP still owns the listener socket accept queue
spinlock, and the reverse dependency already exists in the
TCP stack.
Note that the above is actually a lockdep false positive, as
the chain involves two separate sockets. A different per-socket
lockdep key will address the issue, but such a change will be
quite invasive.
Instead, we can simply stop earlier the socket error handling
for orphaned or unaccepted subflows, breaking the critical
lockdep chain. Error handling in such a scenario is a no-op.
Reported-and-tested-by: Christoph Paasch <cpaasch@apple.com>
Fixes: 15cc10453398 ("mptcp: deliver ssk errors to msk")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/355
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
Note that the fixes tag above is different from the bisected, much
more recent, commit pointed by syzkaller, but I believe (code
inspection shows) that the DL scenario is present since it.
---
net/mptcp/subflow.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 8b46311b8d5e..a14c3a0c7c00 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1437,6 +1437,13 @@ static void subflow_error_report(struct sock *ssk)
{
struct sock *sk = mptcp_subflow_ctx(ssk)->conn;
+ /* bail early if this is a no-op, so that we avoid introducing a
+ * problematic lockdep dependency between TCP accept queue lock
+ * and msk socket spinlock
+ */
+ if (!sk->sk_socket)
+ return;
+
mptcp_data_lock(sk);
if (!sock_owned_by_user(sk))
__mptcp_error_report(sk);
--
2.39.2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: mptcp: fix possible deadlock in subflow_error_report: Tests Results
2023-02-20 14:27 [PATCH mptcp-net] mptcp: fix possible deadlock in subflow_error_report Paolo Abeni
@ 2023-02-20 15:32 ` MPTCP CI
2023-02-20 16:05 ` [PATCH mptcp-net] mptcp: fix possible deadlock in subflow_error_report Matthieu Baerts
1 sibling, 0 replies; 3+ messages in thread
From: MPTCP CI @ 2023-02-20 15:32 UTC (permalink / raw
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5587182899953664
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5587182899953664/summary/summary.txt
- KVM Validation: normal (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/6713082806796288
- Summary: https://api.cirrus-ci.com/v1/artifact/task/6713082806796288/summary/summary.txt
- KVM Validation: debug (except selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/4566836109377536
- Summary: https://api.cirrus-ci.com/v1/artifact/task/4566836109377536/summary/summary.txt
- KVM Validation: debug (only selftest_mptcp_join):
- Success! ✅:
- Task: https://cirrus-ci.com/task/5692736016220160
- Summary: https://api.cirrus-ci.com/v1/artifact/task/5692736016220160/summary/summary.txt
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/38e4dd7859de
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-debug
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 (Tessares)
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH mptcp-net] mptcp: fix possible deadlock in subflow_error_report
2023-02-20 14:27 [PATCH mptcp-net] mptcp: fix possible deadlock in subflow_error_report Paolo Abeni
2023-02-20 15:32 ` mptcp: fix possible deadlock in subflow_error_report: Tests Results MPTCP CI
@ 2023-02-20 16:05 ` Matthieu Baerts
1 sibling, 0 replies; 3+ messages in thread
From: Matthieu Baerts @ 2023-02-20 16:05 UTC (permalink / raw
To: Paolo Abeni, mptcp
Hi Paolo,
On 20/02/2023 15:27, Paolo Abeni wrote:
> Christoph reported a possible deadlock while the TCP stack
> destroys an unaccepted subflow due to an incoming reset: the
> MPTCP socket error path tries to acquire the msk-level socket
> lock while TCP still owns the listener socket accept queue
> spinlock, and the reverse dependency already exists in the
> TCP stack.
>
> Note that the above is actually a lockdep false positive, as
> the chain involves two separate sockets. A different per-socket
> lockdep key will address the issue, but such a change will be
> quite invasive.
>
> Instead, we can simply stop earlier the socket error handling
> for orphaned or unaccepted subflows, breaking the critical
> lockdep chain. Error handling in such a scenario is a no-op.
>
> Reported-and-tested-by: Christoph Paasch <cpaasch@apple.com>
> Fixes: 15cc10453398 ("mptcp: deliver ssk errors to msk")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/355
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> Note that the fixes tag above is different from the bisected, much
> more recent, commit pointed by syzkaller, but I believe (code
> inspection shows) that the DL scenario is present since it.
Thank you for the patch and the note here!
Just applied in our tree (fixes for -net) with my RvB tag:
New patches for t/upstream-net:
- b06fdd2ba8a7: mptcp: fix possible deadlock in subflow_error_report
- Results: 1231bdd38564..275b1fab043a (export-net)
- Results: 1e2b17c72708..ce3a45c82f32 (export)
Tests are now in progress:
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export-net/20230220T160207
https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20230220T160207
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-02-20 16:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-20 14:27 [PATCH mptcp-net] mptcp: fix possible deadlock in subflow_error_report Paolo Abeni
2023-02-20 15:32 ` mptcp: fix possible deadlock in subflow_error_report: Tests Results MPTCP CI
2023-02-20 16:05 ` [PATCH mptcp-net] mptcp: fix possible deadlock in subflow_error_report Matthieu Baerts
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.