* [PATCH v1 net] af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.
@ 2024-02-03 8:32 Kuniyuki Iwashima
2024-02-03 9:01 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2024-02-03 8:32 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Kuniyuki Iwashima, Kuniyuki Iwashima, netdev,
syzbot+fa3ef895554bdbfd1183
syzbot reported a warning [0] in __unix_gc() with a repro, which
creates a socketpair and sends one socket's fd to itself using the
peer.
socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0
sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\360", iov_len=1}],
msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_SOCKET,
cmsg_type=SCM_RIGHTS, cmsg_data=[3]}],
msg_controllen=24, msg_flags=0}, MSG_OOB|MSG_PROBE|MSG_DONTWAIT|MSG_ZEROCOPY) = 1
This forms a self-cyclic reference that GC should finally untangle
but does not due to lack of MSG_OOB handling, resulting in memory
leak.
Recently, commit 11498715f266 ("af_unix: Remove io_uring code for
GC.") removed io_uring's dead code in GC and revealed the problem.
The code was executed at the final stage of GC and unconditionally
moved all GC candidates from gc_candidates to gc_inflight_list.
That papered over the reported problem by always making the following
WARN_ON_ONCE(!list_empty(&gc_candidates)) false.
The problem has been there since commit 2aab4b969002 ("af_unix: fix
struct pid leaks in OOB support") added full scm support for MSG_OOB
while fixing another bug.
To fix this problem, we must call kfree_skb() for unix_sk(sk)->oob_skb
if the socket still exists in gc_candidates after purging collected skb.
Note that the leaked socket remained being linked to a global list, so
kmemleak also could not detect it. We need to check /proc/net/protocol
to notice the unfreed socket.
[0]:
WARNING: CPU: 0 PID: 2863 at net/unix/garbage.c:345 __unix_gc+0xc74/0xe80 net/unix/garbage.c:345
Modules linked in:
CPU: 0 PID: 2863 Comm: kworker/u4:11 Not tainted 6.8.0-rc1-syzkaller-00583-g1701940b1a02 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/25/2024
Workqueue: events_unbound __unix_gc
RIP: 0010:__unix_gc+0xc74/0xe80 net/unix/garbage.c:345
Code: 8b 5c 24 50 e9 86 f8 ff ff e8 f8 e4 22 f8 31 d2 48 c7 c6 30 6a 69 89 4c 89 ef e8 97 ef ff ff e9 80 f9 ff ff e8 dd e4 22 f8 90 <0f> 0b 90 e9 7b fd ff ff 48 89 df e8 5c e7 7c f8 e9 d3 f8 ff ff e8
RSP: 0018:ffffc9000b03fba0 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffffc9000b03fc10 RCX: ffffffff816c493e
RDX: ffff88802c02d940 RSI: ffffffff896982f3 RDI: ffffc9000b03fb30
RBP: ffffc9000b03fce0 R08: 0000000000000001 R09: fffff52001607f66
R10: 0000000000000003 R11: 0000000000000002 R12: dffffc0000000000
R13: ffffc9000b03fc10 R14: ffffc9000b03fc10 R15: 0000000000000001
FS: 0000000000000000(0000) GS:ffff8880b9400000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00005559c8677a60 CR3: 000000000d57a000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
process_one_work+0x889/0x15e0 kernel/workqueue.c:2633
process_scheduled_works kernel/workqueue.c:2706 [inline]
worker_thread+0x8b9/0x12a0 kernel/workqueue.c:2787
kthread+0x2c6/0x3b0 kernel/kthread.c:388
ret_from_fork+0x45/0x80 arch/x86/kernel/process.c:147
ret_from_fork_asm+0x1b/0x30 arch/x86/entry/entry_64.S:242
</TASK>
Reported-by: syzbot+fa3ef895554bdbfd1183@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=fa3ef895554bdbfd1183
Fixes: 2aab4b969002 ("af_unix: fix struct pid leaks in OOB support")
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
Given the commit disabling SCM_RIGHTS w/ io_uring was backporeted to
stable trees, we can backport this patch without commit 11498715f266,
so targeting net tree.
---
net/unix/garbage.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 2405f0f9af31..61f313d4a5dd 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -314,6 +314,15 @@ void unix_gc(void)
/* Here we are. Hitlist is filled. Die. */
__skb_queue_purge(&hitlist);
+ list_for_each_entry_safe(u, next, &gc_candidates, link) {
+ struct sk_buff *skb = u->oob_skb;
+
+ if (skb) {
+ u->oob_skb = NULL;
+ kfree_skb(skb);
+ }
+ }
+
spin_lock(&unix_gc_lock);
/* There could be io_uring registered files, just push them back to
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v1 net] af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.
2024-02-03 8:32 [PATCH v1 net] af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC Kuniyuki Iwashima
@ 2024-02-03 9:01 ` Eric Dumazet
2024-02-03 9:14 ` Kuniyuki Iwashima
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2024-02-03 9:01 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: David S. Miller, Jakub Kicinski, Paolo Abeni, Kuniyuki Iwashima,
netdev, syzbot+fa3ef895554bdbfd1183
On Sat, Feb 3, 2024 at 9:33 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> syzbot reported a warning [0] in __unix_gc() with a repro, which
> creates a socketpair and sends one socket's fd to itself using the
> peer.
>
> socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0
> sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\360", iov_len=1}],
> msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_SOCKET,
> cmsg_type=SCM_RIGHTS, cmsg_data=[3]}],
> msg_controllen=24, msg_flags=0}, MSG_OOB|MSG_PROBE|MSG_DONTWAIT|MSG_ZEROCOPY) = 1
>
> This forms a self-cyclic reference that GC should finally untangle
> but does not due to lack of MSG_OOB handling, resulting in memory
> leak.
>
> Recently, commit 11498715f266 ("af_unix: Remove io_uring code for
> GC.") removed io_uring's dead code in GC and revealed the problem.
>
> The code was executed at the final stage of GC and unconditionally
> moved all GC candidates from gc_candidates to gc_inflight_list.
> That papered over the reported problem by always making the following
> WARN_ON_ONCE(!list_empty(&gc_candidates)) false.
>
> The problem has been there since commit 2aab4b969002 ("af_unix: fix
> struct pid leaks in OOB support") added full scm support for MSG_OOB
> while fixing another bug.
>
> To fix this problem, we must call kfree_skb() for unix_sk(sk)->oob_skb
> if the socket still exists in gc_candidates after purging collected skb.
>
> Note that the leaked socket remained being linked to a global list, so
> kmemleak also could not detect it. We need to check /proc/net/protocol
> to notice the unfreed socket.
>
> [
> Reported-by: syzbot+fa3ef895554bdbfd1183@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=fa3ef895554bdbfd1183
> Fixes: 2aab4b969002 ("af_unix: fix struct pid leaks in OOB support")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> Given the commit disabling SCM_RIGHTS w/ io_uring was backporeted to
> stable trees, we can backport this patch without commit 11498715f266,
> so targeting net tree.
> ---
> net/unix/garbage.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index 2405f0f9af31..61f313d4a5dd 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -314,6 +314,15 @@ void unix_gc(void)
> /* Here we are. Hitlist is filled. Die. */
> __skb_queue_purge(&hitlist);
>
> + list_for_each_entry_safe(u, next, &gc_candidates, link) {
> + struct sk_buff *skb = u->oob_skb;
> +
> + if (skb) {
> + u->oob_skb = NULL;
> + kfree_skb(skb);
> + }
> + }
> +
Reviewed-by: Eric Dumazet <edumazet@google.com>
Note there is already a 'struct sk_buff *skb;" variable in scope.
This could be rewritten
list_for_each_entry_safe(u, next, &gc_candidates, link) {
kfree_skb(u->oob_skb);
u->oob_skb = NULL;
}
Also we probably can send this later:
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 2405f0f9af31c0ccefe2aa404002cfab8583c090..02466224445c9ec9b1259468d30c89fc5e905a6b
100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -283,7 +283,7 @@ void unix_gc(void)
* inflight counters for these as well, and remove the skbuffs
* which are creating the cycle(s).
*/
- skb_queue_head_init(&hitlist);
+ __skb_queue_head_init(&hitlist);
list_for_each_entry(u, &gc_candidates, link)
scan_children(&u->sk, inc_inflight, &hitlist);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 net] af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.
2024-02-03 9:01 ` Eric Dumazet
@ 2024-02-03 9:14 ` Kuniyuki Iwashima
2024-02-03 11:42 ` Eric Dumazet
0 siblings, 1 reply; 5+ messages in thread
From: Kuniyuki Iwashima @ 2024-02-03 9:14 UTC (permalink / raw)
To: edumazet
Cc: davem, kuba, kuni1840, kuniyu, netdev, pabeni,
syzbot+fa3ef895554bdbfd1183
From: Eric Dumazet <edumazet@google.com>
Date: Sat, 3 Feb 2024 10:01:04 +0100
> On Sat, Feb 3, 2024 at 9:33 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > syzbot reported a warning [0] in __unix_gc() with a repro, which
> > creates a socketpair and sends one socket's fd to itself using the
> > peer.
> >
> > socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0
> > sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\360", iov_len=1}],
> > msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_SOCKET,
> > cmsg_type=SCM_RIGHTS, cmsg_data=[3]}],
> > msg_controllen=24, msg_flags=0}, MSG_OOB|MSG_PROBE|MSG_DONTWAIT|MSG_ZEROCOPY) = 1
> >
> > This forms a self-cyclic reference that GC should finally untangle
> > but does not due to lack of MSG_OOB handling, resulting in memory
> > leak.
> >
> > Recently, commit 11498715f266 ("af_unix: Remove io_uring code for
> > GC.") removed io_uring's dead code in GC and revealed the problem.
> >
> > The code was executed at the final stage of GC and unconditionally
> > moved all GC candidates from gc_candidates to gc_inflight_list.
> > That papered over the reported problem by always making the following
> > WARN_ON_ONCE(!list_empty(&gc_candidates)) false.
> >
> > The problem has been there since commit 2aab4b969002 ("af_unix: fix
> > struct pid leaks in OOB support") added full scm support for MSG_OOB
> > while fixing another bug.
> >
> > To fix this problem, we must call kfree_skb() for unix_sk(sk)->oob_skb
> > if the socket still exists in gc_candidates after purging collected skb.
> >
> > Note that the leaked socket remained being linked to a global list, so
> > kmemleak also could not detect it. We need to check /proc/net/protocol
> > to notice the unfreed socket.
> >
> > [
> > Reported-by: syzbot+fa3ef895554bdbfd1183@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=fa3ef895554bdbfd1183
> > Fixes: 2aab4b969002 ("af_unix: fix struct pid leaks in OOB support")
> > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > ---
> > Given the commit disabling SCM_RIGHTS w/ io_uring was backporeted to
> > stable trees, we can backport this patch without commit 11498715f266,
> > so targeting net tree.
> > ---
> > net/unix/garbage.c | 9 +++++++++
> > 1 file changed, 9 insertions(+)
> >
> > diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> > index 2405f0f9af31..61f313d4a5dd 100644
> > --- a/net/unix/garbage.c
> > +++ b/net/unix/garbage.c
> > @@ -314,6 +314,15 @@ void unix_gc(void)
> > /* Here we are. Hitlist is filled. Die. */
> > __skb_queue_purge(&hitlist);
> >
> > + list_for_each_entry_safe(u, next, &gc_candidates, link) {
> > + struct sk_buff *skb = u->oob_skb;
> > +
> > + if (skb) {
> > + u->oob_skb = NULL;
> > + kfree_skb(skb);
> > + }
> > + }
> > +
>
> Reviewed-by: Eric Dumazet <edumazet@google.com>
>
> Note there is already a 'struct sk_buff *skb;" variable in scope.
>
> This could be rewritten
>
> list_for_each_entry_safe(u, next, &gc_candidates, link) {
> kfree_skb(u->oob_skb);
> u->oob_skb = NULL;
> }
I wrote that in the inital fix but noticed that this
kfree_skb() triggers fput(), and later in unix_release_sock()
we will call the duplicate kfree_skb().
if (u->oob_skb) {
kfree_skb(u->oob_skb);
u->oob_skb = NULL;
}
So, we need to set NULL before kfree_skb() in __unix_gc().
>
> Also we probably can send this later:
Yes, I changed as such in a new GC implementation, this needs
respin for MSG_OOB support though...
https://lore.kernel.org/netdev/20240203030058.60750-14-kuniyu@amazon.com/
Thanks!
>
> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index 2405f0f9af31c0ccefe2aa404002cfab8583c090..02466224445c9ec9b1259468d30c89fc5e905a6b
> 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -283,7 +283,7 @@ void unix_gc(void)
> * inflight counters for these as well, and remove the skbuffs
> * which are creating the cycle(s).
> */
> - skb_queue_head_init(&hitlist);
> + __skb_queue_head_init(&hitlist);
> list_for_each_entry(u, &gc_candidates, link)
> scan_children(&u->sk, inc_inflight, &hitlist);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 net] af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.
2024-02-03 9:14 ` Kuniyuki Iwashima
@ 2024-02-03 11:42 ` Eric Dumazet
2024-02-03 17:40 ` Kuniyuki Iwashima
0 siblings, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2024-02-03 11:42 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: davem, kuba, kuni1840, netdev, pabeni, syzbot+fa3ef895554bdbfd1183
On Sat, Feb 3, 2024 at 10:15 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Eric Dumazet <edumazet@google.com>
> Date: Sat, 3 Feb 2024 10:01:04 +0100
> > On Sat, Feb 3, 2024 at 9:33 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > >
> > > syzbot reported a warning [0] in __unix_gc() with a repro, which
> > > creates a socketpair and sends one socket's fd to itself using the
> > > peer.
> > >
> > > socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0
> > > sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\360", iov_len=1}],
> > > msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_SOCKET,
> > > cmsg_type=SCM_RIGHTS, cmsg_data=[3]}],
> > > msg_controllen=24, msg_flags=0}, MSG_OOB|MSG_PROBE|MSG_DONTWAIT|MSG_ZEROCOPY) = 1
> > >
> > > This forms a self-cyclic reference that GC should finally untangle
> > > but does not due to lack of MSG_OOB handling, resulting in memory
> > > leak.
> > >
> > > Recently, commit 11498715f266 ("af_unix: Remove io_uring code for
> > > GC.") removed io_uring's dead code in GC and revealed the problem.
> > >
> > > The code was executed at the final stage of GC and unconditionally
> > > moved all GC candidates from gc_candidates to gc_inflight_list.
> > > That papered over the reported problem by always making the following
> > > WARN_ON_ONCE(!list_empty(&gc_candidates)) false.
> > >
> > > The problem has been there since commit 2aab4b969002 ("af_unix: fix
> > > struct pid leaks in OOB support") added full scm support for MSG_OOB
> > > while fixing another bug.
> > >
> > > To fix this problem, we must call kfree_skb() for unix_sk(sk)->oob_skb
> > > if the socket still exists in gc_candidates after purging collected skb.
> > >
> > > Note that the leaked socket remained being linked to a global list, so
> > > kmemleak also could not detect it. We need to check /proc/net/protocol
> > > to notice the unfreed socket.
> > >
> > > [
> > > Reported-by: syzbot+fa3ef895554bdbfd1183@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=fa3ef895554bdbfd1183
> > > Fixes: 2aab4b969002 ("af_unix: fix struct pid leaks in OOB support")
> > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > > ---
> > > Given the commit disabling SCM_RIGHTS w/ io_uring was backporeted to
> > > stable trees, we can backport this patch without commit 11498715f266,
> > > so targeting net tree.
> > > ---
> > > net/unix/garbage.c | 9 +++++++++
> > > 1 file changed, 9 insertions(+)
> > >
> > > diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> > > index 2405f0f9af31..61f313d4a5dd 100644
> > > --- a/net/unix/garbage.c
> > > +++ b/net/unix/garbage.c
> > > @@ -314,6 +314,15 @@ void unix_gc(void)
> > > /* Here we are. Hitlist is filled. Die. */
> > > __skb_queue_purge(&hitlist);
> > >
> > > + list_for_each_entry_safe(u, next, &gc_candidates, link) {
> > > + struct sk_buff *skb = u->oob_skb;
> > > +
> > > + if (skb) {
> > > + u->oob_skb = NULL;
> > > + kfree_skb(skb);
> > > + }
> > > + }
> > > +
> >
> > Reviewed-by: Eric Dumazet <edumazet@google.com>
> >
> > Note there is already a 'struct sk_buff *skb;" variable in scope.
> >
> > This could be rewritten
> >
> > list_for_each_entry_safe(u, next, &gc_candidates, link) {
> > kfree_skb(u->oob_skb);
> > u->oob_skb = NULL;
> > }
>
> I wrote that in the inital fix but noticed that this
> kfree_skb() triggers fput(), and later in unix_release_sock()
> we will call the duplicate kfree_skb().
>
> if (u->oob_skb) {
> kfree_skb(u->oob_skb);
> u->oob_skb = NULL;
> }
>
> So, we need to set NULL before kfree_skb() in __unix_gc().
Okay...
But we probably need :
#if IS_ENABLED(CONFIG_AF_UNIX_OOB)
>
>
> >
> > Also we probably can send this later:
>
> Yes, I changed as such in a new GC implementation, this needs
> respin for MSG_OOB support though...
> https://lore.kernel.org/netdev/20240203030058.60750-14-kuniyu@amazon.com/
>
> Thanks!
>
> >
> > diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> > index 2405f0f9af31c0ccefe2aa404002cfab8583c090..02466224445c9ec9b1259468d30c89fc5e905a6b
> > 100644
> > --- a/net/unix/garbage.c
> > +++ b/net/unix/garbage.c
> > @@ -283,7 +283,7 @@ void unix_gc(void)
> > * inflight counters for these as well, and remove the skbuffs
> > * which are creating the cycle(s).
> > */
> > - skb_queue_head_init(&hitlist);
> > + __skb_queue_head_init(&hitlist);
> > list_for_each_entry(u, &gc_candidates, link)
> > scan_children(&u->sk, inc_inflight, &hitlist);
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 net] af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.
2024-02-03 11:42 ` Eric Dumazet
@ 2024-02-03 17:40 ` Kuniyuki Iwashima
0 siblings, 0 replies; 5+ messages in thread
From: Kuniyuki Iwashima @ 2024-02-03 17:40 UTC (permalink / raw)
To: edumazet
Cc: davem, kuba, kuni1840, kuniyu, netdev, pabeni,
syzbot+fa3ef895554bdbfd1183
From: Eric Dumazet <edumazet@google.com>
Date: Sat, 3 Feb 2024 12:42:12 +0100
> On Sat, Feb 3, 2024 at 10:15 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> >
> > From: Eric Dumazet <edumazet@google.com>
> > Date: Sat, 3 Feb 2024 10:01:04 +0100
> > > On Sat, Feb 3, 2024 at 9:33 AM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
> > > >
> > > > syzbot reported a warning [0] in __unix_gc() with a repro, which
> > > > creates a socketpair and sends one socket's fd to itself using the
> > > > peer.
> > > >
> > > > socketpair(AF_UNIX, SOCK_STREAM, 0, [3, 4]) = 0
> > > > sendmsg(4, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\360", iov_len=1}],
> > > > msg_iovlen=1, msg_control=[{cmsg_len=20, cmsg_level=SOL_SOCKET,
> > > > cmsg_type=SCM_RIGHTS, cmsg_data=[3]}],
> > > > msg_controllen=24, msg_flags=0}, MSG_OOB|MSG_PROBE|MSG_DONTWAIT|MSG_ZEROCOPY) = 1
> > > >
> > > > This forms a self-cyclic reference that GC should finally untangle
> > > > but does not due to lack of MSG_OOB handling, resulting in memory
> > > > leak.
> > > >
> > > > Recently, commit 11498715f266 ("af_unix: Remove io_uring code for
> > > > GC.") removed io_uring's dead code in GC and revealed the problem.
> > > >
> > > > The code was executed at the final stage of GC and unconditionally
> > > > moved all GC candidates from gc_candidates to gc_inflight_list.
> > > > That papered over the reported problem by always making the following
> > > > WARN_ON_ONCE(!list_empty(&gc_candidates)) false.
> > > >
> > > > The problem has been there since commit 2aab4b969002 ("af_unix: fix
> > > > struct pid leaks in OOB support") added full scm support for MSG_OOB
> > > > while fixing another bug.
> > > >
> > > > To fix this problem, we must call kfree_skb() for unix_sk(sk)->oob_skb
> > > > if the socket still exists in gc_candidates after purging collected skb.
> > > >
> > > > Note that the leaked socket remained being linked to a global list, so
> > > > kmemleak also could not detect it. We need to check /proc/net/protocol
> > > > to notice the unfreed socket.
> > > >
> > > > [
> > > > Reported-by: syzbot+fa3ef895554bdbfd1183@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=fa3ef895554bdbfd1183
> > > > Fixes: 2aab4b969002 ("af_unix: fix struct pid leaks in OOB support")
> > > > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > > > ---
> > > > Given the commit disabling SCM_RIGHTS w/ io_uring was backporeted to
> > > > stable trees, we can backport this patch without commit 11498715f266,
> > > > so targeting net tree.
> > > > ---
> > > > net/unix/garbage.c | 9 +++++++++
> > > > 1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> > > > index 2405f0f9af31..61f313d4a5dd 100644
> > > > --- a/net/unix/garbage.c
> > > > +++ b/net/unix/garbage.c
> > > > @@ -314,6 +314,15 @@ void unix_gc(void)
> > > > /* Here we are. Hitlist is filled. Die. */
> > > > __skb_queue_purge(&hitlist);
> > > >
> > > > + list_for_each_entry_safe(u, next, &gc_candidates, link) {
> > > > + struct sk_buff *skb = u->oob_skb;
> > > > +
> > > > + if (skb) {
> > > > + u->oob_skb = NULL;
> > > > + kfree_skb(skb);
> > > > + }
> > > > + }
> > > > +
> > >
> > > Reviewed-by: Eric Dumazet <edumazet@google.com>
> > >
> > > Note there is already a 'struct sk_buff *skb;" variable in scope.
> > >
> > > This could be rewritten
> > >
> > > list_for_each_entry_safe(u, next, &gc_candidates, link) {
> > > kfree_skb(u->oob_skb);
> > > u->oob_skb = NULL;
> > > }
> >
> > I wrote that in the inital fix but noticed that this
> > kfree_skb() triggers fput(), and later in unix_release_sock()
> > we will call the duplicate kfree_skb().
> >
> > if (u->oob_skb) {
> > kfree_skb(u->oob_skb);
> > u->oob_skb = NULL;
> > }
> >
> > So, we need to set NULL before kfree_skb() in __unix_gc().
>
> Okay...
>
> But we probably need :
>
> #if IS_ENABLED(CONFIG_AF_UNIX_OOB)
Ah exactly, thanks for catching!
will fix in v2.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-02-03 17:41 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-03 8:32 [PATCH v1 net] af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC Kuniyuki Iwashima
2024-02-03 9:01 ` Eric Dumazet
2024-02-03 9:14 ` Kuniyuki Iwashima
2024-02-03 11:42 ` Eric Dumazet
2024-02-03 17:40 ` Kuniyuki Iwashima
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.