linux-kernel - Re: [GIT PULL v3] afs, rxrpc: Clean up refcounting on afs_cell and afs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <66441.1741252359@warthog.procyon.org.uk>
Date: Thu, 06 Mar 2025 09:12:39 +0000
From: David Howells <dhowells@...hat.com>
To: Christian Brauner <brauner@...nel.org>,
    Alexander Viro <viro@...iv.linux.org.uk>
Cc: dhowells@...hat.com, Marc Dionne <marc.dionne@...istor.com>,
    Jakub Kicinski <kuba@...nel.org>,
    "David S.
 Miller" <davem@...emloft.net>,
    Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
    linux-afs@...ts.infradead.org, linux-fsdevel@...ts.infradead.org,
    netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [GIT PULL v3] afs, rxrpc: Clean up refcounting on afs_cell and afs_server records

Al spotted another bug (fix below).  Subject to his okaying of the patch, I'll
fold it down and ask for a repull.

David
---
afs: Fix afs_atcell_get_link() to handle RCU pathwalk

The ->get_link() method may be entered under RCU pathwalk conditions (in
which case, the dentry pointer is NULL).  This is not taken account of by
afs_atcell_get_link() and lockdep will complain when it tries to lock an
rwsem.

Fix this by marking net->ws_cell as __rcu and using RCU access macros on it
and by making afs_atcell_get_link() just return a pointer to the name in
RCU pathwalk without taking net->cells_lock or a ref on the cell as RCU
will protect the name storage (the cell is already freed via call_rcu()).

Reported-by: Alexander Viro <viro@...iv.linux.org.uk>
Signed-off-by: David Howells <dhowells@...hat.com>
---
 fs/afs/cell.c     |   11 ++++++-----
 fs/afs/dynroot.c  |   21 +++++++++++++++++----
 fs/afs/internal.h |    2 +-
 fs/afs/proc.c     |    2 +-
 4 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/fs/afs/cell.c b/fs/afs/cell.c
index 4ca713d3862c..0168bbf53fe0 100644
--- a/fs/afs/cell.c
+++ b/fs/afs/cell.c
@@ -57,7 +57,8 @@ static struct afs_cell *afs_find_cell_locked(struct afs_net *net,
 		return ERR_PTR(-ENAMETOOLONG);
 
 	if (!name) {
-		cell = net->ws_cell;
+		cell = rcu_dereference_protected(net->ws_cell,
+						 lockdep_is_held(&net->cells_lock));
 		if (!cell)
 			return ERR_PTR(-EDESTADDRREQ);
 		goto found;
@@ -394,8 +395,8 @@ int afs_cell_init(struct afs_net *net, const char *rootcell)
 
 	/* install the new cell */
 	down_write(&net->cells_lock);
-	old_root = net->ws_cell;
-	net->ws_cell = new_root;
+	old_root = rcu_replace_pointer(net->ws_cell, new_root,
+				       lockdep_is_held(&net->cells_lock));
 	up_write(&net->cells_lock);
 
 	afs_unuse_cell(old_root, afs_cell_trace_unuse_ws);
@@ -869,8 +870,8 @@ void afs_cell_purge(struct afs_net *net)
 	_enter("");
 
 	down_write(&net->cells_lock);
-	ws = net->ws_cell;
-	net->ws_cell = NULL;
+	ws = rcu_replace_pointer(net->ws_cell, NULL,
+				 lockdep_is_held(&net->cells_lock));
 	up_write(&net->cells_lock);
 	afs_unuse_cell(ws, afs_cell_trace_unuse_ws);
 
diff --git a/fs/afs/dynroot.c b/fs/afs/dynroot.c
index 0b66865e3535..9732a1e17db3 100644
--- a/fs/afs/dynroot.c
+++ b/fs/afs/dynroot.c
@@ -210,12 +210,23 @@ static const char *afs_atcell_get_link(struct dentry *dentry, struct inode *inod
 	const char *name;
 	bool dotted = vnode->fid.vnode == 3;
 
-	if (!net->ws_cell)
+	if (!dentry) {
+		/* We're in RCU-pathwalk. */
+		cell = rcu_dereference(net->ws_cell);
+		if (dotted)
+			name = cell->name - 1;
+		else
+			name = cell->name;
+		/* Shouldn't need to set a delayed call. */
+		return name;
+	}
+
+	if (!rcu_access_pointer(net->ws_cell))
 		return ERR_PTR(-ENOENT);
 
 	down_read(&net->cells_lock);
 
-	cell = net->ws_cell;
+	cell = rcu_dereference_protected(net->ws_cell, lockdep_is_held(&net->cells_lock));
 	if (dotted)
 		name = cell->name - 1;
 	else
@@ -324,12 +335,14 @@ static int afs_dynroot_readdir(struct file *file, struct dir_context *ctx)
 		return 0;
 
 	if (ctx->pos == 2) {
-		if (net->ws_cell && !dir_emit(ctx, "@cell", 5, 2, DT_LNK))
+		if (rcu_access_pointer(net->ws_cell) &&
+		    !dir_emit(ctx, "@cell", 5, 2, DT_LNK))
 			return 0;
 		ctx->pos = 3;
 	}
 	if (ctx->pos == 3) {
-		if (net->ws_cell && !dir_emit(ctx, ".@...l", 6, 3, DT_LNK))
+		if (rcu_access_pointer(net->ws_cell) &&
+		    !dir_emit(ctx, ".@...l", 6, 3, DT_LNK))
 			return 0;
 		ctx->pos = 4;
 	}
diff --git a/fs/afs/internal.h b/fs/afs/internal.h
index addce2f03562..440b0e731093 100644
--- a/fs/afs/internal.h
+++ b/fs/afs/internal.h
@@ -288,7 +288,7 @@ struct afs_net {
 	/* Cell database */
 	struct rb_root		cells;
 	struct idr		cells_dyn_ino;	/* cell->dynroot_ino mapping */
-	struct afs_cell		*ws_cell;
+	struct afs_cell __rcu	*ws_cell;
 	atomic_t		cells_outstanding;
 	struct rw_semaphore	cells_lock;
 	struct mutex		cells_alias_lock;
diff --git a/fs/afs/proc.c b/fs/afs/proc.c
index 0af94c846504..9d4df9e4562b 100644
--- a/fs/afs/proc.c
+++ b/fs/afs/proc.c
@@ -207,7 +207,7 @@ static int afs_proc_rootcell_show(struct seq_file *m, void *v)
 
 	net = afs_seq2net_single(m);
 	down_read(&net->cells_lock);
-	cell = net->ws_cell;
+	cell = rcu_dereference_protected(net->ws_cell, lockdep_is_held(&net->cells_lock));
 	if (cell)
 		seq_printf(m, "%s\n", cell->name);
 	up_read(&net->cells_lock);