| Message ID | 20240514025250.12604-1-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4,net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue lock. | expand |
On 5/14/24 04:52, Kuniyuki Iwashima wrote: > ... > diff --git a/net/unix/garbage.c b/net/unix/garbage.c > index 0104be9d4704..b87e48e2b51b 100644 > --- a/net/unix/garbage.c > +++ b/net/unix/garbage.c > @@ -342,10 +342,12 @@ static void __unix_gc(struct work_struct *work) > scan_children(&u->sk, inc_inflight, &hitlist); > > #if IS_ENABLED(CONFIG_AF_UNIX_OOB) > + spin_lock(&u->sk.sk_receive_queue.lock); > if (u->oob_skb) { > - kfree_skb(u->oob_skb); > + WARN_ON_ONCE(skb_unref(u->oob_skb)); > u->oob_skb = NULL; > } > + spin_unlock(&u->sk.sk_receive_queue.lock); > #endif > } I've realised this part of GC is broken for embryos. And adding a rq lock here turns a warning into a possible deadlock, so below is my attempt at fixing the underlying problem. It's based it on top of your patch, so should I post it now or wait until your patch lands in net? --- Subject: [PATCH] af_unix: Fix garbage collection of embryos carrying OOB/SCM_RIGHTS GC attempts to explicitly drop oob_skb before purging the hit list. The problem is with embryos: instead of trying to kfree_skb(u->oob_skb) of an embryo socket, GC goes for its parent-listener socket, which never carries u->oob_skb. Effectively oob_skb is removed from the receive queue, but remains reachable via u->oob_skb. Tell GC to dispose the right socket's oob_skb. Fixes: aa82ac51d633 ("af_unix: Drop oob_skb ref before purging queue in GC.") Signed-off-by: Michal Luczaj <mhal@rbox.co> --- from array import array from socket import * addr = 'unix-oob-splat' lis = socket(AF_UNIX, SOCK_STREAM) lis.bind(addr) lis.listen(1) s = socket(AF_UNIX, SOCK_STREAM) s.connect(addr) scm = (SOL_SOCKET, SCM_RIGHTS, array('i', [lis.fileno()])) s.sendmsg([b'x'], [scm], MSG_OOB) lis.close() [ 22.208683] WARNING: CPU: 2 PID: 546 at net/unix/garbage.c:371 __unix_gc+0x50e/0x520 [ 22.208687] Modules linked in: 9p netfs kvm_intel kvm 9pnet_virtio 9pnet i2c_piix4 zram crct10dif_pclmul crc32_pclmul crc32c_intel virtio_blk ghash_clmulni_intel serio_raw fuse qemu_fw_cfg virtio_console [ 22.208701] CPU: 2 PID: 546 Comm: kworker/u32:5 Not tainted 6.9.0-rc7nokasan+ #28 [ 22.208703] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014 [ 22.208704] Workqueue: events_unbound __unix_gc [ 22.208706] RIP: 0010:__unix_gc+0x50e/0x520 [ 22.208708] Code: 83 fa 01 0f 84 07 fe ff ff 85 d2 0f 8f 01 fe ff ff be 03 00 00 00 e8 f1 f7 9a ff e9 f2 fd ff ff e8 b7 f9 ff ff e9 28 fd ff ff <0f> 0b e9 07 ff ff ff e8 36 0a 1a 00 66 0f 1f 44 00 00 90 90 90 90 [ 22.208710] RSP: 0018:ffffc9000051fd90 EFLAGS: 00010283 [ 22.208712] RAX: ffff88810b316f30 RBX: ffffffff83563230 RCX: 0000000000000001 [ 22.208713] RDX: 0000000000000001 RSI: ffffffff82956cfb RDI: ffffffff83563880 [ 22.208714] RBP: ffffc9000051fe38 R08: 00000000cba2db62 R09: 00000000000003e5 [ 22.208715] R10: 0000000000000000 R11: 0000000000000000 R12: ffffc9000051fdb0 [ 22.208716] R13: ffff88810b316a00 R14: ffffc9000051fd90 R15: ffffffff83563860 [ 22.208717] FS: 0000000000000000(0000) GS:ffff88842fb00000(0000) knlGS:0000000000000000 [ 22.208718] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 22.208719] CR2: 0000557b30b7406c CR3: 000000011e5be000 CR4: 0000000000750ef0 [ 22.208722] PKRU: 55555554 [ 22.208723] Call Trace: [ 22.208724] <TASK> [ 22.208725] ? __warn.cold+0xb1/0x13e [ 22.208728] ? __unix_gc+0x50e/0x520 [ 22.208730] ? report_bug+0xe6/0x170 [ 22.208733] ? handle_bug+0x3c/0x80 [ 22.208735] ? exc_invalid_op+0x13/0x60 [ 22.208737] ? asm_exc_invalid_op+0x16/0x20 [ 22.208741] ? __unix_gc+0x50e/0x520 [ 22.208747] process_one_work+0x21f/0x590 [ 22.208750] ? move_linked_works+0x70/0xa0 [ 22.208753] worker_thread+0x1bf/0x3d0 [ 22.208756] ? __pfx_worker_thread+0x10/0x10 [ 22.208757] kthread+0xdd/0x110 [ 22.208759] ? __pfx_kthread+0x10/0x10 [ 22.208761] ret_from_fork+0x2d/0x50 [ 22.208763] ? __pfx_kthread+0x10/0x10 [ 22.208765] ret_from_fork_asm+0x1a/0x30 [ 22.208770] </TASK> [ 22.208771] irq event stamp: 198563 [ 22.208772] hardirqs last enabled at (198569): [<ffffffff811b617d>] console_unlock+0x10d/0x140 [ 22.208775] hardirqs last disabled at (198574): [<ffffffff811b6162>] console_unlock+0xf2/0x140 [ 22.208777] softirqs last enabled at (196450): [<ffffffff81110f4d>] __irq_exit_rcu+0x9d/0x100 [ 22.208778] softirqs last disabled at (196445): [<ffffffff81110f4d>] __irq_exit_rcu+0x9d/0x100 net/unix/garbage.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/net/unix/garbage.c b/net/unix/garbage.c index b87e48e2b51b..beecd0bfbf48 100644 --- a/net/unix/garbage.c +++ b/net/unix/garbage.c @@ -170,10 +170,11 @@ static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *), /* Process the descriptors of this socket */ int nfd = UNIXCB(skb).fp->count; struct file **fp = UNIXCB(skb).fp->fp; + struct unix_sock *u; while (nfd--) { /* Get the socket the fd matches if it indeed does so */ - struct unix_sock *u = unix_get_socket(*fp++); + u = unix_get_socket(*fp++); /* Ignore non-candidates, they could have been added * to the queues after starting the garbage collection @@ -187,6 +188,14 @@ static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *), if (hit && hitlist != NULL) { __skb_unlink(skb, &x->sk_receive_queue); __skb_queue_tail(hitlist, skb); + +#if IS_ENABLED(CONFIG_AF_UNIX_OOB) + u = unix_sk(x); + if (u->oob_skb == skb) { + WARN_ON_ONCE(skb_unref(u->oob_skb)); + u->oob_skb = NULL; + } +#endif } } } @@ -338,19 +347,9 @@ static void __unix_gc(struct work_struct *work) * which are creating the cycle(s). */ skb_queue_head_init(&hitlist); - list_for_each_entry(u, &gc_candidates, link) { + list_for_each_entry(u, &gc_candidates, link) scan_children(&u->sk, inc_inflight, &hitlist); -#if IS_ENABLED(CONFIG_AF_UNIX_OOB) - spin_lock(&u->sk.sk_receive_queue.lock); - if (u->oob_skb) { - WARN_ON_ONCE(skb_unref(u->oob_skb)); - u->oob_skb = NULL; - } - spin_unlock(&u->sk.sk_receive_queue.lock); -#endif - } - /* not_cycle_list contains those sockets which do not make up a * cycle. Restore these to the inflight list. */
From: Michal Luczaj <mhal@rbox.co> Date: Tue, 14 May 2024 12:13:36 +0200 > On 5/14/24 04:52, Kuniyuki Iwashima wrote: > > ... > > diff --git a/net/unix/garbage.c b/net/unix/garbage.c > > index 0104be9d4704..b87e48e2b51b 100644 > > --- a/net/unix/garbage.c > > +++ b/net/unix/garbage.c > > @@ -342,10 +342,12 @@ static void __unix_gc(struct work_struct *work) > > scan_children(&u->sk, inc_inflight, &hitlist); > > > > #if IS_ENABLED(CONFIG_AF_UNIX_OOB) > > + spin_lock(&u->sk.sk_receive_queue.lock); > > if (u->oob_skb) { > > - kfree_skb(u->oob_skb); > > + WARN_ON_ONCE(skb_unref(u->oob_skb)); > > u->oob_skb = NULL; > > } > > + spin_unlock(&u->sk.sk_receive_queue.lock); > > #endif > > } > > I've realised this part of GC is broken for embryos. And adding a rq lock > here turns a warning into a possible deadlock, so below is my attempt at > fixing the underlying problem. Exactly, I missed that case. It's memleak rather than deadlock. We need to traverse embryos from listener to drop OOB skb refcount in embroy recvq to drop listener fd's refcount. > > It's based it on top of your patch, so should I post it now or wait until > your patch lands in net? I'll post your patch within v5 that will minimise the delay given we are in rush for the merge window. > --- > Subject: [PATCH] af_unix: Fix garbage collection of embryos carrying > OOB/SCM_RIGHTS > > GC attempts to explicitly drop oob_skb before purging the hit list. The s/oob_skb/oob_skb's refcount/ > problem is with embryos: instead of trying to kfree_skb(u->oob_skb) of an > embryo socket, GC goes for its parent-listener socket, which never carries > u->oob_skb. Effectively oob_skb is removed from the receive queue, but > remains reachable via u->oob_skb. The last sentence is not correct as the listener does not have oob_skb and kfree_skb() is not called. I'll post this patch with some modification of commit message. Thanks! > > Tell GC to dispose the right socket's oob_skb. > > Fixes: aa82ac51d633 ("af_unix: Drop oob_skb ref before purging queue in GC.") > Signed-off-by: Michal Luczaj <mhal@rbox.co> > --- > from array import array > from socket import * > > addr = 'unix-oob-splat' > lis = socket(AF_UNIX, SOCK_STREAM) > lis.bind(addr) > lis.listen(1) > > s = socket(AF_UNIX, SOCK_STREAM) > s.connect(addr) > scm = (SOL_SOCKET, SCM_RIGHTS, array('i', [lis.fileno()])) > s.sendmsg([b'x'], [scm], MSG_OOB) > lis.close() > > [ 22.208683] WARNING: CPU: 2 PID: 546 at net/unix/garbage.c:371 __unix_gc+0x50e/0x520 > [ 22.208687] Modules linked in: 9p netfs kvm_intel kvm 9pnet_virtio 9pnet i2c_piix4 zram crct10dif_pclmul crc32_pclmul crc32c_intel virtio_blk ghash_clmulni_intel serio_raw fuse qemu_fw_cfg virtio_console > [ 22.208701] CPU: 2 PID: 546 Comm: kworker/u32:5 Not tainted 6.9.0-rc7nokasan+ #28 > [ 22.208703] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014 > [ 22.208704] Workqueue: events_unbound __unix_gc > [ 22.208706] RIP: 0010:__unix_gc+0x50e/0x520 > [ 22.208708] Code: 83 fa 01 0f 84 07 fe ff ff 85 d2 0f 8f 01 fe ff ff be 03 00 00 00 e8 f1 f7 9a ff e9 f2 fd ff ff e8 b7 f9 ff ff e9 28 fd ff ff <0f> 0b e9 07 ff ff ff e8 36 0a 1a 00 66 0f 1f 44 00 00 90 90 90 90 > [ 22.208710] RSP: 0018:ffffc9000051fd90 EFLAGS: 00010283 > [ 22.208712] RAX: ffff88810b316f30 RBX: ffffffff83563230 RCX: 0000000000000001 > [ 22.208713] RDX: 0000000000000001 RSI: ffffffff82956cfb RDI: ffffffff83563880 > [ 22.208714] RBP: ffffc9000051fe38 R08: 00000000cba2db62 R09: 00000000000003e5 > [ 22.208715] R10: 0000000000000000 R11: 0000000000000000 R12: ffffc9000051fdb0 > [ 22.208716] R13: ffff88810b316a00 R14: ffffc9000051fd90 R15: ffffffff83563860 > [ 22.208717] FS: 0000000000000000(0000) GS:ffff88842fb00000(0000) knlGS:0000000000000000 > [ 22.208718] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 22.208719] CR2: 0000557b30b7406c CR3: 000000011e5be000 CR4: 0000000000750ef0 > [ 22.208722] PKRU: 55555554 > [ 22.208723] Call Trace: > [ 22.208724] <TASK> > [ 22.208725] ? __warn.cold+0xb1/0x13e > [ 22.208728] ? __unix_gc+0x50e/0x520 > [ 22.208730] ? report_bug+0xe6/0x170 > [ 22.208733] ? handle_bug+0x3c/0x80 > [ 22.208735] ? exc_invalid_op+0x13/0x60 > [ 22.208737] ? asm_exc_invalid_op+0x16/0x20 > [ 22.208741] ? __unix_gc+0x50e/0x520 > [ 22.208747] process_one_work+0x21f/0x590 > [ 22.208750] ? move_linked_works+0x70/0xa0 > [ 22.208753] worker_thread+0x1bf/0x3d0 > [ 22.208756] ? __pfx_worker_thread+0x10/0x10 > [ 22.208757] kthread+0xdd/0x110 > [ 22.208759] ? __pfx_kthread+0x10/0x10 > [ 22.208761] ret_from_fork+0x2d/0x50 > [ 22.208763] ? __pfx_kthread+0x10/0x10 > [ 22.208765] ret_from_fork_asm+0x1a/0x30 > [ 22.208770] </TASK> > [ 22.208771] irq event stamp: 198563 > [ 22.208772] hardirqs last enabled at (198569): [<ffffffff811b617d>] console_unlock+0x10d/0x140 > [ 22.208775] hardirqs last disabled at (198574): [<ffffffff811b6162>] console_unlock+0xf2/0x140 > [ 22.208777] softirqs last enabled at (196450): [<ffffffff81110f4d>] __irq_exit_rcu+0x9d/0x100 > [ 22.208778] softirqs last disabled at (196445): [<ffffffff81110f4d>] __irq_exit_rcu+0x9d/0x100 > > net/unix/garbage.c | 23 +++++++++++------------ > 1 file changed, 11 insertions(+), 12 deletions(-) > > diff --git a/net/unix/garbage.c b/net/unix/garbage.c > index b87e48e2b51b..beecd0bfbf48 100644 > --- a/net/unix/garbage.c > +++ b/net/unix/garbage.c > @@ -170,10 +170,11 @@ static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *), > /* Process the descriptors of this socket */ > int nfd = UNIXCB(skb).fp->count; > struct file **fp = UNIXCB(skb).fp->fp; > + struct unix_sock *u; > > while (nfd--) { > /* Get the socket the fd matches if it indeed does so */ > - struct unix_sock *u = unix_get_socket(*fp++); > + u = unix_get_socket(*fp++); > > /* Ignore non-candidates, they could have been added > * to the queues after starting the garbage collection > @@ -187,6 +188,14 @@ static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *), > if (hit && hitlist != NULL) { > __skb_unlink(skb, &x->sk_receive_queue); > __skb_queue_tail(hitlist, skb); > + > +#if IS_ENABLED(CONFIG_AF_UNIX_OOB) > + u = unix_sk(x); > + if (u->oob_skb == skb) { > + WARN_ON_ONCE(skb_unref(u->oob_skb)); > + u->oob_skb = NULL; > + } > +#endif > } > } > } > @@ -338,19 +347,9 @@ static void __unix_gc(struct work_struct *work) > * which are creating the cycle(s). > */ > skb_queue_head_init(&hitlist); > - list_for_each_entry(u, &gc_candidates, link) { > + list_for_each_entry(u, &gc_candidates, link) > scan_children(&u->sk, inc_inflight, &hitlist); > > -#if IS_ENABLED(CONFIG_AF_UNIX_OOB) > - spin_lock(&u->sk.sk_receive_queue.lock); > - if (u->oob_skb) { > - WARN_ON_ONCE(skb_unref(u->oob_skb)); > - u->oob_skb = NULL; > - } > - spin_unlock(&u->sk.sk_receive_queue.lock); > -#endif > - } > - > /* not_cycle_list contains those sockets which do not make up a > * cycle. Restore these to the inflight list. > */ > -- > 2.45.0
On 5/15/24 02:07, Kuniyuki Iwashima wrote: > From: Michal Luczaj <mhal@rbox.co> > Date: Tue, 14 May 2024 12:13:36 +0200 >> On 5/14/24 04:52, Kuniyuki Iwashima wrote: >>> ... >>> diff --git a/net/unix/garbage.c b/net/unix/garbage.c >>> index 0104be9d4704..b87e48e2b51b 100644 >>> --- a/net/unix/garbage.c >>> +++ b/net/unix/garbage.c >>> @@ -342,10 +342,12 @@ static void __unix_gc(struct work_struct *work) >>> scan_children(&u->sk, inc_inflight, &hitlist); >>> >>> #if IS_ENABLED(CONFIG_AF_UNIX_OOB) >>> + spin_lock(&u->sk.sk_receive_queue.lock); >>> if (u->oob_skb) { >>> - kfree_skb(u->oob_skb); >>> + WARN_ON_ONCE(skb_unref(u->oob_skb)); >>> u->oob_skb = NULL; >>> } >>> + spin_unlock(&u->sk.sk_receive_queue.lock); >>> #endif >>> } >> >> I've realised this part of GC is broken for embryos. And adding a rq lock >> here turns a warning into a possible deadlock, so below is my attempt at >> fixing the underlying problem. > > Exactly, I missed that case. It's memleak rather than deadlock. > > We need to traverse embryos from listener to drop OOB skb refcount > in embroy recvq to drop listener fd's refcount. In a way, yeah. See below. >> It's based it on top of your patch, so should I post it now or wait until >> your patch lands in net? > > I'll post your patch within v5 that will minimise the delay given > we are in rush for the merge window. Awesome! >> Subject: [PATCH] af_unix: Fix garbage collection of embryos carrying >> OOB/SCM_RIGHTS >> >> GC attempts to explicitly drop oob_skb before purging the hit list. The > > s/oob_skb/oob_skb's refcount/ Ah, yeah, you're right. >> problem is with embryos: instead of trying to kfree_skb(u->oob_skb) of an >> embryo socket, GC goes for its parent-listener socket, which never carries >> u->oob_skb. Effectively oob_skb is removed from the receive queue, but >> remains reachable via u->oob_skb. > > The last sentence is not correct as the listener does not have oob_skb and > kfree_skb() is not called. I was referring to embryo's oob_skb. Anyway, I took a look at your v5 series and I see you've changed my commit message in ways I disagree with, so I'll comment there. > I'll post this patch with some modification of commit message. > > Thanks!
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index e94839d89b09..9bc879f3e34e 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -2217,13 +2217,15 @@ static int queue_oob(struct socket *sock, struct msghdr *msg, struct sock *other maybe_add_creds(skb, sock, other); skb_get(skb); + scm_stat_add(other, skb); + + spin_lock(&other->sk_receive_queue.lock); if (ousk->oob_skb) consume_skb(ousk->oob_skb); - WRITE_ONCE(ousk->oob_skb, skb); + __skb_queue_tail(&other->sk_receive_queue, skb); + spin_unlock(&other->sk_receive_queue.lock); - scm_stat_add(other, skb); - skb_queue_tail(&other->sk_receive_queue, skb); sk_send_sigurg(other); unix_state_unlock(other); other->sk_data_ready(other); @@ -2614,8 +2616,10 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state) mutex_lock(&u->iolock); unix_state_lock(sk); + spin_lock(&sk->sk_receive_queue.lock); if (sock_flag(sk, SOCK_URGINLINE) || !u->oob_skb) { + spin_unlock(&sk->sk_receive_queue.lock); unix_state_unlock(sk); mutex_unlock(&u->iolock); return -EINVAL; @@ -2627,6 +2631,8 @@ static int unix_stream_recv_urg(struct unix_stream_read_state *state) WRITE_ONCE(u->oob_skb, NULL); else skb_get(oob_skb); + + spin_unlock(&sk->sk_receive_queue.lock); unix_state_unlock(sk); chunk = state->recv_actor(oob_skb, 0, chunk, state); @@ -2655,6 +2661,10 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, consume_skb(skb); skb = NULL; } else { + struct sk_buff *unlinked_skb = NULL; + + spin_lock(&sk->sk_receive_queue.lock); + if (skb == u->oob_skb) { if (copied) { skb = NULL; @@ -2666,13 +2676,19 @@ static struct sk_buff *manage_oob(struct sk_buff *skb, struct sock *sk, } else if (flags & MSG_PEEK) { skb = NULL; } else { - skb_unlink(skb, &sk->sk_receive_queue); + __skb_unlink(skb, &sk->sk_receive_queue); WRITE_ONCE(u->oob_skb, NULL); - if (!WARN_ON_ONCE(skb_unref(skb))) - kfree_skb(skb); + unlinked_skb = skb; skb = skb_peek(&sk->sk_receive_queue); } } + + spin_unlock(&sk->sk_receive_queue.lock); + + if (unlinked_skb) { + WARN_ON_ONCE(skb_unref(unlinked_skb)); + kfree_skb(unlinked_skb); + } } return skb; } diff --git a/net/unix/garbage.c b/net/unix/garbage.c index 0104be9d4704..b87e48e2b51b 100644 --- a/net/unix/garbage.c +++ b/net/unix/garbage.c @@ -342,10 +342,12 @@ static void __unix_gc(struct work_struct *work) scan_children(&u->sk, inc_inflight, &hitlist); #if IS_ENABLED(CONFIG_AF_UNIX_OOB) + spin_lock(&u->sk.sk_receive_queue.lock); if (u->oob_skb) { - kfree_skb(u->oob_skb); + WARN_ON_ONCE(skb_unref(u->oob_skb)); u->oob_skb = NULL; } + spin_unlock(&u->sk.sk_receive_queue.lock); #endif }
Billy Jheng Bing-Jhong reported a race between __unix_gc() and queue_oob(). __unix_gc() tries to garbage-collect close()d inflight sockets, and then if the socket has MSG_OOB in unix_sk(sk)->oob_skb, GC will drop the reference and set NULL to it locklessly. However, the peer socket still can send MSG_OOB message and queue_oob() can update unix_sk(sk)->oob_skb concurrently, leading NULL pointer dereference. [0] To fix the issue, let's update unix_sk(sk)->oob_skb under the sk_receive_queue's lock and take it everywhere we touch oob_skb. Note that the same issue exists in the new GC, and the change in queue_oob() can be applied as is. Also note that we change kfree_skb() to skb_unref() in __unix_gc() to make it clear that we don't actually free OOB skb there, and we defer kfree_skb() in manage_oob() to silence lockdep false-positive (See [1]). [0]: BUG: kernel NULL pointer dereference, address: 0000000000000008 PF: supervisor write access in kernel mode PF: error_code(0x0002) - not-present page PGD 8000000009f5e067 P4D 8000000009f5e067 PUD 9f5d067 PMD 0 Oops: 0002 [#1] PREEMPT SMP PTI CPU: 3 PID: 50 Comm: kworker/3:1 Not tainted 6.9.0-rc5-00191-gd091e579b864 #110 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014 Workqueue: events delayed_fput RIP: 0010:skb_dequeue (./include/linux/skbuff.h:2386 ./include/linux/skbuff.h:2402 net/core/skbuff.c:3847) Code: 39 e3 74 3e 8b 43 10 48 89 ef 83 e8 01 89 43 10 49 8b 44 24 08 49 c7 44 24 08 00 00 00 00 49 8b 14 24 49 c7 04 24 00 00 00 00 <48> 89 42 08 48 89 10 e8 e7 c5 42 00 4c 89 e0 5b 5d 41 5c c3 cc cc RSP: 0018:ffffc900001bfd48 EFLAGS: 00000002 RAX: 0000000000000000 RBX: ffff8880088f5ae8 RCX: 00000000361289f9 RDX: 0000000000000000 RSI: 0000000000000206 RDI: ffff8880088f5b00 RBP: ffff8880088f5b00 R08: 0000000000080000 R09: 0000000000000001 R10: 0000000000000003 R11: 0000000000000001 R12: ffff8880056b6a00 R13: ffff8880088f5280 R14: 0000000000000001 R15: ffff8880088f5a80 FS: 0000000000000000(0000) GS:ffff88807dd80000(0000) knlGS:0000000000000000 CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 CR2: 0000000000000008 CR3: 0000000006314000 CR4: 00000000007506f0 PKRU: 55555554 Call Trace: <TASK> unix_release_sock (net/unix/af_unix.c:654) unix_release (net/unix/af_unix.c:1050) __sock_release (net/socket.c:660) sock_close (net/socket.c:1423) __fput (fs/file_table.c:423) delayed_fput (fs/file_table.c:444 (discriminator 3)) process_one_work (kernel/workqueue.c:3259) worker_thread (kernel/workqueue.c:3329 kernel/workqueue.c:3416) kthread (kernel/kthread.c:388) ret_from_fork (arch/x86/kernel/process.c:153) ret_from_fork_asm (arch/x86/entry/entry_64.S:257) </TASK> Modules linked in: CR2: 0000000000000008 Link: https://lore.kernel.org/netdev/a00d3993-c461-43f2-be6d-07259c98509a@rbox.co/ [1] Fixes: 1279f9d9dec2 ("af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.") Reported-by: Billy Jheng Bing-Jhong <billy@starlabs.sg> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- v4: * Free oob skb properly (Simon) v3: https://lore.kernel.org/all/20240513130628.33641-1-kuniyu@amazon.com/ * Fix lockdep false-positive by calling kfree_skb outside of recvq lock (Michal) v2: https://lore.kernel.org/netdev/20240510093905.25510-1-kuniyu@amazon.com/ * Add recvq locking everywhere we touch oob_skb (Paolo) v1: https://lore.kernel.org/netdev/20240507170018.83385-1-kuniyu@amazon.com/ --- net/unix/af_unix.c | 28 ++++++++++++++++++++++------ net/unix/garbage.c | 4 +++- 2 files changed, 25 insertions(+), 7 deletions(-)