| Message ID | 20240507163545.1131404-1-mhal@rbox.co (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] af_unix: Fix garbage collector racing against send() MSG_OOB | expand |
On 5/7/24 18:29, Michal Luczaj wrote: > Garbage collector takes care to explicitly drop oob_skb references before > purging the hit list. However, it does not account for a race: another > oob_skb might get assigned through in-flight socket's peer. Ah, my apologises, I'm not subscribed to netdev and haven't seen https://lore.kernel.org/netdev/20240507170018.83385-1-kuniyu@amazon.com/ I guess that was a race condition ;) Michal
diff --git a/include/net/af_unix.h b/include/net/af_unix.h index 3dee0b2721aa..a125bf1c1bb9 100644 --- a/include/net/af_unix.h +++ b/include/net/af_unix.h @@ -88,6 +88,7 @@ enum unix_socket_lock_class { U_LOCK_GC_LISTENER, /* used for listening socket while determining gc * candidates to close a small race window. */ + U_LOCK_GC_OOB, /* freeing oob_skb during GC. */ }; static inline void unix_state_lock_nested(struct sock *sk, diff --git a/net/unix/garbage.c b/net/unix/garbage.c index 0104be9d4704..9b0070589282 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) + unix_state_lock_nested(&u->sk, U_LOCK_GC_OOB); if (u->oob_skb) { kfree_skb(u->oob_skb); u->oob_skb = NULL; } + unix_state_unlock(&u->sk); #endif }
Garbage collector takes care to explicitly drop oob_skb references before purging the hit list. However, it does not account for a race: another oob_skb might get assigned through in-flight socket's peer. a/A and b/B are peers; A and B have been close()d and form an (OOB) in-flight cycle: a -- A <-> B -- b When A's and B's queues are about to be collected, their peers are still reachable. User can send another OOB packet: queue_oob unix_gc --------- ------- unix_state_lock(u) if (u->oob_sbk) consume_skb(u->oob_skb) // oob_skb refcnt == 1 scan_children(&u->sk, inc_inflight, &hitlist) if (u->oob_skb) { kfree_skb(u->oob_skb) // oob_skb refcnt == 0, but in hitlist (!) u->oob_skb = NULL } u->oob_skb = skb skb_queue(u->queue, skb) unix_state_unlock(u) Tell GC to remove oob_skb under unix_state_lock(). Fixes: aa82ac51d633 ("af_unix: Drop oob_skb ref before purging queue in GC.") Signed-off-by: Michal Luczaj <mhal@rbox.co> --- [ 16.320707] WARNING: CPU: 7 PID: 552 at net/unix/garbage.c:369 __unix_gc+0x4be/0x4d0 [ 16.320712] 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 [ 16.320726] CPU: 7 PID: 552 Comm: kworker/u32:8 Not tainted 6.9.0-rc7nokasan+ #12 [ 16.320728] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Arch Linux 1.16.3-1-1 04/01/2014 [ 16.320729] Workqueue: events_unbound __unix_gc [ 16.320731] RIP: 0010:__unix_gc+0x4be/0x4d0 [ 16.320733] Code: c0 b5 f1 81 3c 0a 74 18 e8 7f f8 ff ff e9 92 fd ff ff 0f 0b e9 bc fb ff ff 0f 0b e9 06 fc ff ff e8 07 fa ff ff e9 7a fd ff ff <0f> 0b e9 3b ff ff ff e8 e6 01 11 00 66 0f 1f 44 00 00 90 90 90 90 [ 16.320735] RSP: 0018:ffffc9000143fd90 EFLAGS: 00010283 [ 16.320737] RAX: ffff8881054576b0 RBX: ffffffff83563070 RCX: 0000000053568fdc [ 16.320738] RDX: 0000000000000001 RSI: ffffffff8293c754 RDI: ffffffff835636c0 [ 16.320739] RBP: ffffc9000143fe40 R08: 00000000000003c3 R09: 00000000000003c3 [ 16.320740] R10: 0000000000000000 R11: 0000000000000001 R12: ffffc9000143fdb0 [ 16.320741] R13: ffffffff835636a0 R14: ffffc9000143fd90 R15: ffff8881054576b0 [ 16.320742] FS: 0000000000000000(0000) GS:ffff88842fd80000(0000) knlGS:0000000000000000 [ 16.320743] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [ 16.320744] CR2: 0000559764faa540 CR3: 0000000003238000 CR4: 0000000000750ef0 [ 16.320747] PKRU: 55555554 [ 16.320748] Call Trace: [ 16.320749] <TASK> [ 16.320751] ? __warn+0x88/0x180 [ 16.320755] ? __unix_gc+0x4be/0x4d0 [ 16.320757] ? report_bug+0x18d/0x1c0 [ 16.320760] ? handle_bug+0x3c/0x80 [ 16.320762] ? exc_invalid_op+0x13/0x60 [ 16.320764] ? asm_exc_invalid_op+0x16/0x20 [ 16.320769] ? __unix_gc+0x4be/0x4d0 [ 16.320775] process_one_work+0x217/0x700 [ 16.320778] ? move_linked_works+0x70/0xa0 [ 16.320781] worker_thread+0x1ca/0x3b0 [ 16.320784] ? __pfx_worker_thread+0x10/0x10 [ 16.320786] kthread+0xdd/0x110 [ 16.320787] ? __pfx_kthread+0x10/0x10 [ 16.320789] ret_from_fork+0x2d/0x50 [ 16.320792] ? __pfx_kthread+0x10/0x10 [ 16.320793] ret_from_fork_asm+0x1a/0x30 [ 16.320799] </TASK> [ 16.320800] irq event stamp: 24197 [ 16.320801] hardirqs last enabled at (24203): [<ffffffff811c5b7d>] console_unlock+0xfd/0x130 [ 16.320803] hardirqs last disabled at (24208): [<ffffffff811c5b62>] console_unlock+0xe2/0x130 [ 16.320804] softirqs last enabled at (21512): [<ffffffff81117e51>] __irq_exit_rcu+0xa1/0x110 [ 16.320806] softirqs last disabled at (21505): [<ffffffff81117e51>] __irq_exit_rcu+0xa1/0x110 include/net/af_unix.h | 1 + net/unix/garbage.c | 2 ++ 2 files changed, 3 insertions(+)