* [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC.
@ 2024-01-29 19:04 Kuniyuki Iwashima
2024-01-29 19:04 ` [PATCH v1 net-next 1/3] af_unix: Replace BUG_ON() with WARN_ON_ONCE() Kuniyuki Iwashima
` (4 more replies)
0 siblings, 5 replies; 9+ messages in thread
From: Kuniyuki Iwashima @ 2024-01-29 19:04 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Jens Axboe, Pavel Begunkov, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev
I will post another series that rewrites the garbage collector for
AF_UNIX socket.
This is a prep series to clean up changes to GC made by io_uring but
now not necessary.
Kuniyuki Iwashima (3):
af_unix: Replace BUG_ON() with WARN_ON_ONCE().
af_unix: Remove io_uring code for GC.
af_unix: Remove CONFIG_UNIX_SCM.
include/net/af_unix.h | 8 +--
net/Makefile | 2 +-
net/unix/Kconfig | 5 --
net/unix/Makefile | 2 -
net/unix/af_unix.c | 63 ++++++++++++++++-
net/unix/garbage.c | 106 ++++++++++++++++++++--------
net/unix/scm.c | 156 ------------------------------------------
net/unix/scm.h | 10 ---
8 files changed, 143 insertions(+), 209 deletions(-)
delete mode 100644 net/unix/scm.c
delete mode 100644 net/unix/scm.h
--
2.30.2
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v1 net-next 1/3] af_unix: Replace BUG_ON() with WARN_ON_ONCE().
2024-01-29 19:04 [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Kuniyuki Iwashima
@ 2024-01-29 19:04 ` Kuniyuki Iwashima
2024-01-29 19:04 ` [PATCH v1 net-next 2/3] af_unix: Remove io_uring code for GC Kuniyuki Iwashima
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Kuniyuki Iwashima @ 2024-01-29 19:04 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Jens Axboe, Pavel Begunkov, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev
This is a prep patch for the last patch in this series so that
checkpatch will not warn about BUG_ON().
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
net/unix/garbage.c | 8 ++++----
net/unix/scm.c | 8 ++++----
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 4046c606f0e6..af676bb8fb67 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -145,7 +145,7 @@ static void scan_children(struct sock *x, void (*func)(struct unix_sock *),
/* An embryo cannot be in-flight, so it's safe
* to use the list link.
*/
- BUG_ON(!list_empty(&u->link));
+ WARN_ON_ONCE(!list_empty(&u->link));
list_add_tail(&u->link, &embryos);
}
spin_unlock(&x->sk_receive_queue.lock);
@@ -213,8 +213,8 @@ static void __unix_gc(struct work_struct *work)
total_refs = file_count(u->sk.sk_socket->file);
- BUG_ON(!u->inflight);
- BUG_ON(total_refs < u->inflight);
+ WARN_ON_ONCE(!u->inflight);
+ WARN_ON_ONCE(total_refs < u->inflight);
if (total_refs == u->inflight) {
list_move_tail(&u->link, &gc_candidates);
__set_bit(UNIX_GC_CANDIDATE, &u->gc_flags);
@@ -294,7 +294,7 @@ static void __unix_gc(struct work_struct *work)
list_move_tail(&u->link, &gc_inflight_list);
/* All candidates should have been detached by now. */
- BUG_ON(!list_empty(&gc_candidates));
+ WARN_ON_ONCE(!list_empty(&gc_candidates));
/* Paired with READ_ONCE() in wait_for_unix_gc(). */
WRITE_ONCE(gc_in_progress, false);
diff --git a/net/unix/scm.c b/net/unix/scm.c
index b5ae5ab16777..505e56cf02a2 100644
--- a/net/unix/scm.c
+++ b/net/unix/scm.c
@@ -51,10 +51,10 @@ void unix_inflight(struct user_struct *user, struct file *fp)
if (u) {
if (!u->inflight) {
- BUG_ON(!list_empty(&u->link));
+ WARN_ON_ONCE(!list_empty(&u->link));
list_add_tail(&u->link, &gc_inflight_list);
} else {
- BUG_ON(list_empty(&u->link));
+ WARN_ON_ONCE(list_empty(&u->link));
}
u->inflight++;
/* Paired with READ_ONCE() in wait_for_unix_gc() */
@@ -71,8 +71,8 @@ void unix_notinflight(struct user_struct *user, struct file *fp)
spin_lock(&unix_gc_lock);
if (u) {
- BUG_ON(!u->inflight);
- BUG_ON(list_empty(&u->link));
+ WARN_ON_ONCE(!u->inflight);
+ WARN_ON_ONCE(list_empty(&u->link));
u->inflight--;
if (!u->inflight)
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v1 net-next 2/3] af_unix: Remove io_uring code for GC.
2024-01-29 19:04 [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Kuniyuki Iwashima
2024-01-29 19:04 ` [PATCH v1 net-next 1/3] af_unix: Replace BUG_ON() with WARN_ON_ONCE() Kuniyuki Iwashima
@ 2024-01-29 19:04 ` Kuniyuki Iwashima
2024-02-12 2:17 ` Pengfei Xu
2024-01-29 19:04 ` [PATCH v1 net-next 3/3] af_unix: Remove CONFIG_UNIX_SCM Kuniyuki Iwashima
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: Kuniyuki Iwashima @ 2024-01-29 19:04 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Jens Axboe, Pavel Begunkov, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev
Since commit 705318a99a13 ("io_uring/af_unix: disable sending
io_uring over sockets"), io_uring's unix socket cannot be passed
via SCM_RIGHTS, so it does not contribute to cyclic reference and
no longer be candidate for garbage collection.
Also, commit 6e5e6d274956 ("io_uring: drop any code related to
SCM_RIGHTS") cleaned up SCM_RIGHTS code in io_uring.
Let's do it in AF_UNIX as well by reverting commit 0091bfc81741
("io_uring/af_unix: defer registered files gc to io_uring release")
and commit 10369080454d ("net: reclaim skb->scm_io_uring bit").
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
include/net/af_unix.h | 1 -
net/unix/garbage.c | 25 ++-----------------------
net/unix/scm.c | 6 ------
3 files changed, 2 insertions(+), 30 deletions(-)
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index f045bbd9017d..9e39b2ec4524 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -20,7 +20,6 @@ static inline struct unix_sock *unix_get_socket(struct file *filp)
void unix_inflight(struct user_struct *user, struct file *fp);
void unix_notinflight(struct user_struct *user, struct file *fp);
void unix_destruct_scm(struct sk_buff *skb);
-void io_uring_destruct_scm(struct sk_buff *skb);
void unix_gc(void);
void wait_for_unix_gc(struct scm_fp_list *fpl);
struct sock *unix_peer_get(struct sock *sk);
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index af676bb8fb67..ce5b5f87b16e 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -184,12 +184,10 @@ static bool gc_in_progress;
static void __unix_gc(struct work_struct *work)
{
- struct sk_buff *next_skb, *skb;
- struct unix_sock *u;
- struct unix_sock *next;
struct sk_buff_head hitlist;
- struct list_head cursor;
+ struct unix_sock *u, *next;
LIST_HEAD(not_cycle_list);
+ struct list_head cursor;
spin_lock(&unix_gc_lock);
@@ -269,30 +267,11 @@ static void __unix_gc(struct work_struct *work)
spin_unlock(&unix_gc_lock);
- /* We need io_uring to clean its registered files, ignore all io_uring
- * originated skbs. It's fine as io_uring doesn't keep references to
- * other io_uring instances and so killing all other files in the cycle
- * will put all io_uring references forcing it to go through normal
- * release.path eventually putting registered files.
- */
- skb_queue_walk_safe(&hitlist, skb, next_skb) {
- if (skb->destructor == io_uring_destruct_scm) {
- __skb_unlink(skb, &hitlist);
- skb_queue_tail(&skb->sk->sk_receive_queue, skb);
- }
- }
-
/* Here we are. Hitlist is filled. Die. */
__skb_queue_purge(&hitlist);
spin_lock(&unix_gc_lock);
- /* There could be io_uring registered files, just push them back to
- * the inflight list
- */
- list_for_each_entry_safe(u, next, &gc_candidates, link)
- list_move_tail(&u->link, &gc_inflight_list);
-
/* All candidates should have been detached by now. */
WARN_ON_ONCE(!list_empty(&gc_candidates));
diff --git a/net/unix/scm.c b/net/unix/scm.c
index 505e56cf02a2..db65b0ab5947 100644
--- a/net/unix/scm.c
+++ b/net/unix/scm.c
@@ -148,9 +148,3 @@ void unix_destruct_scm(struct sk_buff *skb)
sock_wfree(skb);
}
EXPORT_SYMBOL(unix_destruct_scm);
-
-void io_uring_destruct_scm(struct sk_buff *skb)
-{
- unix_destruct_scm(skb);
-}
-EXPORT_SYMBOL(io_uring_destruct_scm);
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v1 net-next 3/3] af_unix: Remove CONFIG_UNIX_SCM.
2024-01-29 19:04 [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Kuniyuki Iwashima
2024-01-29 19:04 ` [PATCH v1 net-next 1/3] af_unix: Replace BUG_ON() with WARN_ON_ONCE() Kuniyuki Iwashima
2024-01-29 19:04 ` [PATCH v1 net-next 2/3] af_unix: Remove io_uring code for GC Kuniyuki Iwashima
@ 2024-01-29 19:04 ` Kuniyuki Iwashima
2024-01-29 21:02 ` [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Jens Axboe
2024-02-01 1:30 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: Kuniyuki Iwashima @ 2024-01-29 19:04 UTC (permalink / raw)
To: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: Jens Axboe, Pavel Begunkov, Kuniyuki Iwashima, Kuniyuki Iwashima, netdev
Originally, the code related to garbage collection was all in garbage.c.
Commit f4e65870e5ce ("net: split out functions related to registering
inflight socket files") moved some functions to scm.c for io_uring and
added CONFIG_UNIX_SCM just in case AF_UNIX was built as module.
However, since commit 97154bcf4d1b ("af_unix: Kconfig: make CONFIG_UNIX
bool"), AF_UNIX is no longer built separately. Also, io_uring does not
support SCM_RIGHTS now.
Let's move the functions back to garbage.c
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
---
include/net/af_unix.h | 7 +-
net/Makefile | 2 +-
net/unix/Kconfig | 5 --
net/unix/Makefile | 2 -
net/unix/af_unix.c | 63 +++++++++++++++++-
net/unix/garbage.c | 73 +++++++++++++++++++-
net/unix/scm.c | 150 ------------------------------------------
net/unix/scm.h | 10 ---
8 files changed, 137 insertions(+), 175 deletions(-)
delete mode 100644 net/unix/scm.c
delete mode 100644 net/unix/scm.h
diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 9e39b2ec4524..54e346152eb1 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -17,19 +17,20 @@ static inline struct unix_sock *unix_get_socket(struct file *filp)
}
#endif
+extern spinlock_t unix_gc_lock;
+extern unsigned int unix_tot_inflight;
+
void unix_inflight(struct user_struct *user, struct file *fp);
void unix_notinflight(struct user_struct *user, struct file *fp);
-void unix_destruct_scm(struct sk_buff *skb);
void unix_gc(void);
void wait_for_unix_gc(struct scm_fp_list *fpl);
+
struct sock *unix_peer_get(struct sock *sk);
#define UNIX_HASH_MOD (256 - 1)
#define UNIX_HASH_SIZE (256 * 2)
#define UNIX_HASH_BITS 8
-extern unsigned int unix_tot_inflight;
-
struct unix_address {
refcount_t refcnt;
int len;
diff --git a/net/Makefile b/net/Makefile
index b06b5539e7a6..65bb8c72a35e 100644
--- a/net/Makefile
+++ b/net/Makefile
@@ -17,7 +17,7 @@ obj-$(CONFIG_NETFILTER) += netfilter/
obj-$(CONFIG_INET) += ipv4/
obj-$(CONFIG_TLS) += tls/
obj-$(CONFIG_XFRM) += xfrm/
-obj-$(CONFIG_UNIX_SCM) += unix/
+obj-$(CONFIG_UNIX) += unix/
obj-y += ipv6/
obj-$(CONFIG_PACKET) += packet/
obj-$(CONFIG_NET_KEY) += key/
diff --git a/net/unix/Kconfig b/net/unix/Kconfig
index 28b232f281ab..8b5d04210d7c 100644
--- a/net/unix/Kconfig
+++ b/net/unix/Kconfig
@@ -16,11 +16,6 @@ config UNIX
Say Y unless you know what you are doing.
-config UNIX_SCM
- bool
- depends on UNIX
- default y
-
config AF_UNIX_OOB
bool
depends on UNIX
diff --git a/net/unix/Makefile b/net/unix/Makefile
index 20491825b4d0..4ddd125c4642 100644
--- a/net/unix/Makefile
+++ b/net/unix/Makefile
@@ -11,5 +11,3 @@ unix-$(CONFIG_BPF_SYSCALL) += unix_bpf.o
obj-$(CONFIG_UNIX_DIAG) += unix_diag.o
unix_diag-y := diag.o
-
-obj-$(CONFIG_UNIX_SCM) += scm.o
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index 1720419d93d6..1cfbc586adb4 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -118,8 +118,6 @@
#include <linux/btf_ids.h>
#include <linux/bpf-cgroup.h>
-#include "scm.h"
-
static atomic_long_t unix_nr_socks;
static struct hlist_head bsd_socket_buckets[UNIX_HASH_SIZE / 2];
static spinlock_t bsd_socket_locks[UNIX_HASH_SIZE / 2];
@@ -1790,6 +1788,52 @@ static int unix_getname(struct socket *sock, struct sockaddr *uaddr, int peer)
return err;
}
+/* The "user->unix_inflight" variable is protected by the garbage
+ * collection lock, and we just read it locklessly here. If you go
+ * over the limit, there might be a tiny race in actually noticing
+ * it across threads. Tough.
+ */
+static inline bool too_many_unix_fds(struct task_struct *p)
+{
+ struct user_struct *user = current_user();
+
+ if (unlikely(READ_ONCE(user->unix_inflight) > task_rlimit(p, RLIMIT_NOFILE)))
+ return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
+ return false;
+}
+
+static int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
+{
+ int i;
+
+ if (too_many_unix_fds(current))
+ return -ETOOMANYREFS;
+
+ /* Need to duplicate file references for the sake of garbage
+ * collection. Otherwise a socket in the fps might become a
+ * candidate for GC while the skb is not yet queued.
+ */
+ UNIXCB(skb).fp = scm_fp_dup(scm->fp);
+ if (!UNIXCB(skb).fp)
+ return -ENOMEM;
+
+ for (i = scm->fp->count - 1; i >= 0; i--)
+ unix_inflight(scm->fp->user, scm->fp->fp[i]);
+
+ return 0;
+}
+
+static void unix_detach_fds(struct scm_cookie *scm, struct sk_buff *skb)
+{
+ int i;
+
+ scm->fp = UNIXCB(skb).fp;
+ UNIXCB(skb).fp = NULL;
+
+ for (i = scm->fp->count - 1; i >= 0; i--)
+ unix_notinflight(scm->fp->user, scm->fp->fp[i]);
+}
+
static void unix_peek_fds(struct scm_cookie *scm, struct sk_buff *skb)
{
scm->fp = scm_fp_dup(UNIXCB(skb).fp);
@@ -1837,6 +1881,21 @@ static void unix_peek_fds(struct scm_cookie *scm, struct sk_buff *skb)
spin_unlock(&unix_gc_lock);
}
+static void unix_destruct_scm(struct sk_buff *skb)
+{
+ struct scm_cookie scm;
+
+ memset(&scm, 0, sizeof(scm));
+ scm.pid = UNIXCB(skb).pid;
+ if (UNIXCB(skb).fp)
+ unix_detach_fds(&scm, skb);
+
+ /* Alas, it calls VFS */
+ /* So fscking what? fput() had been SMP-safe since the last Summer */
+ scm_destroy(&scm);
+ sock_wfree(skb);
+}
+
static int unix_scm_to_skb(struct scm_cookie *scm, struct sk_buff *skb, bool send_fds)
{
int err = 0;
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index ce5b5f87b16e..9b8473dd79a4 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -81,11 +81,80 @@
#include <net/scm.h>
#include <net/tcp_states.h>
-#include "scm.h"
+struct unix_sock *unix_get_socket(struct file *filp)
+{
+ struct inode *inode = file_inode(filp);
+
+ /* Socket ? */
+ if (S_ISSOCK(inode->i_mode) && !(filp->f_mode & FMODE_PATH)) {
+ struct socket *sock = SOCKET_I(inode);
+ const struct proto_ops *ops;
+ struct sock *sk = sock->sk;
-/* Internal data structures and random procedures: */
+ ops = READ_ONCE(sock->ops);
+ /* PF_UNIX ? */
+ if (sk && ops && ops->family == PF_UNIX)
+ return unix_sk(sk);
+ }
+
+ return NULL;
+}
+
+DEFINE_SPINLOCK(unix_gc_lock);
+unsigned int unix_tot_inflight;
static LIST_HEAD(gc_candidates);
+static LIST_HEAD(gc_inflight_list);
+
+/* Keep the number of times in flight count for the file
+ * descriptor if it is for an AF_UNIX socket.
+ */
+void unix_inflight(struct user_struct *user, struct file *filp)
+{
+ struct unix_sock *u = unix_get_socket(filp);
+
+ spin_lock(&unix_gc_lock);
+
+ if (u) {
+ if (!u->inflight) {
+ WARN_ON_ONCE(!list_empty(&u->link));
+ list_add_tail(&u->link, &gc_inflight_list);
+ } else {
+ WARN_ON_ONCE(list_empty(&u->link));
+ }
+ u->inflight++;
+
+ /* Paired with READ_ONCE() in wait_for_unix_gc() */
+ WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + 1);
+ }
+
+ WRITE_ONCE(user->unix_inflight, user->unix_inflight + 1);
+
+ spin_unlock(&unix_gc_lock);
+}
+
+void unix_notinflight(struct user_struct *user, struct file *filp)
+{
+ struct unix_sock *u = unix_get_socket(filp);
+
+ spin_lock(&unix_gc_lock);
+
+ if (u) {
+ WARN_ON_ONCE(!u->inflight);
+ WARN_ON_ONCE(list_empty(&u->link));
+
+ u->inflight--;
+ if (!u->inflight)
+ list_del_init(&u->link);
+
+ /* Paired with READ_ONCE() in wait_for_unix_gc() */
+ WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - 1);
+ }
+
+ WRITE_ONCE(user->unix_inflight, user->unix_inflight - 1);
+
+ spin_unlock(&unix_gc_lock);
+}
static void scan_inflight(struct sock *x, void (*func)(struct unix_sock *),
struct sk_buff_head *hitlist)
diff --git a/net/unix/scm.c b/net/unix/scm.c
deleted file mode 100644
index db65b0ab5947..000000000000
--- a/net/unix/scm.c
+++ /dev/null
@@ -1,150 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/string.h>
-#include <linux/socket.h>
-#include <linux/net.h>
-#include <linux/fs.h>
-#include <net/af_unix.h>
-#include <net/scm.h>
-#include <linux/init.h>
-#include <linux/io_uring.h>
-
-#include "scm.h"
-
-unsigned int unix_tot_inflight;
-EXPORT_SYMBOL(unix_tot_inflight);
-
-LIST_HEAD(gc_inflight_list);
-EXPORT_SYMBOL(gc_inflight_list);
-
-DEFINE_SPINLOCK(unix_gc_lock);
-EXPORT_SYMBOL(unix_gc_lock);
-
-struct unix_sock *unix_get_socket(struct file *filp)
-{
- struct inode *inode = file_inode(filp);
-
- /* Socket ? */
- if (S_ISSOCK(inode->i_mode) && !(filp->f_mode & FMODE_PATH)) {
- struct socket *sock = SOCKET_I(inode);
- const struct proto_ops *ops = READ_ONCE(sock->ops);
- struct sock *s = sock->sk;
-
- /* PF_UNIX ? */
- if (s && ops && ops->family == PF_UNIX)
- return unix_sk(s);
- }
-
- return NULL;
-}
-EXPORT_SYMBOL(unix_get_socket);
-
-/* Keep the number of times in flight count for the file
- * descriptor if it is for an AF_UNIX socket.
- */
-void unix_inflight(struct user_struct *user, struct file *fp)
-{
- struct unix_sock *u = unix_get_socket(fp);
-
- spin_lock(&unix_gc_lock);
-
- if (u) {
- if (!u->inflight) {
- WARN_ON_ONCE(!list_empty(&u->link));
- list_add_tail(&u->link, &gc_inflight_list);
- } else {
- WARN_ON_ONCE(list_empty(&u->link));
- }
- u->inflight++;
- /* Paired with READ_ONCE() in wait_for_unix_gc() */
- WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + 1);
- }
- WRITE_ONCE(user->unix_inflight, user->unix_inflight + 1);
- spin_unlock(&unix_gc_lock);
-}
-
-void unix_notinflight(struct user_struct *user, struct file *fp)
-{
- struct unix_sock *u = unix_get_socket(fp);
-
- spin_lock(&unix_gc_lock);
-
- if (u) {
- WARN_ON_ONCE(!u->inflight);
- WARN_ON_ONCE(list_empty(&u->link));
-
- u->inflight--;
- if (!u->inflight)
- list_del_init(&u->link);
- /* Paired with READ_ONCE() in wait_for_unix_gc() */
- WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - 1);
- }
- WRITE_ONCE(user->unix_inflight, user->unix_inflight - 1);
- spin_unlock(&unix_gc_lock);
-}
-
-/*
- * The "user->unix_inflight" variable is protected by the garbage
- * collection lock, and we just read it locklessly here. If you go
- * over the limit, there might be a tiny race in actually noticing
- * it across threads. Tough.
- */
-static inline bool too_many_unix_fds(struct task_struct *p)
-{
- struct user_struct *user = current_user();
-
- if (unlikely(READ_ONCE(user->unix_inflight) > task_rlimit(p, RLIMIT_NOFILE)))
- return !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN);
- return false;
-}
-
-int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb)
-{
- int i;
-
- if (too_many_unix_fds(current))
- return -ETOOMANYREFS;
-
- /*
- * Need to duplicate file references for the sake of garbage
- * collection. Otherwise a socket in the fps might become a
- * candidate for GC while the skb is not yet queued.
- */
- UNIXCB(skb).fp = scm_fp_dup(scm->fp);
- if (!UNIXCB(skb).fp)
- return -ENOMEM;
-
- for (i = scm->fp->count - 1; i >= 0; i--)
- unix_inflight(scm->fp->user, scm->fp->fp[i]);
- return 0;
-}
-EXPORT_SYMBOL(unix_attach_fds);
-
-void unix_detach_fds(struct scm_cookie *scm, struct sk_buff *skb)
-{
- int i;
-
- scm->fp = UNIXCB(skb).fp;
- UNIXCB(skb).fp = NULL;
-
- for (i = scm->fp->count-1; i >= 0; i--)
- unix_notinflight(scm->fp->user, scm->fp->fp[i]);
-}
-EXPORT_SYMBOL(unix_detach_fds);
-
-void unix_destruct_scm(struct sk_buff *skb)
-{
- struct scm_cookie scm;
-
- memset(&scm, 0, sizeof(scm));
- scm.pid = UNIXCB(skb).pid;
- if (UNIXCB(skb).fp)
- unix_detach_fds(&scm, skb);
-
- /* Alas, it calls VFS */
- /* So fscking what? fput() had been SMP-safe since the last Summer */
- scm_destroy(&scm);
- sock_wfree(skb);
-}
-EXPORT_SYMBOL(unix_destruct_scm);
diff --git a/net/unix/scm.h b/net/unix/scm.h
deleted file mode 100644
index 5a255a477f16..000000000000
--- a/net/unix/scm.h
+++ /dev/null
@@ -1,10 +0,0 @@
-#ifndef NET_UNIX_SCM_H
-#define NET_UNIX_SCM_H
-
-extern struct list_head gc_inflight_list;
-extern spinlock_t unix_gc_lock;
-
-int unix_attach_fds(struct scm_cookie *scm, struct sk_buff *skb);
-void unix_detach_fds(struct scm_cookie *scm, struct sk_buff *skb);
-
-#endif
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC.
2024-01-29 19:04 [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Kuniyuki Iwashima
` (2 preceding siblings ...)
2024-01-29 19:04 ` [PATCH v1 net-next 3/3] af_unix: Remove CONFIG_UNIX_SCM Kuniyuki Iwashima
@ 2024-01-29 21:02 ` Jens Axboe
2024-02-01 1:30 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: Jens Axboe @ 2024-01-29 21:02 UTC (permalink / raw)
To: Kuniyuki Iwashima, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni
Cc: Pavel Begunkov, Kuniyuki Iwashima, netdev
On 1/29/24 12:04 PM, Kuniyuki Iwashima wrote:
> I will post another series that rewrites the garbage collector for
> AF_UNIX socket.
>
> This is a prep series to clean up changes to GC made by io_uring but
> now not necessary.
Didn't look at this in detail, but it looks like it's just reverting the
changes we had to make there originally to accommodate io_uring. Hence
they are fine to get reverted:
Acked-by: Jens Axboe <axboe@kernel.dk>
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC.
2024-01-29 19:04 [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Kuniyuki Iwashima
` (3 preceding siblings ...)
2024-01-29 21:02 ` [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Jens Axboe
@ 2024-02-01 1:30 ` patchwork-bot+netdevbpf
4 siblings, 0 replies; 9+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-02-01 1:30 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: davem, edumazet, kuba, pabeni, axboe, asml.silence, kuni1840, netdev
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:
On Mon, 29 Jan 2024 11:04:32 -0800 you wrote:
> I will post another series that rewrites the garbage collector for
> AF_UNIX socket.
>
> This is a prep series to clean up changes to GC made by io_uring but
> now not necessary.
>
>
> [...]
Here is the summary with links:
- [v1,net-next,1/3] af_unix: Replace BUG_ON() with WARN_ON_ONCE().
https://git.kernel.org/netdev/net-next/c/d0f6dc263468
- [v1,net-next,2/3] af_unix: Remove io_uring code for GC.
https://git.kernel.org/netdev/net-next/c/11498715f266
- [v1,net-next,3/3] af_unix: Remove CONFIG_UNIX_SCM.
https://git.kernel.org/netdev/net-next/c/99a7a5b9943e
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 net-next 2/3] af_unix: Remove io_uring code for GC.
2024-01-29 19:04 ` [PATCH v1 net-next 2/3] af_unix: Remove io_uring code for GC Kuniyuki Iwashima
@ 2024-02-12 2:17 ` Pengfei Xu
2024-02-12 17:47 ` Jens Axboe
0 siblings, 1 reply; 9+ messages in thread
From: Pengfei Xu @ 2024-02-12 2:17 UTC (permalink / raw)
To: Kuniyuki Iwashima
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Jens Axboe, Pavel Begunkov, Kuniyuki Iwashima, netdev
Hi,
On 2024-01-29 at 11:04:34 -0800, Kuniyuki Iwashima wrote:
> Since commit 705318a99a13 ("io_uring/af_unix: disable sending
> io_uring over sockets"), io_uring's unix socket cannot be passed
> via SCM_RIGHTS, so it does not contribute to cyclic reference and
> no longer be candidate for garbage collection.
>
> Also, commit 6e5e6d274956 ("io_uring: drop any code related to
> SCM_RIGHTS") cleaned up SCM_RIGHTS code in io_uring.
>
> Let's do it in AF_UNIX as well by reverting commit 0091bfc81741
> ("io_uring/af_unix: defer registered files gc to io_uring release")
> and commit 10369080454d ("net: reclaim skb->scm_io_uring bit").
>
> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> ---
> include/net/af_unix.h | 1 -
> net/unix/garbage.c | 25 ++-----------------------
> net/unix/scm.c | 6 ------
> 3 files changed, 2 insertions(+), 30 deletions(-)
>
> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> index f045bbd9017d..9e39b2ec4524 100644
> --- a/include/net/af_unix.h
> +++ b/include/net/af_unix.h
> @@ -20,7 +20,6 @@ static inline struct unix_sock *unix_get_socket(struct file *filp)
> void unix_inflight(struct user_struct *user, struct file *fp);
> void unix_notinflight(struct user_struct *user, struct file *fp);
> void unix_destruct_scm(struct sk_buff *skb);
> -void io_uring_destruct_scm(struct sk_buff *skb);
> void unix_gc(void);
> void wait_for_unix_gc(struct scm_fp_list *fpl);
> struct sock *unix_peer_get(struct sock *sk);
> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> index af676bb8fb67..ce5b5f87b16e 100644
> --- a/net/unix/garbage.c
> +++ b/net/unix/garbage.c
> @@ -184,12 +184,10 @@ static bool gc_in_progress;
>
> static void __unix_gc(struct work_struct *work)
> {
> - struct sk_buff *next_skb, *skb;
> - struct unix_sock *u;
> - struct unix_sock *next;
> struct sk_buff_head hitlist;
> - struct list_head cursor;
> + struct unix_sock *u, *next;
> LIST_HEAD(not_cycle_list);
> + struct list_head cursor;
>
> spin_lock(&unix_gc_lock);
>
> @@ -269,30 +267,11 @@ static void __unix_gc(struct work_struct *work)
>
> spin_unlock(&unix_gc_lock);
>
> - /* We need io_uring to clean its registered files, ignore all io_uring
> - * originated skbs. It's fine as io_uring doesn't keep references to
> - * other io_uring instances and so killing all other files in the cycle
> - * will put all io_uring references forcing it to go through normal
> - * release.path eventually putting registered files.
> - */
> - skb_queue_walk_safe(&hitlist, skb, next_skb) {
> - if (skb->destructor == io_uring_destruct_scm) {
> - __skb_unlink(skb, &hitlist);
> - skb_queue_tail(&skb->sk->sk_receive_queue, skb);
> - }
> - }
> -
> /* Here we are. Hitlist is filled. Die. */
> __skb_queue_purge(&hitlist);
>
> spin_lock(&unix_gc_lock);
>
> - /* There could be io_uring registered files, just push them back to
> - * the inflight list
> - */
> - list_for_each_entry_safe(u, next, &gc_candidates, link)
> - list_move_tail(&u->link, &gc_inflight_list);
> -
> /* All candidates should have been detached by now. */
> WARN_ON_ONCE(!list_empty(&gc_candidates));
>
> diff --git a/net/unix/scm.c b/net/unix/scm.c
> index 505e56cf02a2..db65b0ab5947 100644
> --- a/net/unix/scm.c
> +++ b/net/unix/scm.c
> @@ -148,9 +148,3 @@ void unix_destruct_scm(struct sk_buff *skb)
> sock_wfree(skb);
> }
> EXPORT_SYMBOL(unix_destruct_scm);
> -
> -void io_uring_destruct_scm(struct sk_buff *skb)
> -{
> - unix_destruct_scm(skb);
> -}
> -EXPORT_SYMBOL(io_uring_destruct_scm);
Syzkaller found below issue.
There is WARNING in __unix_gc in v6.8-rc3_internal-devel_hourly-20240205-094544,
the kernel contains kernel-next patches.
Bisected and found first bad commit:
"
11498715f266 af_unix: Remove io_uring code for GC.
"
It's the same patch as above.
All detailed info: https://github.com/xupengfe/syzkaller_logs/tree/main/240211_144134___unix_gc
Syzkaller reproduced code: https://github.com/xupengfe/syzkaller_logs/blob/main/240211_144134___unix_gc/repro.c
Syzkaller repro syscall steps: https://github.com/xupengfe/syzkaller_logs/blob/main/240211_144134___unix_gc/repro.prog
Kconfig(make olddefconfig): https://github.com/xupengfe/syzkaller_logs/blob/main/240211_144134___unix_gc/kconfig_origin
Bisect info: https://github.com/xupengfe/syzkaller_logs/blob/main/240211_144134___unix_gc/bisect_info.log
Issue dmesg: https://github.com/xupengfe/syzkaller_logs/blob/main/240211_144134___unix_gc/3561c4956a5c9e7f995ae47d4ef703eb9c6a93cd_dmesg.log
bzImage: https://github.com/xupengfe/syzkaller_logs/raw/main/240211_144134___unix_gc/bzImage_3561c4956a5c.tar.gz
repro.report: https://github.com/xupengfe/syzkaller_logs/blob/main/240211_144134___unix_gc/repro.report
"
[ 27.629798] ------------[ cut here ]------------
[ 27.630447] WARNING: CPU: 0 PID: 52 at net/unix/garbage.c:345 __unix_gc+0x99e/0xb50
[ 27.631312] Modules linked in:
[ 27.631671] CPU: 0 PID: 52 Comm: kworker/u4:3 Not tainted 6.8.0-rc3-3561c4956a5c+ #1
[ 27.632787] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
[ 27.634018] Workqueue: events_unbound __unix_gc
[ 27.634544] RIP: 0010:__unix_gc+0x99e/0xb50
[ 27.635026] Code: b2 4f fc 0f 0b e9 6c f8 ff ff e8 0d b2 4f fc 31 d2 48 c7 c6 e0 8f 12 85 4c 89 e7 e8 ec f1 ff ff e9 32 fb ff ff e8 f2 b1 4f fc <0f> 0b e9 7e fe ff ff 4c 89 e7 e8 c3 dd b0 fc e9 9c fa ff ff e8 b9
[ 27.637177] RSP: 0018:ffff88800c677b90 EFLAGS: 00010293
[ 27.637768] RAX: 0000000000000000 RBX: dffffc0000000000 RCX: ffffffff8140b3c2
[ 27.638544] RDX: ffff88800bfbca00 RSI: ffffffff8512a5be RDI: ffff88800c677af8
[ 27.639329] RBP: ffff88800c677cc8 R08: 0000000000000001 R09: ffffed10018cef5f
[ 27.640112] R10: 0000000000000003 R11: 0000000000000001 R12: ffff88800c677c00
[ 27.640992] R13: ffff88800c677c00 R14: ffff88800c677c00 R15: ffff88800c677c00
[ 27.641768] FS: 0000000000000000(0000) GS:ffff88806cc00000(0000) knlGS:0000000000000000
[ 27.642646] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 27.643285] CR2: 00007f6063373fa8 CR3: 0000000006a7e004 CR4: 0000000000770ef0
[ 27.644069] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 27.644876] DR3: 0000000000000000 DR6: 00000000ffff07f0 DR7: 0000000000000400
[ 27.645658] PKRU: 55555554
[ 27.645974] Call Trace:
[ 27.646266] <TASK>
[ 27.646524] ? show_regs+0xa9/0xc0
[ 27.646933] ? __warn+0xef/0x340
[ 27.647317] ? report_bug+0x25e/0x4b0
[ 27.647748] ? __unix_gc+0x99e/0xb50
[ 27.648190] ? report_bug+0x2cb/0x4b0
[ 27.648630] ? __unix_gc+0x99e/0xb50
[ 27.649049] ? handle_bug+0xa2/0x130
[ 27.649470] ? exc_invalid_op+0x3c/0x80
[ 27.649922] ? asm_exc_invalid_op+0x1f/0x30
[ 27.650416] ? do_raw_spin_lock+0x142/0x290
[ 27.650892] ? __unix_gc+0x99e/0xb50
[ 27.651315] ? __unix_gc+0x99e/0xb50
[ 27.651742] ? __pfx___unix_gc+0x10/0x10
[ 27.652209] ? __this_cpu_preempt_check+0x21/0x30
[ 27.652757] ? lock_acquire+0x1d9/0x530
[ 27.653219] ? __this_cpu_preempt_check+0x21/0x30
[ 27.653754] ? _raw_spin_unlock_irq+0x2c/0x60
[ 27.654267] process_one_work+0x813/0x15a0
[ 27.654757] ? __pfx_process_one_work+0x10/0x10
[ 27.655271] ? move_linked_works+0x1bf/0x2c0
[ 27.655767] ? __this_cpu_preempt_check+0x21/0x30
[ 27.656346] ? assign_work+0x19f/0x250
[ 27.656780] ? lock_is_held_type+0xf0/0x150
[ 27.657267] worker_thread+0x823/0x11a0
[ 27.657710] ? _raw_spin_unlock_irqrestore+0x35/0x70
[ 27.658269] ? trace_hardirqs_on+0x26/0x120
[ 27.658771] kthread+0x35f/0x470
[ 27.659153] ? __pfx_worker_thread+0x10/0x10
[ 27.659647] ? __pfx_kthread+0x10/0x10
[ 27.660089] ret_from_fork+0x56/0x90
[ 27.660535] ? __pfx_kthread+0x10/0x10
[ 27.660973] ret_from_fork_asm+0x1b/0x30
[ 27.661451] </TASK>
[ 27.661715] irq event stamp: 12659
[ 27.662104] hardirqs last enabled at (12667): [<ffffffff814359a5>] console_unlock+0x2d5/0x310
[ 27.663049] hardirqs last disabled at (12674): [<ffffffff8143598a>] console_unlock+0x2ba/0x310
[ 27.663991] softirqs last enabled at (12306): [<ffffffff8126fcf8>] __irq_exit_rcu+0xa8/0x110
[ 27.664946] softirqs last disabled at (12291): [<ffffffff8126fcf8>] __irq_exit_rcu+0xa8/0x110
[ 27.665878] ---[ end trace 0000000000000000 ]---
"
As above WARNING and bisect info, do you mind to take a look?
Thanks!
---
If you don't need the following environment to reproduce the problem or if you
already have one reproduced environment, please ignore the following information.
How to reproduce:
git clone https://gitlab.com/xupengfe/repro_vm_env.git
cd repro_vm_env
tar -xvf repro_vm_env.tar.gz
cd repro_vm_env; ./start3.sh // it needs qemu-system-x86_64 and I used v7.1.0
// start3.sh will load bzImage_2241ab53cbb5cdb08a6b2d4688feb13971058f65 v6.2-rc5 kernel
// You could change the bzImage_xxx as you want
// Maybe you need to remove line "-drive if=pflash,format=raw,readonly=on,file=./OVMF_CODE.fd \" for different qemu version
You could use below command to log in, there is no password for root.
ssh -p 10023 root@localhost
After login vm(virtual machine) successfully, you could transfer reproduced
binary to the vm by below way, and reproduce the problem in vm:
gcc -pthread -o repro repro.c
scp -P 10023 repro root@localhost:/root/
Get the bzImage for target kernel:
Please use target kconfig and copy it to kernel_src/.config
make olddefconfig
make -jx bzImage //x should equal or less than cpu num your pc has
Fill the bzImage file into above start3.sh to load the target kernel in vm.
Tips:
If you already have qemu-system-x86_64, please ignore below info.
If you want to install qemu v7.1.0 version:
git clone https://github.com/qemu/qemu.git
cd qemu
git checkout -f v7.1.0
mkdir build
cd build
yum install -y ninja-build.x86_64
yum -y install libslirp-devel.x86_64
../configure --target-list=x86_64-softmmu --enable-kvm --enable-vnc --enable-gtk --enable-sdl --enable-usb-redir --enable-slirp
make
make install
Best Regards,
Thanks!
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 net-next 2/3] af_unix: Remove io_uring code for GC.
2024-02-12 2:17 ` Pengfei Xu
@ 2024-02-12 17:47 ` Jens Axboe
2024-02-14 1:08 ` Pengfei Xu
0 siblings, 1 reply; 9+ messages in thread
From: Jens Axboe @ 2024-02-12 17:47 UTC (permalink / raw)
To: Pengfei Xu, Kuniyuki Iwashima
Cc: David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Pavel Begunkov, Kuniyuki Iwashima, netdev
On 2/11/24 7:17 PM, Pengfei Xu wrote:
> Hi,
>
> On 2024-01-29 at 11:04:34 -0800, Kuniyuki Iwashima wrote:
>> Since commit 705318a99a13 ("io_uring/af_unix: disable sending
>> io_uring over sockets"), io_uring's unix socket cannot be passed
>> via SCM_RIGHTS, so it does not contribute to cyclic reference and
>> no longer be candidate for garbage collection.
>>
>> Also, commit 6e5e6d274956 ("io_uring: drop any code related to
>> SCM_RIGHTS") cleaned up SCM_RIGHTS code in io_uring.
>>
>> Let's do it in AF_UNIX as well by reverting commit 0091bfc81741
>> ("io_uring/af_unix: defer registered files gc to io_uring release")
>> and commit 10369080454d ("net: reclaim skb->scm_io_uring bit").
>>
>> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
>> ---
>> include/net/af_unix.h | 1 -
>> net/unix/garbage.c | 25 ++-----------------------
>> net/unix/scm.c | 6 ------
>> 3 files changed, 2 insertions(+), 30 deletions(-)
>>
>> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
>> index f045bbd9017d..9e39b2ec4524 100644
>> --- a/include/net/af_unix.h
>> +++ b/include/net/af_unix.h
>> @@ -20,7 +20,6 @@ static inline struct unix_sock *unix_get_socket(struct file *filp)
>> void unix_inflight(struct user_struct *user, struct file *fp);
>> void unix_notinflight(struct user_struct *user, struct file *fp);
>> void unix_destruct_scm(struct sk_buff *skb);
>> -void io_uring_destruct_scm(struct sk_buff *skb);
>> void unix_gc(void);
>> void wait_for_unix_gc(struct scm_fp_list *fpl);
>> struct sock *unix_peer_get(struct sock *sk);
>> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
>> index af676bb8fb67..ce5b5f87b16e 100644
>> --- a/net/unix/garbage.c
>> +++ b/net/unix/garbage.c
>> @@ -184,12 +184,10 @@ static bool gc_in_progress;
>>
>> static void __unix_gc(struct work_struct *work)
>> {
>> - struct sk_buff *next_skb, *skb;
>> - struct unix_sock *u;
>> - struct unix_sock *next;
>> struct sk_buff_head hitlist;
>> - struct list_head cursor;
>> + struct unix_sock *u, *next;
>> LIST_HEAD(not_cycle_list);
>> + struct list_head cursor;
>>
>> spin_lock(&unix_gc_lock);
>>
>> @@ -269,30 +267,11 @@ static void __unix_gc(struct work_struct *work)
>>
>> spin_unlock(&unix_gc_lock);
>>
>> - /* We need io_uring to clean its registered files, ignore all io_uring
>> - * originated skbs. It's fine as io_uring doesn't keep references to
>> - * other io_uring instances and so killing all other files in the cycle
>> - * will put all io_uring references forcing it to go through normal
>> - * release.path eventually putting registered files.
>> - */
>> - skb_queue_walk_safe(&hitlist, skb, next_skb) {
>> - if (skb->destructor == io_uring_destruct_scm) {
>> - __skb_unlink(skb, &hitlist);
>> - skb_queue_tail(&skb->sk->sk_receive_queue, skb);
>> - }
>> - }
>> -
>> /* Here we are. Hitlist is filled. Die. */
>> __skb_queue_purge(&hitlist);
>>
>> spin_lock(&unix_gc_lock);
>>
>> - /* There could be io_uring registered files, just push them back to
>> - * the inflight list
>> - */
>> - list_for_each_entry_safe(u, next, &gc_candidates, link)
>> - list_move_tail(&u->link, &gc_inflight_list);
>> -
>> /* All candidates should have been detached by now. */
>> WARN_ON_ONCE(!list_empty(&gc_candidates));
>>
>> diff --git a/net/unix/scm.c b/net/unix/scm.c
>> index 505e56cf02a2..db65b0ab5947 100644
>> --- a/net/unix/scm.c
>> +++ b/net/unix/scm.c
>> @@ -148,9 +148,3 @@ void unix_destruct_scm(struct sk_buff *skb)
>> sock_wfree(skb);
>> }
>> EXPORT_SYMBOL(unix_destruct_scm);
>> -
>> -void io_uring_destruct_scm(struct sk_buff *skb)
>> -{
>> - unix_destruct_scm(skb);
>> -}
>> -EXPORT_SYMBOL(io_uring_destruct_scm);
>
> Syzkaller found below issue.
> There is WARNING in __unix_gc in v6.8-rc3_internal-devel_hourly-20240205-094544,
> the kernel contains kernel-next patches.
>
> Bisected and found first bad commit:
> "
> 11498715f266 af_unix: Remove io_uring code for GC.
> "
> It's the same patch as above.
It should be fixed by:
commit 1279f9d9dec2d7462823a18c29ad61359e0a007d
Author: Kuniyuki Iwashima <kuniyu@amazon.com>
Date: Sat Feb 3 10:31:49 2024 -0800
af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.
which is in Linus's tree.
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v1 net-next 2/3] af_unix: Remove io_uring code for GC.
2024-02-12 17:47 ` Jens Axboe
@ 2024-02-14 1:08 ` Pengfei Xu
0 siblings, 0 replies; 9+ messages in thread
From: Pengfei Xu @ 2024-02-14 1:08 UTC (permalink / raw)
To: Jens Axboe
Cc: Kuniyuki Iwashima, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Pavel Begunkov, Kuniyuki Iwashima, netdev
Hi Jens Axboe,
On 2024-02-12 at 10:47:20 -0700, Jens Axboe wrote:
> On 2/11/24 7:17 PM, Pengfei Xu wrote:
> > Hi,
> >
> > On 2024-01-29 at 11:04:34 -0800, Kuniyuki Iwashima wrote:
> >> Since commit 705318a99a13 ("io_uring/af_unix: disable sending
> >> io_uring over sockets"), io_uring's unix socket cannot be passed
> >> via SCM_RIGHTS, so it does not contribute to cyclic reference and
> >> no longer be candidate for garbage collection.
> >>
> >> Also, commit 6e5e6d274956 ("io_uring: drop any code related to
> >> SCM_RIGHTS") cleaned up SCM_RIGHTS code in io_uring.
> >>
> >> Let's do it in AF_UNIX as well by reverting commit 0091bfc81741
> >> ("io_uring/af_unix: defer registered files gc to io_uring release")
> >> and commit 10369080454d ("net: reclaim skb->scm_io_uring bit").
> >>
> >> Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> >> ---
> >> include/net/af_unix.h | 1 -
> >> net/unix/garbage.c | 25 ++-----------------------
> >> net/unix/scm.c | 6 ------
> >> 3 files changed, 2 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/include/net/af_unix.h b/include/net/af_unix.h
> >> index f045bbd9017d..9e39b2ec4524 100644
> >> --- a/include/net/af_unix.h
> >> +++ b/include/net/af_unix.h
> >> @@ -20,7 +20,6 @@ static inline struct unix_sock *unix_get_socket(struct file *filp)
> >> void unix_inflight(struct user_struct *user, struct file *fp);
> >> void unix_notinflight(struct user_struct *user, struct file *fp);
> >> void unix_destruct_scm(struct sk_buff *skb);
> >> -void io_uring_destruct_scm(struct sk_buff *skb);
> >> void unix_gc(void);
> >> void wait_for_unix_gc(struct scm_fp_list *fpl);
> >> struct sock *unix_peer_get(struct sock *sk);
> >> diff --git a/net/unix/garbage.c b/net/unix/garbage.c
> >> index af676bb8fb67..ce5b5f87b16e 100644
> >> --- a/net/unix/garbage.c
> >> +++ b/net/unix/garbage.c
> >> @@ -184,12 +184,10 @@ static bool gc_in_progress;
> >>
> >> static void __unix_gc(struct work_struct *work)
> >> {
> >> - struct sk_buff *next_skb, *skb;
> >> - struct unix_sock *u;
> >> - struct unix_sock *next;
> >> struct sk_buff_head hitlist;
> >> - struct list_head cursor;
> >> + struct unix_sock *u, *next;
> >> LIST_HEAD(not_cycle_list);
> >> + struct list_head cursor;
> >>
> >> spin_lock(&unix_gc_lock);
> >>
> >> @@ -269,30 +267,11 @@ static void __unix_gc(struct work_struct *work)
> >>
> >> spin_unlock(&unix_gc_lock);
> >>
> >> - /* We need io_uring to clean its registered files, ignore all io_uring
> >> - * originated skbs. It's fine as io_uring doesn't keep references to
> >> - * other io_uring instances and so killing all other files in the cycle
> >> - * will put all io_uring references forcing it to go through normal
> >> - * release.path eventually putting registered files.
> >> - */
> >> - skb_queue_walk_safe(&hitlist, skb, next_skb) {
> >> - if (skb->destructor == io_uring_destruct_scm) {
> >> - __skb_unlink(skb, &hitlist);
> >> - skb_queue_tail(&skb->sk->sk_receive_queue, skb);
> >> - }
> >> - }
> >> -
> >> /* Here we are. Hitlist is filled. Die. */
> >> __skb_queue_purge(&hitlist);
> >>
> >> spin_lock(&unix_gc_lock);
> >>
> >> - /* There could be io_uring registered files, just push them back to
> >> - * the inflight list
> >> - */
> >> - list_for_each_entry_safe(u, next, &gc_candidates, link)
> >> - list_move_tail(&u->link, &gc_inflight_list);
> >> -
> >> /* All candidates should have been detached by now. */
> >> WARN_ON_ONCE(!list_empty(&gc_candidates));
> >>
> >> diff --git a/net/unix/scm.c b/net/unix/scm.c
> >> index 505e56cf02a2..db65b0ab5947 100644
> >> --- a/net/unix/scm.c
> >> +++ b/net/unix/scm.c
> >> @@ -148,9 +148,3 @@ void unix_destruct_scm(struct sk_buff *skb)
> >> sock_wfree(skb);
> >> }
> >> EXPORT_SYMBOL(unix_destruct_scm);
> >> -
> >> -void io_uring_destruct_scm(struct sk_buff *skb)
> >> -{
> >> - unix_destruct_scm(skb);
> >> -}
> >> -EXPORT_SYMBOL(io_uring_destruct_scm);
> >
> > Syzkaller found below issue.
> > There is WARNING in __unix_gc in v6.8-rc3_internal-devel_hourly-20240205-094544,
> > the kernel contains kernel-next patches.
> >
> > Bisected and found first bad commit:
> > "
> > 11498715f266 af_unix: Remove io_uring code for GC.
> > "
> > It's the same patch as above.
>
> It should be fixed by:
>
> commit 1279f9d9dec2d7462823a18c29ad61359e0a007d
> Author: Kuniyuki Iwashima <kuniyu@amazon.com>
> Date: Sat Feb 3 10:31:49 2024 -0800
>
> af_unix: Call kfree_skb() for dead unix_(sk)->oob_skb in GC.
>
> which is in Linus's tree.
Thank you for the commit tip for the fix. This is indeed the same problem and
has been fixed.
I will check the community mail carefully next time to avoid reporting
duplicate problems.
Best Regards,
Thanks!
>
> --
> Jens Axboe
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-02-14 1:13 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-29 19:04 [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Kuniyuki Iwashima
2024-01-29 19:04 ` [PATCH v1 net-next 1/3] af_unix: Replace BUG_ON() with WARN_ON_ONCE() Kuniyuki Iwashima
2024-01-29 19:04 ` [PATCH v1 net-next 2/3] af_unix: Remove io_uring code for GC Kuniyuki Iwashima
2024-02-12 2:17 ` Pengfei Xu
2024-02-12 17:47 ` Jens Axboe
2024-02-14 1:08 ` Pengfei Xu
2024-01-29 19:04 ` [PATCH v1 net-next 3/3] af_unix: Remove CONFIG_UNIX_SCM Kuniyuki Iwashima
2024-01-29 21:02 ` [PATCH v1 net-next 0/3] af_unix: Remove io_uring dead code in GC Jens Axboe
2024-02-01 1:30 ` patchwork-bot+netdevbpf
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.