[PATCH 2/3] nfs: answer a handed-off silly-delete on a removal target with -EBUSY

[Jonathan Flynn <jonathan.flynn@hammerspace.com>]

When the last user of a silly-renamed file goes away, nfs_call_unlink()
can hand the delete it still owes to another cached entry for the same
name: it marks that entry DCACHE_NFSFS_RENAMED and stores its struct
nfs_unlinkdata in ->d_fsdata, under the entry's ->d_lock.  NeilBrown's
"nfs: fix nfs_call_unlink()" makes that hand-off reachable; before it,
the lookup for the other entry was given a name with no hash and never
found one.

The VFS refuses to remove an entry so marked, or to rename over one,
with -EBUSY, but may_delete_dentry() tests the flag without ->d_lock,
and a hand-off can land after it.  nfs_unlink() repeats the test under
->d_lock and treats a marked entry as a bug: it warns and returns
-ETXTBSY.  nfs_rename() makes the same test before it takes ->d_lock, so
a hand-off can also land between the test and block_revalidate(), which
then kfree()s the nfs_unlinkdata as if it were a device name.  Its
credential and name are leaked, DCACHE_NFSFS_RENAMED stays set with no
data behind it, and the final dput() of the target reaches
nfs_call_unlink() with a NULL pointer, which it dereferences.

Take the target's ->d_lock before the tests in nfs_rename(), as
nfs_unlink() already does, and answer a marked entry in both with
-EBUSY and no warning, as may_delete_dentry() does.  No hand-off can
land between the test and the store any more, and one that landed
before the test makes the entry the busy file the VFS already refuses.

This was found by an AI coding assistant (Claude Code) reading the
code.  The hand-off itself has been reproduced with "nfs: fix
nfs_call_unlink()" 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@hammerspace.com>
---

Notes:
    For reviewers and review tools; not part of the changelog.
    
    A hand-off that lands while nfs_rename() is in its silly-rename
    branch (the target's count above 2, with ->d_lock dropped before
    d_duplicate()) still gets -ETXTBSY: nfs_sillyrename() fails with
    -EBUSY, and that branch returns -ETXTBSY for every
    nfs_sillyrename() failure.  This patch leaves the branch as it
    was, and the handed-off data is neither freed nor leaked there,
    since nfs_async_unlink() tests and stores under ->d_lock.  An
    automated review (Claude Code running Sashiko's review prompts)
    raised this; it was checked and dropped as not introduced here.

 fs/nfs/dir.c | 32 ++++++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 36cb99f746..dc7784b666 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2605,10 +2605,18 @@ int nfs_unlink(struct inode *dir, struct dentry *dentry)
 	 * completes.  ->d_revalidate will wait for ->d_fsdata
 	 * to clear.  We set it here to ensure no lookup succeeds until
 	 * the unlink is complete on the server.
+	 *
+	 * may_delete_dentry() refused an entry carrying a silly-delete,
+	 * but without ->d_lock, and nfs_call_unlink() can have handed
+	 * this one a silly-delete since.  It is busy, as the VFS says.
 	 */
+	error = -EBUSY;
+	if (dentry->d_flags & DCACHE_NFSFS_RENAMED) {
+		spin_unlock(&dentry->d_lock);
+		goto out;
+	}
 	error = -ETXTBSY;
-	if (WARN_ON(dentry->d_flags & DCACHE_NFSFS_RENAMED) ||
-	    WARN_ON(dentry->d_fsdata == NFS_FSDATA_BLOCKED)) {
+	if (WARN_ON(dentry->d_fsdata == NFS_FSDATA_BLOCKED)) {
 		spin_unlock(&dentry->d_lock);
 		goto out;
 	}
@@ -2801,13 +2809,25 @@ int nfs_rename(const struct mnt_idmap *idmap, struct inode *old_dir,
 		 * completes.  ->d_revalidate will wait for ->d_fsdata
 		 * to clear.  We set it here to ensure no lookup succeeds until
 		 * the unlink is complete on the server.
+		 *
+		 * Test under ->d_lock, which nfs_call_unlink() hands a
+		 * silly-delete over under and block_revalidate() below
+		 * stores under, so that no hand-off lands between the test
+		 * and the store, which would kfree() it.  A target carrying
+		 * one is busy, as may_delete_dentry() says when it sees the
+		 * flag first.
 		 */
+		spin_lock(&new_dentry->d_lock);
+		error = -EBUSY;
+		if (new_dentry->d_flags & DCACHE_NFSFS_RENAMED) {
+			spin_unlock(&new_dentry->d_lock);
+			goto out;
+		}
 		error = -ETXTBSY;
-		if (WARN_ON(new_dentry->d_flags & DCACHE_NFSFS_RENAMED) ||
-		    WARN_ON(new_dentry->d_fsdata == NFS_FSDATA_BLOCKED))
+		if (WARN_ON(new_dentry->d_fsdata == NFS_FSDATA_BLOCKED)) {
+			spin_unlock(&new_dentry->d_lock);
 			goto out;
-
-		spin_lock(&new_dentry->d_lock);
+		}
 		if (d_count(new_dentry) > 2) {
 			int err;
 
-- 
2.43.0