All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.