WARNING: refcount bug in inet_twsk_kill

Hello,

syzbot found the following issue on:

HEAD commit:    33e02dc69afb Merge tag 'sound-6.10-rc1' of git://git.kerne..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=117f3182980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=25544a2faf4bae65
dashboard link: https://syzkaller.appspot.com/bug?extid=8ea26396ff85d23a8929
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7bc7510fe41f/non_bootable_disk-33e02dc6.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/573c88ac3233/vmlinux-33e02dc6.xz
kernel image: https://storage.googleapis.com/syzbot-assets/760a52b9a00a/bzImage-33e02dc6.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:

Reported-by [email protected]

------------[ cut here ]------------
refcount_t: decrement hit 0; leaking memory.
WARNING: CPU: 3 PID: 1396 at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210 lib/refcount.c:31

Modules linked in:
CPU: 3 PID: 1396 Comm: syz-executor.3 Not tainted 6.9.0-syzkaller-07370-g33e02dc69afb #0
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:refcount_warn_saturate+0x1ed/0x210 lib/refcount.c:31
Code: 8b e8 37 85 cf fc 90 0f 0b 90 90 e9 c3 fe ff ff e8 68 34 0d fd c6 05 0d 81 4c 0b 01 90 48 c7 c7 20 2b 8f 8b e8 14 85 cf fc 90 <0f> 0b 90 90 e9 a0 fe ff ff 48 89 ef e8 e2 e8 68 fd e9 44 fe ff ff
RSP: 0018:ffffc9000480fa70 EFLAGS: 00010282
RAX: 0000000000000000 RBX: 0000000000000000 RCX: ffffc9002ce28000
RDX: 0000000000040000 RSI: ffffffff81505406 RDI: 0000000000000001
RBP: ffff88804d8b3f80 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000002 R12: ffff88804d8b3f80
R13: ffff888031c601c0 R14: ffffc900013c04f8 R15: 000000002a3e5567
FS:  00007f56d897c6c0(0000) GS:ffff88806b300000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000001b3182b000 CR3: 0000000034ed6000 CR4: 0000000000350ef0
Call Trace:
 <TASK>
 __refcount_dec include/linux/refcount.h:336 [inline]
 refcount_dec include/linux/refcount.h:351 [inline]
 inet_twsk_kill+0x758/0x9c0 net/ipv4/inet_timewait_sock.c:70
 inet_twsk_deschedule_put net/ipv4/inet_timewait_sock.c:221 [inline]
 inet_twsk_purge+0x725/0x890 net/ipv4/inet_timewait_sock.c:304
 tcp_twsk_purge+0x115/0x150 net/ipv4/tcp_minisocks.c:402
 tcp_sk_exit_batch+0x1c/0x170 net/ipv4/tcp_ipv4.c:3522
 ops_exit_list+0x128/0x180 net/core/net_namespace.c:178
 setup_net+0x714/0xb40 net/core/net_namespace.c:375
 copy_net_ns+0x2f0/0x670 net/core/net_namespace.c:508
 create_new_namespaces+0x3ea/0xb10 kernel/nsproxy.c:110
 unshare_nsproxy_namespaces+0xc0/0x1f0 kernel/nsproxy.c:228
 ksys_unshare+0x419/0x970 kernel/fork.c:3323
 __do_sys_unshare kernel/fork.c:3394 [inline]
 __se_sys_unshare kernel/fork.c:3392 [inline]
 __x64_sys_unshare+0x31/0x40 kernel/fork.c:3392
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcf/0x260 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f56d7c7cee9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 20 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f56d897c0c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000110
RAX: ffffffffffffffda RBX: 00007f56d7dac1f0 RCX: 00007f56d7c7cee9
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000042000000
RBP: 00007f56d7cc949e R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 000000000000006e R14: 00007f56d7dac1f0 R15: 00007ffe66454be8
 </TASK>

From: syzbot <[email protected]>
Date: Sat, 10 Aug 2024 18:29:20 -0700

Quoted 18 lines

Hello,

syzbot found the following issue on:

HEAD commit:    33e02dc69afb Merge tag 'sound-6.10-rc1' of git://git.kerne..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=117f3182980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=25544a2faf4bae65
dashboard link: https://syzkaller.appspot.com/bug?extid=8ea26396ff85d23a8929
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7bc7510fe41f/non_bootable_disk-33e02dc6.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/573c88ac3233/vmlinux-33e02dc6.xz
kernel image: https://storage.googleapis.com/syzbot-assets/760a52b9a00a/bzImage-33e02dc6.xz

Quoted 6 lines

IMPORTANT: if you fix the issue, please add the following tag to the commit:

Reported-by [email protected]

------------[ cut here ]------------
refcount_t: decrement hit 0; leaking memory.
WARNING: CPU: 3 PID: 1396 at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210 lib/refcount.c:31

Eric, this is the weird report I was talking about at netdevconf :)

It seems refcount_dec(&tw->tw_dr->tw_refcount) is somehow done earlier than refcount_inc().

I started to see the same splat at a very low rate after consuming commit b334b924c9b7 ("net: tcp/dccp: prepare for tw_timer un-pinning").

The commit a bit deferred refcount_inc(tw_refcount) after the hash dance, so twsk is now visible before tw_dr->tw_refcount is incremented.

I came up with the diff below but was suspecting a bug in another place, possibly QEMU, so I haven't posted the diff officially.

refcount_inc() was actually deferred, but it's still under an ehash lock, and inet_twsk_deschedule_put() must be serialised with the same ehash lock. Even inet_twsk_kill() performs the ehash lock dance before calling refcount_dec().

So, it should be impossible that refcount_inc() is not visible after double lock/unlock and before refcount_dec(), so this report looks bogus to me :S

From: syzbot <[email protected]>
Date: Sat, 10 Aug 2024 18:29:20 -0700

Quoted 18 lines

Hello,

syzbot found the following issue on:

HEAD commit:    33e02dc69afb Merge tag 'sound-6.10-rc1' of git://git.kerne..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=117f3182980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=25544a2faf4bae65
dashboard link: https://syzkaller.appspot.com/bug?extid=8ea26396ff85d23a8929
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7bc7510fe41f/non_bootable_disk-33e02dc6.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/573c88ac3233/vmlinux-33e02dc6.xz
kernel image: https://storage.googleapis.com/syzbot-assets/760a52b9a00a/bzImage-33e02dc6.xz

Quoted 6 lines

IMPORTANT: if you fix the issue, please add the following tag to the commit:

Reported-by [email protected]

------------[ cut here ]------------
refcount_t: decrement hit 0; leaking memory.
WARNING: CPU: 3 PID: 1396 at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210 lib/refcount.c:31

Eric, this is the weird report I was talking about at netdevconf :)

It seems refcount_dec(&tw->tw_dr->tw_refcount) is somehow done earlier than refcount_inc().

I started to see the same splat at a very low rate after consuming commit b334b924c9b7 ("net: tcp/dccp: prepare for tw_timer un-pinning").

The commit a bit deferred refcount_inc(tw_refcount) after the hash dance, so twsk is now visible before tw_dr->tw_refcount is incremented.

I came up with the diff below but was suspecting a bug in another place, possibly QEMU, so I haven't posted the diff officially.

refcount_inc() was actually deferred, but it's still under an ehash lock,

Quoted 4 lines

I came up with the diff below but was suspecting a bug in another place, possibly QEMU, so I haven't posted the diff officially.

refcount_inc() was actually deferred, but it's still under an ehash lock,

but different struct inet_hashinfo, so the locks don't help :/

https://syzkaller.appspot.com/x/log.txt?x=117f3182980000

... shows at two cores racing:

[ 3127.234402][ T1396] CPU: 3 PID: 1396 Comm: syz-executor.3 Not
and
[ 3127.257864][   T13] CPU: 1 PID: 13 Comm: kworker/u32:1 Not tainted 6.9.0-syzkalle (netns cleanup net).

first splat backtrace shows invocation of tcp_sk_exit_batch() from netns error unwinding code.

Second one lacks backtrace, but its also in tcp_sk_exit_batch(),

Quoted 9 lines · Florian Westphal

https://syzkaller.appspot.com/x/log.txt?x=117f3182980000

... shows at two cores racing:

[ 3127.234402][ T1396] CPU: 3 PID: 1396 Comm: syz-executor.3 Not
and
[ 3127.257864][   T13] CPU: 1 PID: 13 Comm: kworker/u32:1 Not tainted 6.9.0-syzkalle (netns cleanup net).

Quoted 4 lines

first splat backtrace shows invocation of tcp_sk_exit_batch() from netns error unwinding code.

Second one lacks backtrace, but its also in tcp_sk_exit_batch(),

... which doesn't work. Does this look like a plausible theory/exlanation?

Given:
1 exiting netns, has >= 1 tw sk.
1 (unrelated) netns that failed in setup_net

... we run into following race:

exiting netns, from cleanup wq, calls tcp_sk_exit_batch(), which calls inet_twsk_purge(&tcp_hashinfo).

At same time, from error unwinding code, we also call tcp_sk_exit_batch().

Both threads walk tcp_hashinfo ehash buckets.

From work queue (normal netns exit path), we hit

303 if (state == TCP_TIME_WAIT) {
304 inet_twsk_deschedule_put(inet_twsk(sk));

Because both threads operate on tcp_hashinfo, the unrelated struct net (exiting net) is also visible to error-unwinding thread.

So, error unwinding code will call

303 if (state == TCP_TIME_WAIT) {
304 inet_twsk_deschedule_put(inet_twsk(sk));

for the same tw sk and both threads do

218 void inet_twsk_deschedule_put(struct inet_timewait_sock *tw) 219 {
220 if (del_timer_sync(&tw->tw_timer))
221 inet_twsk_kill(tw);

Error unwind path cancel timer, calls inet_twsk_kill, while work queue sees timer as already shut-down so it ends up returning to tcp_sk_exit_batch(), where it will WARN here:

  WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

... because the supposedly-last tw_refcount decrement did not drop it down to 0.

Meanwhile, error unwiding thread calls refcount_dec() on tw_refcount, which now drops down to 0 instead of 1, which provides another warn splat.

I'll ponder on ways to fix this tomorrow unless someone else already has better theory/solution.

From: Florian Westphal <[email protected]>
Date: Sun, 11 Aug 2024 18:28:50 +0200

Quoted 10 lines

Quoted 9 lines · Florian Westphal

https://syzkaller.appspot.com/x/log.txt?x=117f3182980000

... shows at two cores racing:

[ 3127.234402][ T1396] CPU: 3 PID: 1396 Comm: syz-executor.3 Not
and
[ 3127.257864][   T13] CPU: 1 PID: 13 Comm: kworker/u32:1 Not tainted 6.9.0-syzkalle (netns cleanup net).

Quoted 7 lines

Quoted 4 lines

first splat backtrace shows invocation of tcp_sk_exit_batch() from netns error unwinding code.

Second one lacks backtrace, but its also in tcp_sk_exit_batch(),

... which doesn't work. Does this look like a plausible theory/exlanation?

Yes! The problem here is that inet_twsk_purge() operates on twsk not in net_exit_list, but I think such a check is overkill and we can work around it in another way.

Quoted 43 lines

Given:
1 exiting netns, has >= 1 tw sk.
1 (unrelated) netns that failed in setup_net

... we run into following race:

exiting netns, from cleanup wq, calls tcp_sk_exit_batch(), which calls inet_twsk_purge(&tcp_hashinfo).

At same time, from error unwinding code, we also call tcp_sk_exit_batch().

Both threads walk tcp_hashinfo ehash buckets.

From work queue (normal netns exit path), we hit

303 if (state == TCP_TIME_WAIT) {
304 inet_twsk_deschedule_put(inet_twsk(sk));

Because both threads operate on tcp_hashinfo, the unrelated struct net (exiting net) is also visible to error-unwinding thread.

So, error unwinding code will call

303 if (state == TCP_TIME_WAIT) {
304 inet_twsk_deschedule_put(inet_twsk(sk));

for the same tw sk and both threads do

218 void inet_twsk_deschedule_put(struct inet_timewait_sock *tw) 219 {
220 if (del_timer_sync(&tw->tw_timer))
221 inet_twsk_kill(tw);

Error unwind path cancel timer, calls inet_twsk_kill, while work queue sees timer as already shut-down so it ends up returning to tcp_sk_exit_batch(), where it will WARN here:

  WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

... because the supposedly-last tw_refcount decrement did not drop it down to 0.

Quoted 6 lines

Meanwhile, error unwiding thread calls refcount_dec() on tw_refcount, which now drops down to 0 instead of 1, which provides another warn splat.

I'll ponder on ways to fix this tomorrow unless someone else already has better theory/solution.

We need to sync two inet_twsk_kill(), so maybe give up one if twsk is not hashed ?

---8<---

net/ipv4/inet_timewait_sock.c +0 -0

index 337390ba85b4..51889567274b 100644

net/ipv4/inet_timewait_sock.c +1 -1

+	}
 	spin_unlock(lock);
 
 	/* Disassociate with bind bucket. */
---8<---

From: Kuniyuki Iwashima <[email protected]>
Date: Sun, 11 Aug 2024 16:00:29 -0700

Quoted 71 lines

From: Florian Westphal <[email protected]>
Date: Sun, 11 Aug 2024 18:28:50 +0200

Quoted 10 lines

Quoted 9 lines · Florian Westphal

https://syzkaller.appspot.com/x/log.txt?x=117f3182980000

... shows at two cores racing:

[ 3127.234402][ T1396] CPU: 3 PID: 1396 Comm: syz-executor.3 Not
and
[ 3127.257864][   T13] CPU: 1 PID: 13 Comm: kworker/u32:1 Not tainted 6.9.0-syzkalle (netns cleanup net).

Quoted 7 lines

Quoted 4 lines

first splat backtrace shows invocation of tcp_sk_exit_batch() from netns error unwinding code.

Second one lacks backtrace, but its also in tcp_sk_exit_batch(),

... which doesn't work. Does this look like a plausible theory/exlanation?

Yes! The problem here is that inet_twsk_purge() operates on twsk not in net_exit_list, but I think such a check is overkill and we can work around it in another way.

Quoted 46 lines

Given:
1 exiting netns, has >= 1 tw sk.
1 (unrelated) netns that failed in setup_net

... we run into following race:

exiting netns, from cleanup wq, calls tcp_sk_exit_batch(), which calls inet_twsk_purge(&tcp_hashinfo).

At same time, from error unwinding code, we also call tcp_sk_exit_batch().

Both threads walk tcp_hashinfo ehash buckets.

From work queue (normal netns exit path), we hit

303 if (state == TCP_TIME_WAIT) {
304 inet_twsk_deschedule_put(inet_twsk(sk));

Because both threads operate on tcp_hashinfo, the unrelated struct net (exiting net) is also visible to error-unwinding thread.

So, error unwinding code will call

303 if (state == TCP_TIME_WAIT) {
304 inet_twsk_deschedule_put(inet_twsk(sk));

for the same tw sk and both threads do

218 void inet_twsk_deschedule_put(struct inet_timewait_sock *tw) 219 {
220 if (del_timer_sync(&tw->tw_timer))
221 inet_twsk_kill(tw);

Error unwind path cancel timer, calls inet_twsk_kill, while work queue sees timer as already shut-down so it ends up returning to tcp_sk_exit_batch(), where it will WARN here:

  WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

... because the supposedly-last tw_refcount decrement did not drop it down to 0.

Meanwhile, error unwiding thread calls refcount_dec() on tw_refcount, which now drops down to 0 instead of 1, which provides another warn splat.

Quoted 15 lines

Quoted 3 lines

I'll ponder on ways to fix this tomorrow unless someone else already has better theory/solution.

We need to sync two inet_twsk_kill(), so maybe give up one if twsk is not hashed ?

---8<---

net/ipv4/inet_timewait_sock.c +0 -0

index 337390ba85b4..51889567274b 100644
@@ -52,7 +52,10 @@ static void inet_twsk_kill(struct inet_timewait_sock *tw)
      struct inet_bind_hashbucket *bhead, *bhead2;
 

net/ipv4/inet_timewait_sock.c static void inet_twsk_kill(struct inet_timewait_sock *tw) · line 57

Quoted 5 lines

net/ipv4/inet_timewait_sock.c +3 -1

      spin_lock(lock);
-     sk_nulls_del_node_init_rcu((struct sock *)tw);
+     if (!sk_nulls_del_node_init_rcu((struct sock *)tw)) {
+             spin_unlock(lock);
+             return false;

forgot to remove false, just return :)

Quoted 7 lines

net/ipv4/inet_timewait_sock.c +1 -1

+     }
      spin_unlock(lock);
 
      /* Disassociate with bind bucket. */
---8<---

From: Kuniyuki Iwashima <[email protected]>
Date: Sun, 11 Aug 2024 16:00:29 -0700

Quoted 23 lines

From: Florian Westphal <[email protected]>
Date: Sun, 11 Aug 2024 18:28:50 +0200

Quoted 10 lines

Quoted 9 lines · Florian Westphal

https://syzkaller.appspot.com/x/log.txt?x=117f3182980000

... shows at two cores racing:

[ 3127.234402][ T1396] CPU: 3 PID: 1396 Comm: syz-executor.3 Not
and
[ 3127.257864][   T13] CPU: 1 PID: 13 Comm: kworker/u32:1 Not tainted 6.9.0-syzkalle (netns cleanup net).

Quoted 7 lines

Quoted 4 lines

first splat backtrace shows invocation of tcp_sk_exit_batch() from netns error unwinding code.

Second one lacks backtrace, but its also in tcp_sk_exit_batch(),

... which doesn't work. Does this look like a plausible theory/exlanation?

Yes! The problem here is that inet_twsk_purge() operates on twsk not in net_exit_list, but I think such a check is overkill and we can work around it in another way.

Quoted 2 lines

We need to sync two inet_twsk_kill(), so maybe give up one if twsk is not hashed ?

Quoted 8 lines

---8<---

net/ipv4/inet_timewait_sock.c +0 -0

index 337390ba85b4..51889567274b 100644
@@ -52,7 +52,10 @@ static void inet_twsk_kill(struct inet_timewait_sock *tw)
 	struct inet_bind_hashbucket *bhead, *bhead2;
 

net/ipv4/inet_timewait_sock.c static void inet_twsk_kill(struct inet_timewait_sock *tw) · line 57

Quoted 5 lines

net/ipv4/inet_timewait_sock.c +3 -1

 	spin_lock(lock);
-	sk_nulls_del_node_init_rcu((struct sock *)tw);
+	if (!sk_nulls_del_node_init_rcu((struct sock *)tw)) {
+		spin_unlock(lock);
+		return false;

forgot to remove false, just return :)

Quoted 25 lines · Kuniyuki Iwashima

From: Kuniyuki Iwashima <[email protected]>
Date: Sun, 11 Aug 2024 16:00:29 -0700

Quoted 23 lines

From: Florian Westphal <[email protected]>
Date: Sun, 11 Aug 2024 18:28:50 +0200

Quoted 10 lines

Quoted 9 lines · Florian Westphal

https://syzkaller.appspot.com/x/log.txt?x=117f3182980000

... shows at two cores racing:

[ 3127.234402][ T1396] CPU: 3 PID: 1396 Comm: syz-executor.3 Not
and
[ 3127.257864][   T13] CPU: 1 PID: 13 Comm: kworker/u32:1 Not tainted 6.9.0-syzkalle (netns cleanup net).

Quoted 7 lines

Quoted 4 lines

first splat backtrace shows invocation of tcp_sk_exit_batch() from netns error unwinding code.

Second one lacks backtrace, but its also in tcp_sk_exit_batch(),

... which doesn't work. Does this look like a plausible theory/exlanation?

Yes! The problem here is that inet_twsk_purge() operates on twsk not in net_exit_list, but I think such a check is overkill and we can work around it in another way.

I'm not so sure. Once 'other' inet_twsk_purge() found the twsk and called inet_twsk_kill(), 'our' task has to wait for that to complete.

We need to force proper ordering so that all twsk found

static void __net_exit tcp_sk_exit_batch(struct list_head *net_exit_list)

{
        struct net *net;

/*HERE*/tcp_twsk_purge(net_exit_list);

        list_for_each_entry(net, net_exit_list, exit_list) {
                inet_pernet_hashinfo_free(net->ipv4.tcp_death_row.hashinfo);

.... have gone through inet_twsk_kill() so tw_refcount managed to drop back to 1 before doing

                WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

.

Quoted 2 lines

Quoted 2 lines

We need to sync two inet_twsk_kill(), so maybe give up one if twsk is not hashed ?

Not sure, afaiu only one thread enters inet_twsk_kill() (the one that manages to deactivate the timer).

Quoted 8 lines

Quoted 8 lines

---8<---

net/ipv4/inet_timewait_sock.c +0 -0

index 337390ba85b4..51889567274b 100644
@@ -52,7 +52,10 @@ static void inet_twsk_kill(struct inet_timewait_sock *tw)
    struct inet_bind_hashbucket *bhead, *bhead2;
 

Quoted 7 lines

net/ipv4/inet_timewait_sock.c static void inet_twsk_kill(struct inet_timewait_sock *tw) · line 57

Quoted 5 lines

net/ipv4/inet_timewait_sock.c +3 -1

    spin_lock(lock);
-   sk_nulls_del_node_init_rcu((struct sock *)tw);
+   if (!sk_nulls_del_node_init_rcu((struct sock *)tw)) {
+           spin_unlock(lock);
+           return false;

forgot to remove false, just return :)

I don't see how this helps, we need to wait until 'stolen' twsk has gone through inet_twsk_kill() and decremented tw_refcount. Obviously It would be a bit simpler if we had a reliable reproducer :-)

Quoted 3 lines

I don't see how this helps, we need to wait until 'stolen' twsk has gone through inet_twsk_kill() and decremented tw_refcount. Obviously It would be a bit simpler if we had a reliable reproducer :-)

Allow me to say something irrelevant to this bug report.

Do you think that Kuniyuki's patch can solve the race between two 'killers' calling inet_twsk_deschedule_put()->inet_twsk_kill() concurrently at two cores, say, inet_twsk_purge() and tcp_abort()?

It at least does help avoid decrementing tw_refcount twice in the above case if I understand correctly.

Quoted 9 lines · Jason Xing

Quoted 3 lines

I don't see how this helps, we need to wait until 'stolen' twsk has gone through inet_twsk_kill() and decremented tw_refcount. Obviously It would be a bit simpler if we had a reliable reproducer :-)

Allow me to say something irrelevant to this bug report.

Do you think that Kuniyuki's patch can solve the race between two 'killers' calling inet_twsk_deschedule_put()->inet_twsk_kill() concurrently at two cores, say, inet_twsk_purge() and tcp_abort()?

I don't think its possible, tcp_abort() calls inet_twsk_deschedule_put, which does:

        if (timer_shutdown_sync(&tw->tw_timer))
                inet_twsk_kill(tw);

So I don't see how two concurrent callers, working on same tw address, would both be able to shut down the timer.

One will shut it down and calls inet_twsk_kill(), other will wait until the callback has completed, but it doesn't call inet_twsk_kill().

Quoted 3 lines

It at least does help avoid decrementing tw_refcount twice in the above case if I understand correctly.

I don't think the refcount is decremented twice.

Problem is one thread is already at the 'final' decrement of
WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

in tcp_sk_exit_batch(), while other thread has not yet called refcount_dec() on it (inet_twsk_kill still executing).

So we get two splats, refcount_dec_and_test() returns 1 not expected 0 and refcount_dec() coming right afterwards from other task observes the transition to 0, while it should have dropped down to 1.

Quoted 25 lines · Kuniyuki Iwashima

From: Kuniyuki Iwashima <[email protected]>
Date: Sun, 11 Aug 2024 16:00:29 -0700

Quoted 23 lines

From: Florian Westphal <[email protected]>
Date: Sun, 11 Aug 2024 18:28:50 +0200

Quoted 10 lines

Quoted 9 lines · Florian Westphal

https://syzkaller.appspot.com/x/log.txt?x=117f3182980000

... shows at two cores racing:

[ 3127.234402][ T1396] CPU: 3 PID: 1396 Comm: syz-executor.3 Not
and
[ 3127.257864][   T13] CPU: 1 PID: 13 Comm: kworker/u32:1 Not tainted 6.9.0-syzkalle (netns cleanup net).

Quoted 7 lines

Quoted 4 lines

first splat backtrace shows invocation of tcp_sk_exit_batch() from netns error unwinding code.

Second one lacks backtrace, but its also in tcp_sk_exit_batch(),

... which doesn't work. Does this look like a plausible theory/exlanation?

Yes! The problem here is that inet_twsk_purge() operates on twsk not in net_exit_list, but I think such a check is overkill and we can work around it in another way.

I'm not so sure. Once 'other' inet_twsk_purge() found the twsk and called inet_twsk_kill(), 'our' task has to wait for that to complete.

We need to force proper ordering so that all twsk found

static void __net_exit tcp_sk_exit_batch(struct list_head *net_exit_list)

{
        struct net *net;

/*HERE*/tcp_twsk_purge(net_exit_list);

        list_for_each_entry(net, net_exit_list, exit_list) {
                inet_pernet_hashinfo_free(net->ipv4.tcp_death_row.hashinfo);

.... have gone through inet_twsk_kill() so tw_refcount managed to drop back to 1 before doing

                WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

.

Quoted 2 lines

Quoted 2 lines

We need to sync two inet_twsk_kill(), so maybe give up one if twsk is not hashed ?

Not sure, afaiu only one thread enters inet_twsk_kill() (the one that manages to deactivate the timer).

Quoted 9 lines

Quoted 9 lines

---8<---

net/ipv4/inet_timewait_sock.c +0 -0

index 337390ba85b4..51889567274b 100644
@@ -52,7 +52,10 @@ static void inet_twsk_kill(struct inet_timewait_sock *tw)
 	struct inet_bind_hashbucket *bhead, *bhead2;
 

Quoted 7 lines

net/ipv4/inet_timewait_sock.c static void inet_twsk_kill(struct inet_timewait_sock *tw) · line 57

Quoted 5 lines

net/ipv4/inet_timewait_sock.c +3 -1

 	spin_lock(lock);
-	sk_nulls_del_node_init_rcu((struct sock *)tw);
+	if (!sk_nulls_del_node_init_rcu((struct sock *)tw)) {
+		spin_unlock(lock);
+		return false;

forgot to remove false, just return :)

I don't see how this helps, we need to wait until 'stolen' twsk has gone through inet_twsk_kill() and decremented tw_refcount. Obviously It would be a bit simpler if we had a reliable reproducer :-)

Possible solutions I came up with so far:

1) revert b099ce2602d8 ("net: Batch inet_twsk_purge").

This commit replaced a net_eq(twsk_net(tw) ... with a check for dead netns (ns.count == 0),

Downside: We need to remove the purged_once trick that calls inet_twsk_purge(&tcp_hashinfo) only once per exiting batch in tcp_twsk_purge() as well.

As per b099ce2602d8 changelog, likely increases netns dismantle times.

Upside: simpler code, so this is my preferred solution.

No concurrent runoff anymore, by time tcp_twsk_purge() returns it has called refcount_dec(->tw_refcount) for every twsk in the exiting netns list, without other task stealing twsks owned by exiting netns.

Solution 2: change tcp_sk_exit_batch like this:

   tcp_twsk_purge(net_exit_list);

(quoted diff) +5 -0

+  list_for_each_entry(net, net_exit_list, exit_list) {
+      while (refcount_read(&net->ipv4.tcp_death_row.tw_refcount) > 1)
+         schedule();
+
+  }
 
    list_for_each_entry(net, net_exit_list, exit_list) {
       inet_pernet_hashinfo_free(net->ipv4.tcp_death_row.hashinfo);
       WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

This synchronizes two concurrent tcp_sk_exit_batch() calls via existing refcount; if netns setup error unwinding ran off with one of 'our' twsk, it will wait until other task has completed the refcount decrement.

I don't expect it to increase netns dismantle times, else we'd have seen the WARN_ON_ONCE splat frequently.

Solution 3:

Similar to 2), but via mutex_lock/unlock pair:

static void __net_exit tcp_sk_exit_batch(struct list_head *net_exit_list)

{
        struct net *net;

	mutex_lock(&tcp_exit_batch_mutex);

        tcp_twsk_purge(net_exit_list);

        list_for_each_entry(net, net_exit_list, exit_list) {
                inet_pernet_hashinfo_free(net->ipv4.tcp_death_row.hashinfo);
                WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));
                tcp_fastopen_ctx_destroy(net);
        }
	mutex_unlock(&tcp_exit_batch_mutex);
}

Solution 4:

I have doubts wrt. tcp_twsk_purge() interaction with tw timer firing at the 'wrong' time. This is independent "problem", I might be imagining things here.

Consider:
313 void inet_twsk_purge(struct inet_hashinfo *hashinfo) 314 {
[..]
321 for (slot = 0; slot <= ehash_mask; slot++, head++) {

tw sk timer fires on other cpu, inet_twsk_kill() does:

56         spin_lock(lock);
57         sk_nulls_del_node_init_rcu((struct sock *)tw);
58         spin_unlock(lock);

... then other cpu gets preempted. inet_twsk_purge() resumes and hits empty chain head:

322 if (hlist_nulls_empty(&head->chain))
323 continue;

so we don't(can't) wait for the timer to run to completion.

If this sounds plausible to you, this gives us solution 4:

Restart inet_twsk_purge() loop until tw_dr->tw_refcount) has dropped down to 1.

Alternatively (still assuming the above race is real), sk_nulls_del_node_init_rcu needs to be moved down:

48 static void inet_twsk_kill(struct inet_timewait_sock *tw)
...
58 /* Disassociate with bind bucket. */
...
68 spin_unlock(&bhead->lock);

70 refcount_dec(&tw->tw_dr->tw_refcount);

 +      spin_lock(lock);
 +      sk_nulls_del_node_init_rcu((struct sock *)tw);
 +      spin_unlock(lock);
71      inet_twsk_put(tw);

72 }

... so concurrent purge() call will find us the node list (and then wait on timer_shutdown_sync()) until other cpu executing the timer is done.

If twsk was unlinked from table already before inet_twsk_purge() had chance to find it sk, then in worst case call to tcp_twsk_destructor() is missing, but I don't see any ordering requirements that need us to wait for this.

Its possible that two threads call tcp_sk_exit_batch() concurrently, once from the cleanup_net workqueue, once from a task that failed to clone a new netns. In the latter case, error unwinding calls the exit handlers in reverse order for the 'failed' netns.

tcp_sk_exit_batch() calls tcp_twsk_purge().
Problem is that since commit b099ce2602d8 ("net: Batch inet_twsk_purge"), this function picks up twsk in any dying netns, not just the one passed in via exit_batch list.

This means that the error unwind of setup_net() can "steal" and destroy timewait sockets belonging to the exiting netns.

This allows the netns exit worker to proceed to call

WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

without the expected 1 -> 0 transition, which then splats.

At same time, error unwind path that is also running inet_twsk_purge() will splat as well:

WARNING: .. at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210
...

 refcount_dec include/linux/refcount.h:351 [inline]
 inet_twsk_kill+0x758/0x9c0 net/ipv4/inet_timewait_sock.c:70
 inet_twsk_deschedule_put net/ipv4/inet_timewait_sock.c:221
 inet_twsk_purge+0x725/0x890 net/ipv4/inet_timewait_sock.c:304
 tcp_sk_exit_batch+0x1c/0x170 net/ipv4/tcp_ipv4.c:3522
 ops_exit_list+0x128/0x180 net/core/net_namespace.c:178
 setup_net+0x714/0xb40 net/core/net_namespace.c:375
 copy_net_ns+0x2f0/0x670 net/core/net_namespace.c:508
 create_new_namespaces+0x3ea/0xb10 kernel/nsproxy.c:110

... because refcount_dec() of tw_refcount unexpectedly dropped to 0.

This doesn't seem like an actual bug (no tw sockets got lost and I don't see a use-after-free) but as erroneous trigger of debug check.

Add a mutex to force strict ordering: the task that calls tcp_twsk_purge() blocks other task from doing final _dec_and_test before mutex-owner has removed all tw sockets of dying netns.

Fixes e9bd0cca09d1 ("tcp: Don't allocate tcp_death_row outside of struct netns_ipv4.")

Cc Kuniyuki Iwashima <[email protected]>

Cc Jason Xing <[email protected]>

Reported-by [email protected]

Closes https://lore.kernel.org/netdev/[email protected]/ open here

Link https://lore.kernel.org/netdev/[email protected]/ open here

Signed-off-by Florian Westphal <[email protected]>

Quoted 4 lines

... because refcount_dec() of tw_refcount unexpectedly dropped to 0.

This doesn't seem like an actual bug (no tw sockets got lost and I don't see a use-after-free) but as erroneous trigger of debug check.

I guess the reason you don't move the check back to tcp_sk_exit() is

to catch a potential issue explained in solution 4 in the link, right ?

Quoted 6 lines · Kuniyuki Iwashima

Quoted 4 lines

... because refcount_dec() of tw_refcount unexpectedly dropped to 0.

This doesn't seem like an actual bug (no tw sockets got lost and I don't see a use-after-free) but as erroneous trigger of debug check.

I guess the reason you don't move the check back to tcp_sk_exit() is

No, it would not work. .exit runs before .exit_batch, we'd splat.

Before e9bd0cca09d1 ordering doesn't matter because refcount_dec_and_test is used consistently, so it was not relevant if the 0-transition occured from .exit or later via inet_twsk_kill.

Quoted 1 line

to catch a potential issue explained in solution 4 in the link, right ?

Yes, it would help to catch such issue wrt. twsk lifetime. Having the WARN ensures no twsk can escape .exit_batch completion.

E.g. if twsk destructors in the future refer to some other object that has to be released before netns pointers become invalid or something like that.

Does that make sense?

Otherwise I'm open to alternative approaches.
Or we can wait until syzbot finds a reproducer, I don't think its a real/urgent bug.

Its possible that two threads call tcp_sk_exit_batch() concurrently, once from the cleanup_net workqueue, once from a task that failed to clone a new netns. In the latter case, error unwinding calls the exit handlers in reverse order for the 'failed' netns.

tcp_sk_exit_batch() calls tcp_twsk_purge().
Problem is that since commit b099ce2602d8 ("net: Batch inet_twsk_purge"), this function picks up twsk in any dying netns, not just the one passed in via exit_batch list.

This means that the error unwind of setup_net() can "steal" and destroy timewait sockets belonging to the exiting netns.

This allows the netns exit worker to proceed to call

WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

without the expected 1 -> 0 transition, which then splats.

At same time, error unwind path that is also running inet_twsk_purge() will splat as well:

WARNING: .. at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210
...

 refcount_dec include/linux/refcount.h:351 [inline]
 inet_twsk_kill+0x758/0x9c0 net/ipv4/inet_timewait_sock.c:70
 inet_twsk_deschedule_put net/ipv4/inet_timewait_sock.c:221
 inet_twsk_purge+0x725/0x890 net/ipv4/inet_timewait_sock.c:304
 tcp_sk_exit_batch+0x1c/0x170 net/ipv4/tcp_ipv4.c:3522
 ops_exit_list+0x128/0x180 net/core/net_namespace.c:178
 setup_net+0x714/0xb40 net/core/net_namespace.c:375
 copy_net_ns+0x2f0/0x670 net/core/net_namespace.c:508
 create_new_namespaces+0x3ea/0xb10 kernel/nsproxy.c:110

... because refcount_dec() of tw_refcount unexpectedly dropped to 0.

This doesn't seem like an actual bug (no tw sockets got lost and I don't see a use-after-free) but as erroneous trigger of debug check.

Add a mutex to force strict ordering: the task that calls tcp_twsk_purge() blocks other task from doing final _dec_and_test before mutex-owner has removed all tw sockets of dying netns.

Fixes e9bd0cca09d1 ("tcp: Don't allocate tcp_death_row outside of struct netns_ipv4.")

Cc Kuniyuki Iwashima <[email protected]>

Cc Jason Xing <[email protected]>

Reported-by [email protected]

Closes https://lore.kernel.org/netdev/[email protected]/ open here

Link https://lore.kernel.org/netdev/[email protected]/ open here

Signed-off-by Florian Westphal <[email protected]>

Its possible that two threads call tcp_sk_exit_batch() concurrently, once from the cleanup_net workqueue, once from a task that failed to clone a new netns. In the latter case, error unwinding calls the exit handlers in reverse order for the 'failed' netns.

tcp_sk_exit_batch() calls tcp_twsk_purge().
Problem is that since commit b099ce2602d8 ("net: Batch inet_twsk_purge"), this function picks up twsk in any dying netns, not just the one passed in via exit_batch list.

This means that the error unwind of setup_net() can "steal" and destroy timewait sockets belonging to the exiting netns.

This allows the netns exit worker to proceed to call

WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

without the expected 1 -> 0 transition, which then splats.

At same time, error unwind path that is also running inet_twsk_purge() will splat as well:

WARNING: .. at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210
...

  refcount_dec include/linux/refcount.h:351 [inline]
  inet_twsk_kill+0x758/0x9c0 net/ipv4/inet_timewait_sock.c:70
  inet_twsk_deschedule_put net/ipv4/inet_timewait_sock.c:221
  inet_twsk_purge+0x725/0x890 net/ipv4/inet_timewait_sock.c:304
  tcp_sk_exit_batch+0x1c/0x170 net/ipv4/tcp_ipv4.c:3522
  ops_exit_list+0x128/0x180 net/core/net_namespace.c:178
  setup_net+0x714/0xb40 net/core/net_namespace.c:375
  copy_net_ns+0x2f0/0x670 net/core/net_namespace.c:508
  create_new_namespaces+0x3ea/0xb10 kernel/nsproxy.c:110

... because refcount_dec() of tw_refcount unexpectedly dropped to 0.

This doesn't seem like an actual bug (no tw sockets got lost and I don't see a use-after-free) but as erroneous trigger of debug check.

Add a mutex to force strict ordering: the task that calls tcp_twsk_purge() blocks other task from doing final _dec_and_test before mutex-owner has removed all tw sockets of dying netns.

Fixes e9bd0cca09d1 ("tcp: Don't allocate tcp_death_row outside of struct netns_ipv4.")

Cc Kuniyuki Iwashima <[email protected]>

Cc Jason Xing <[email protected]>

Reported-by [email protected]

Closes https://lore.kernel.org/netdev/[email protected]/ open here

Link https://lore.kernel.org/netdev/[email protected]/ open here

Signed-off-by Florian Westphal <[email protected]>

Quoted 43 lines · Florian Westphal

Its possible that two threads call tcp_sk_exit_batch() concurrently, once from the cleanup_net workqueue, once from a task that failed to clone a new netns. In the latter case, error unwinding calls the exit handlers in reverse order for the 'failed' netns.

tcp_sk_exit_batch() calls tcp_twsk_purge().
Problem is that since commit b099ce2602d8 ("net: Batch inet_twsk_purge"), this function picks up twsk in any dying netns, not just the one passed in via exit_batch list.

This means that the error unwind of setup_net() can "steal" and destroy timewait sockets belonging to the exiting netns.

This allows the netns exit worker to proceed to call

WARN_ON_ONCE(!refcount_dec_and_test(&net->ipv4.tcp_death_row.tw_refcount));

without the expected 1 -> 0 transition, which then splats.

At same time, error unwind path that is also running inet_twsk_purge() will splat as well:

WARNING: .. at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210
...

  refcount_dec include/linux/refcount.h:351 [inline]
  inet_twsk_kill+0x758/0x9c0 net/ipv4/inet_timewait_sock.c:70
  inet_twsk_deschedule_put net/ipv4/inet_timewait_sock.c:221
  inet_twsk_purge+0x725/0x890 net/ipv4/inet_timewait_sock.c:304
  tcp_sk_exit_batch+0x1c/0x170 net/ipv4/tcp_ipv4.c:3522
  ops_exit_list+0x128/0x180 net/core/net_namespace.c:178
  setup_net+0x714/0xb40 net/core/net_namespace.c:375
  copy_net_ns+0x2f0/0x670 net/core/net_namespace.c:508
  create_new_namespaces+0x3ea/0xb10 kernel/nsproxy.c:110

... because refcount_dec() of tw_refcount unexpectedly dropped to 0.

This doesn't seem like an actual bug (no tw sockets got lost and I don't see a use-after-free) but as erroneous trigger of debug check.

Add a mutex to force strict ordering: the task that calls tcp_twsk_purge() blocks other task from doing final _dec_and_test before mutex-owner has removed all tw sockets of dying netns.

Quoted 7 lines

Fixes e9bd0cca09d1 ("tcp: Don't allocate tcp_death_row outside of struct netns_ipv4.")

Cc Kuniyuki Iwashima <[email protected]>

Cc Jason Xing <[email protected]>

Reported-by [email protected]

Closes https://lore.kernel.org/netdev/[email protected]/ open here

Link https://lore.kernel.org/netdev/[email protected]/ open here

Signed-off-by Florian Westphal <[email protected]>

The fixes LGTM, but I prefer to keep the patch in PW a little longer to allow Eric having a look here.

From: syzbot <[email protected]>
Date: Sat, 10 Aug 2024 18:29:20 -0700

Quoted 18 lines

Hello,

syzbot found the following issue on:

HEAD commit:    33e02dc69afb Merge tag 'sound-6.10-rc1' of git://git.kerne..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=117f3182980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=25544a2faf4bae65
dashboard link: https://syzkaller.appspot.com/bug?extid=8ea26396ff85d23a8929
compiler:       gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40

Unfortunately, I don't have any reproducer for this issue yet.

Downloadable assets:
disk image (non-bootable): https://storage.googleapis.com/syzbot-assets/7bc7510fe41f/non_bootable_disk-33e02dc6.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/573c88ac3233/vmlinux-33e02dc6.xz
kernel image: https://storage.googleapis.com/syzbot-assets/760a52b9a00a/bzImage-33e02dc6.xz

Quoted 6 lines

IMPORTANT: if you fix the issue, please add the following tag to the commit:

Reported-by [email protected]

------------[ cut here ]------------
refcount_t: decrement hit 0; leaking memory.
WARNING: CPU: 3 PID: 1396 at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210 lib/refcount.c:31

Eric, this is the weird report I was talking about at netdevconf :)

It seems refcount_dec(&tw->tw_dr->tw_refcount) is somehow done earlier than refcount_inc().

I started to see the same splat at a very low rate after consuming commit b334b924c9b7 ("net: tcp/dccp: prepare for tw_timer un-pinning").

Quoted 11 lines

Quoted 3 lines

------------[ cut here ]------------
refcount_t: decrement hit 0; leaking memory.
WARNING: CPU: 3 PID: 1396 at lib/refcount.c:31 refcount_warn_saturate+0x1ed/0x210 lib/refcount.c:31

Eric, this is the weird report I was talking about at netdevconf :)

It seems refcount_dec(&tw->tw_dr->tw_refcount) is somehow done earlier than refcount_inc().

I started to see the same splat at a very low rate after consuming commit b334b924c9b7 ("net: tcp/dccp: prepare for tw_timer un-pinning").

Could you share a splat you saw?

Was it also for the fallback tcp_hashinfo and/or from error unwindinding after a ops->init() failure on netns creation?

I don't think it makes sense following this specific report given b334b924c9b7 made changes in this area, but a post-b334b924c9b7 would be more relevant I think.