netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lee Jones <lee@kernel.org>
To: lee@kernel.org, "David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Kuniyuki Iwashima <kuniyu@amazon.com>,
	Jens Axboe <axboe@kernel.dk>, Sasha Levin <sashal@kernel.org>,
	Michal Luczaj <mhal@rbox.co>, Rao Shoaib <Rao.Shoaib@oracle.com>,
	Pavel Begunkov <asml.silence@gmail.com>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Cc: stable@vger.kernel.org, kernel test robot <oliver.sang@intel.com>
Subject: [PATCH v6.6 22/26] af_unix: Try not to hold unix_gc_lock during accept().
Date: Wed, 21 May 2025 14:45:30 +0000	[thread overview]
Message-ID: <20250521144803.2050504-23-lee@kernel.org> (raw)
In-Reply-To: <20250521144803.2050504-1-lee@kernel.org>

From: Kuniyuki Iwashima <kuniyu@amazon.com>

[ Upstream commit fd86344823b521149bb31d91eba900ba3525efa6 ]

Commit dcf70df2048d ("af_unix: Fix up unix_edge.successor for embryo
socket.") added spin_lock(&unix_gc_lock) in accept() path, and it
caused regression in a stress test as reported by kernel test robot.

If the embryo socket is not part of the inflight graph, we need not
hold the lock.

To decide that in O(1) time and avoid the regression in the normal
use case,

  1. add a new stat unix_sk(sk)->scm_stat.nr_unix_fds

  2. count the number of inflight AF_UNIX sockets in the receive
     queue under unix_state_lock()

  3. move unix_update_edges() call under unix_state_lock()

  4. avoid locking if nr_unix_fds is 0 in unix_update_edges()

Reported-by: kernel test robot <oliver.sang@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202404101427.92a08551-oliver.sang@intel.com
Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Link: https://lore.kernel.org/r/20240413021928.20946-1-kuniyu@amazon.com
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
(cherry picked from commit fd86344823b521149bb31d91eba900ba3525efa6)
Signed-off-by: Lee Jones <lee@kernel.org>
---
 include/net/af_unix.h |  1 +
 net/unix/af_unix.c    |  2 +-
 net/unix/garbage.c    | 20 ++++++++++++++++----
 3 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/include/net/af_unix.h b/include/net/af_unix.h
index 47042de4a2a9c..b6eedf7650da5 100644
--- a/include/net/af_unix.h
+++ b/include/net/af_unix.h
@@ -67,6 +67,7 @@ struct unix_skb_parms {
 
 struct scm_stat {
 	atomic_t nr_fds;
+	unsigned long nr_unix_fds;
 };
 
 #define UNIXCB(skb)	(*(struct unix_skb_parms *)&((skb)->cb))
diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c
index df70d8a7ee837..236a2cd2bc93d 100644
--- a/net/unix/af_unix.c
+++ b/net/unix/af_unix.c
@@ -1703,12 +1703,12 @@ static int unix_accept(struct socket *sock, struct socket *newsock, int flags,
 	}
 
 	tsk = skb->sk;
-	unix_update_edges(unix_sk(tsk));
 	skb_free_datagram(sk, skb);
 	wake_up_interruptible(&unix_sk(sk)->peer_wait);
 
 	/* attach accepted sock to socket */
 	unix_state_lock(tsk);
+	unix_update_edges(unix_sk(tsk));
 	newsock->state = SS_CONNECTED;
 	unix_sock_inherit_flags(sock, newsock);
 	sock_graft(tsk, newsock);
diff --git a/net/unix/garbage.c b/net/unix/garbage.c
index 12a4ec27e0d4d..95240a59808f2 100644
--- a/net/unix/garbage.c
+++ b/net/unix/garbage.c
@@ -209,6 +209,7 @@ void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver)
 		unix_add_edge(fpl, edge);
 	} while (i < fpl->count_unix);
 
+	receiver->scm_stat.nr_unix_fds += fpl->count_unix;
 	WRITE_ONCE(unix_tot_inflight, unix_tot_inflight + fpl->count_unix);
 out:
 	WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight + fpl->count);
@@ -222,6 +223,7 @@ void unix_add_edges(struct scm_fp_list *fpl, struct unix_sock *receiver)
 
 void unix_del_edges(struct scm_fp_list *fpl)
 {
+	struct unix_sock *receiver;
 	int i = 0;
 
 	spin_lock(&unix_gc_lock);
@@ -235,6 +237,8 @@ void unix_del_edges(struct scm_fp_list *fpl)
 		unix_del_edge(fpl, edge);
 	} while (i < fpl->count_unix);
 
+	receiver = fpl->edges[0].successor;
+	receiver->scm_stat.nr_unix_fds -= fpl->count_unix;
 	WRITE_ONCE(unix_tot_inflight, unix_tot_inflight - fpl->count_unix);
 out:
 	WRITE_ONCE(fpl->user->unix_inflight, fpl->user->unix_inflight - fpl->count);
@@ -246,10 +250,18 @@ void unix_del_edges(struct scm_fp_list *fpl)
 
 void unix_update_edges(struct unix_sock *receiver)
 {
-	spin_lock(&unix_gc_lock);
-	unix_update_graph(unix_sk(receiver->listener)->vertex);
-	receiver->listener = NULL;
-	spin_unlock(&unix_gc_lock);
+	/* nr_unix_fds is only updated under unix_state_lock().
+	 * If it's 0 here, the embryo socket is not part of the
+	 * inflight graph, and GC will not see it, so no lock needed.
+	 */
+	if (!receiver->scm_stat.nr_unix_fds) {
+		receiver->listener = NULL;
+	} else {
+		spin_lock(&unix_gc_lock);
+		unix_update_graph(unix_sk(receiver->listener)->vertex);
+		receiver->listener = NULL;
+		spin_unlock(&unix_gc_lock);
+	}
 }
 
 int unix_prepare_fpl(struct scm_fp_list *fpl)
-- 
2.49.0.1112.g889b7c5bd8-goog


  parent reply	other threads:[~2025-05-21 14:52 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-05-21 14:45 [PATCH v6.6 00/26] af_unix: Align with upstream to avoid a potential UAF Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 01/26] af_unix: Return struct unix_sock from unix_get_socket() Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 02/26] af_unix: Run GC on only one CPU Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 03/26] af_unix: Try to run GC async Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 04/26] af_unix: Replace BUG_ON() with WARN_ON_ONCE() Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 05/26] af_unix: Remove io_uring code for GC Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 06/26] af_unix: Remove CONFIG_UNIX_SCM Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 07/26] af_unix: Allocate struct unix_vertex for each inflight AF_UNIX fd Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 08/26] af_unix: Allocate struct unix_edge " Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 09/26] af_unix: Link struct unix_edge when queuing skb Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 10/26] af_unix: Bulk update unix_tot_inflight/unix_inflight " Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 11/26] af_unix: Iterate all vertices by DFS Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 12/26] af_unix: Detect Strongly Connected Components Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 13/26] af_unix: Save listener for embryo socket Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 14/26] af_unix: Fix up unix_edge.successor " Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 15/26] af_unix: Save O(n) setup of Tarjan's algo Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 16/26] af_unix: Skip GC if no cycle exists Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 17/26] af_unix: Avoid Tarjan's algorithm if unnecessary Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 18/26] af_unix: Assign a unique index to SCC Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 19/26] af_unix: Detect dead SCC Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 20/26] af_unix: Replace garbage collection algorithm Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 21/26] af_unix: Remove lock dance in unix_peek_fds() Lee Jones
2025-05-21 14:45 ` Lee Jones [this message]
2025-05-21 14:45 ` [PATCH v6.6 23/26] af_unix: Don't access successor in unix_del_edges() during GC Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 24/26] af_unix: Add dead flag to struct scm_fp_list Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 25/26] af_unix: Fix garbage collection of embryos carrying OOB with SCM_RIGHTS Lee Jones
2025-05-21 14:45 ` [PATCH v6.6 26/26] af_unix: Fix uninit-value in __unix_walk_scc() Lee Jones
2025-05-29 12:26 ` [PATCH v6.6 00/26] af_unix: Align with upstream to avoid a potential UAF Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20250521144803.2050504-23-lee@kernel.org \
    --to=lee@kernel.org \
    --cc=Rao.Shoaib@oracle.com \
    --cc=asml.silence@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=kuba@kernel.org \
    --cc=kuniyu@amazon.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhal@rbox.co \
    --cc=netdev@vger.kernel.org \
    --cc=oliver.sang@intel.com \
    --cc=pabeni@redhat.com \
    --cc=sashal@kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).