[PATCH 1/3] nfs: do not transfer a sillydelete onto a blocked dentry

[Jonathan Flynn]

Harden the hand-off of a pending silly-delete.  When the last user of a
silly-renamed file goes away, the client can move the delete it still
owes onto another cached entry for the same name.  Leave that entry alone
if a removal of the name, or a rename onto it, is already in flight on
it.  Moving the delete there would overwrite the mark that makes lookups
of the name wait for that removal: the wait would end early, the removal
could report success without removing anything, the pending delete would
be leaked, and a later release of the entry would dereference a NULL
pointer.  If that removal succeeds, the silly-delete was redundant; if
it fails, the silly-renamed file stays on the server, which is the cost
of skipping rather than waiting.

The mark and the hand-off have shared ->d_fsdata since the commit named
below, which reasoned that no silly-delete is pending on an entry being
removed or renamed over, but not that a hand-off could arrive while that
removal is in flight.  The hand-off has stayed unreachable since then
only because nfs_call_unlink() passed d_alloc_parallel() a name whose
hash it had not computed, so the lookup for the other entry never found
it.  NeilBrown's "nfs: fix nfs_call_unlink()" computes that hash, which
makes the hand-off reachable even though every removal holds the parent
directory exclusive: the hand-off does not take that lock.  That fix
adds no test of the mark; this is the test, and a tree that takes that
fix needs this one too.  It does not depend on any other patch.

This was found by an AI coding assistant (Claude Code) reading the
code; Sashiko's review of NeilBrown's patch reports it too.  The
hand-off itself has been reproduced with that patch applied; its race
with a removal or rename has not.

Fixes: 3c59366c207e ("NFS: don't unhash dentry during unlink/rename")
Signed-off-by: Jonathan Flynn <jonathan.flynn@xxxxxxxxxxxxxxx>
---

Notes:
    For reviewers and review tools; not part of the changelog.
    
    Known and not fixed by this series: a hand-off that lands after
    nfs_unlink() or nfs_rename() clears the mark, and before the VFS's
    d_delete() or nfs_rename()'s d_move().  The cover letter describes
    it, with what it costs and a fix offered.

 fs/nfs/unlink.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index 0eddc7ee11..491ebd8582 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -142,9 +142,26 @@ static int nfs_call_unlink(struct dentry *dentry, struct inode *inode, struct nf
 		 * the sillyrename information to the aliased dentry.
 		 */
 		spin_lock(&alias->d_lock);
+		/*
+		 * Not if a removal of this name already owns the alias:
+		 * ->d_fsdata == NFS_FSDATA_BLOCKED means nfs_unlink() has a
+		 * REMOVE of exactly this name, or nfs_rename() a RENAME onto
+		 * it, in flight and ->d_revalidate() is waiting on that field,
+		 * so storing over it would lift the block early and the later
+		 * unblock_revalidate() would store NULL over @data.  Tested
+		 * under the same ->d_lock block_revalidate() stores under.
+		 * Skip rather than wait: that removal makes this sillydelete
+		 * redundant if it succeeds, and a wait here would sleep in a
+		 * dput() path -- the nfsiod callback that releases a
+		 * silly-rename among its callers -- for the length of another
+		 * task's RPC, with ->d_lock dropped and these tests redone.
+		 * If that removal then fails, the silly-renamed file stays on
+		 * the server: the price of skipping.
+		 */
 		if (d_really_is_positive(alias) &&
 		    !nfs_compare_fh(NFS_FH(inode), NFS_FH(d_inode(alias))) &&
-		    !(alias->d_flags & DCACHE_NFSFS_RENAMED)) {
+		    !(alias->d_flags & DCACHE_NFSFS_RENAMED) &&
+		    alias->d_fsdata != NFS_FSDATA_BLOCKED) {
 			devname_garbage = alias->d_fsdata;
 			alias->d_fsdata = data;
 			alias->d_flags |= DCACHE_NFSFS_RENAMED;
-- 
2.43.0