mptcp: fix sleep in atomic at close time

[PATCH mptcp-net] mptcp: fix sleep in atomic at close time

Posted by Paolo Abeni 1 year, 4 months ago

Mat reported a splat at msk close time:

BUG: sleeping function called from invalid context at net/mptcp/protocol.c:2877
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 155, name: packetdrill
preempt_count: 201, expected: 0
RCU nest depth: 0, expected: 0
4 locks held by packetdrill/155:
#0: ffff888001536990 (&sb->s_type->i_mutex_key#6){+.+.}-{3:3}, at: __sock_release (net/socket.c:650)
#1: ffff88800b498130 (sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_close (net/mptcp/protocol.c:2973)
#2: ffff88800b49a130 (sk_lock-AF_INET/1){+.+.}-{0:0}, at: __mptcp_close_ssk (net/mptcp/protocol.c:2363)
#3: ffff88800b49a0b0 (slock-AF_INET){+...}-{2:2}, at: __lock_sock_fast (include/net/sock.h:1820)
Preemption disabled at:
0x0
CPU: 1 PID: 155 Comm: packetdrill Not tainted 6.1.0-rc5 #365
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
Call Trace:
<TASK>
dump_stack_lvl (lib/dump_stack.c:107 (discriminator 4))
__might_resched.cold (kernel/sched/core.c:9891)
__mptcp_destroy_sock (include/linux/kernel.h:110)
__mptcp_close (net/mptcp/protocol.c:2959)
mptcp_subflow_queue_clean (include/net/sock.h:1777)
__mptcp_close_ssk (net/mptcp/protocol.c:2363)
mptcp_destroy_common (net/mptcp/protocol.c:3170)
mptcp_destroy (include/net/sock.h:1495)
__mptcp_destroy_sock (net/mptcp/protocol.c:2886)
__mptcp_close (net/mptcp/protocol.c:2959)
mptcp_close (net/mptcp/protocol.c:2974)
inet_release (net/ipv4/af_inet.c:432)
__sock_release (net/socket.c:651)
sock_close (net/socket.c:1367)
__fput (fs/file_table.c:320)
task_work_run (kernel/task_work.c:181 (discriminator 1))
exit_to_user_mode_prepare (include/linux/resume_user_mode.h:49)
syscall_exit_to_user_mode (kernel/entry/common.c:130)
do_syscall_64 (arch/x86/entry/common.c:87)
entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)

We can't acquire call mptcp_close under the 'fast' socket lock veriant,
replace it with a sock_lock_nested() as the relevant code is already
under the listening msk socket lock protection.

Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Closes: Matthieu Baerts <matthieu.baerts@tessares.net>
Fixes: 30e51b923e43 ("mptcp: fix unreleased socket in accept queue")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/subflow.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 05099b3760b5..602cb2fe5148 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1802,16 +1802,16 @@ void mptcp_subflow_queue_clean(struct sock *listener_ssk)
 
 	for (msk = head; msk; msk = next) {
 		struct sock *sk = (struct sock *)msk;
-		bool slow, do_cancel_work;
+		bool do_cancel_work;
 
 		sock_hold(sk);
-		slow = lock_sock_fast_nested(sk);
+		lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
 		next = msk->dl_next;
 		msk->first = NULL;
 		msk->dl_next = NULL;
 
 		do_cancel_work = __mptcp_close(sk, 0);
-		unlock_sock_fast(sk, slow);
+		release_sock(sk);
 		if (do_cancel_work)
 			mptcp_cancel_work(sk);
 		sock_put(sk);
-- 
2.38.1

Re: [PATCH mptcp-net] mptcp: fix sleep in atomic at close time

Posted by Matthieu Baerts 1 year, 4 months ago

Hi Paolo,

On 23/11/2022 12:23, Paolo Abeni wrote:
> Mat reported a splat at msk close time:

(...)

> We can't acquire call mptcp_close under the 'fast' socket lock veriant,
> replace it with a sock_lock_nested() as the relevant code is already
> under the listening msk socket lock protection.

Thank you for the patch!

Now in our tree (fix for -net) with my RvB tag (and the 3 other small
fixes mentioned in my previous email)

New patches for t/upstream-net:
- ff05017f41a8: mptcp: fix sleep in atomic at close time
- Results: 4b0094499418..515e95912b40 (export-net)
- Results: 3d12c8bc8c2c..e97e3cfdc32b (export)

Tests are now in progress:

https://cirrus-ci.com/github/multipath-tcp/mptcp_net-next/export/20221123T164310

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-net] mptcp: fix sleep in atomic at close time

Posted by Matthieu Baerts 1 year, 4 months ago

Hi Paolo,

Thank you for the patch!

On 23/11/2022 12:23, Paolo Abeni wrote:
> Mat reported a splat at msk close time:
> 
> BUG: sleeping function called from invalid context at net/mptcp/protocol.c:2877
> in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 155, name: packetdrill
> preempt_count: 201, expected: 0
> RCU nest depth: 0, expected: 0
> 4 locks held by packetdrill/155:
> #0: ffff888001536990 (&sb->s_type->i_mutex_key#6){+.+.}-{3:3}, at: __sock_release (net/socket.c:650)
> #1: ffff88800b498130 (sk_lock-AF_INET){+.+.}-{0:0}, at: mptcp_close (net/mptcp/protocol.c:2973)
> #2: ffff88800b49a130 (sk_lock-AF_INET/1){+.+.}-{0:0}, at: __mptcp_close_ssk (net/mptcp/protocol.c:2363)
> #3: ffff88800b49a0b0 (slock-AF_INET){+...}-{2:2}, at: __lock_sock_fast (include/net/sock.h:1820)
> Preemption disabled at:
> 0x0
> CPU: 1 PID: 155 Comm: packetdrill Not tainted 6.1.0-rc5 #365
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.15.0-1 04/01/2014
> Call Trace:
> <TASK>
> dump_stack_lvl (lib/dump_stack.c:107 (discriminator 4))
> __might_resched.cold (kernel/sched/core.c:9891)
> __mptcp_destroy_sock (include/linux/kernel.h:110)
> __mptcp_close (net/mptcp/protocol.c:2959)
> mptcp_subflow_queue_clean (include/net/sock.h:1777)
> __mptcp_close_ssk (net/mptcp/protocol.c:2363)
> mptcp_destroy_common (net/mptcp/protocol.c:3170)
> mptcp_destroy (include/net/sock.h:1495)
> __mptcp_destroy_sock (net/mptcp/protocol.c:2886)
> __mptcp_close (net/mptcp/protocol.c:2959)
> mptcp_close (net/mptcp/protocol.c:2974)
> inet_release (net/ipv4/af_inet.c:432)
> __sock_release (net/socket.c:651)
> sock_close (net/socket.c:1367)
> __fput (fs/file_table.c:320)
> task_work_run (kernel/task_work.c:181 (discriminator 1))
> exit_to_user_mode_prepare (include/linux/resume_user_mode.h:49)
> syscall_exit_to_user_mode (kernel/entry/common.c:130)
> do_syscall_64 (arch/x86/entry/common.c:87)
> entry_SYSCALL_64_after_hwframe (arch/x86/entry/entry_64.S:120)
> 
> We can't acquire call mptcp_close under the 'fast' socket lock veriant,

I guess I can remove "acquire" (and s/veriant/variant/)

> replace it with a sock_lock_nested() as the relevant code is already
> under the listening msk socket lock protection.
> 
> Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> Closes: Matthieu Baerts <matthieu.baerts@tessares.net>

Please don't close me, it took me some times to find how to re-open
myself...

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/316

For the rest, it looks good to me!

Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-net] mptcp: fix sleep in atomic at close time

Posted by Paolo Abeni 1 year, 4 months ago

On Wed, 2022-11-23 at 16:36 +0100, Matthieu Baerts wrote:
> > Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> > Closes: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
> Please don't close me, it took me some times to find how to re-open
> myself...

What can I say? at I list did not accidently close the b&b ;)

> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/316
> 
> For the rest, it looks good to me!
> 
> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>

feel free to add even your tested-by tag, as you already did that :)
and we can produce same monster tag like 'reported-reviewed-and-tested-
by: ...' ;)

Or do you prefer I'll repost?

Thanks!

/P

Re: [PATCH mptcp-net] mptcp: fix sleep in atomic at close time

Posted by Matthieu Baerts 1 year, 4 months ago

On 23/11/2022 16:42, Paolo Abeni wrote:
> On Wed, 2022-11-23 at 16:36 +0100, Matthieu Baerts wrote:
>>> Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
>>> Closes: Matthieu Baerts <matthieu.baerts@tessares.net>
>>
>> Please don't close me, it took me some times to find how to re-open
>> myself...
> 
> What can I say? at I list did not accidently close the b&b ;)

:-P

>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/316
>>
>> For the rest, it looks good to me!
>>
>> Reviewed-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> 
> feel free to add even your tested-by tag, as you already did that :)
> and we can produce same monster tag like 'reported-reviewed-and-tested-
> by: ...' ;)

I guess it is better to avoid them :)
We don't need my Tested-by but I will keep the Reported-by just to
understand to which "Mat" you are talking about at the beginning ;)

> Or do you prefer I'll repost?

No, thank you, I can of course do the modifications when applying the patch!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net