[PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue lock.

[PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue lock.

[Kuniyuki Iwashima]

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(-)

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
 	}
 
-- 
2.30.2

Re: [PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue lock.

[Michal Luczaj]

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.
 	 */
-- 
2.45.0

Re: [PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue lock.

[Kuniyuki Iwashima]

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

Re: [PATCH v4 net] af_unix: Update unix_sk(sk)->oob_skb under sk_receive_queue lock.

[Michal Luczaj]

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!