* Re: [GIT PULL v3] afs, rxrpc: Clean up refcounting on afs_cell and afs_server records
2025-03-03 9:49 [GIT PULL v3] afs, rxrpc: Clean up refcounting on afs_cell and afs_server records David Howells
@ 2025-03-04 9:45 ` Christian Brauner
2025-03-06 9:12 ` David Howells
1 sibling, 0 replies; 3+ messages in thread
From: Christian Brauner @ 2025-03-04 9:45 UTC (permalink / raw)
To: David Howells
Cc: Christian Brauner, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-afs, linux-fsdevel, netdev,
linux-kernel
On Mon, 03 Mar 2025 09:49:10 +0000, David Howells wrote:
> Could you pull this into the VFS tree onto a stable branch, replacing the
> earlier pull? The patches were previously posted here as part of a longer
> series:
>
> https://lore.kernel.org/r/20250224234154.2014840-1-dhowells@redhat.com/
>
> This fixes an occasional hang that's only really encountered when rmmod'ing
> the kafs module, one of the reasons why I'm proposing it for the next merge
> window rather than immediate upstreaming. The changes include:
>
> [...]
Pulled into the vfs-6.15.shared.afs branch of the vfs/vfs.git tree.
Patches in the vfs-6.15.shared.afs branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series or pull request allowing us to
drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.15.shared.afs
https://git.kernel.org/vfs/vfs/c/acf689e88306
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [GIT PULL v3] afs, rxrpc: Clean up refcounting on afs_cell and afs_server records
2025-03-03 9:49 [GIT PULL v3] afs, rxrpc: Clean up refcounting on afs_cell and afs_server records David Howells
2025-03-04 9:45 ` Christian Brauner
@ 2025-03-06 9:12 ` David Howells
1 sibling, 0 replies; 3+ messages in thread
From: David Howells @ 2025-03-06 9:12 UTC (permalink / raw)
To: Christian Brauner, Alexander Viro
Cc: dhowells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, linux-afs, linux-fsdevel, netdev,
linux-kernel
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@zeniv.linux.org.uk>
Signed-off-by: David Howells <dhowells@redhat.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, ".@cell", 6, 3, DT_LNK))
+ if (rcu_access_pointer(net->ws_cell) &&
+ !dir_emit(ctx, ".@cell", 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);
^ permalink raw reply [flat|nested] 3+ messages in thread