Re: [PATCH 0/3] nfs: fix the silly-delete hand-off that 2/9 makes reachable

[NeilBrown]

On Fri, 30 Oct 2026, Jonathan Flynn wrote:
> Hi Neil,
> 
> Your "nfs: fix nfs_call_unlink()" computes the silly name's hash before
> d_alloc_parallel(), so nfs_call_unlink() can now find another cached
> entry for that name and hand it the pending silly-delete.  With it
> applied, that happens: an entry made by a fresh lookup of the silly
> name, after the original entry was invalidated, receives the delete
> when the file is closed.  Three problems in the hand-off become
> reachable with it, and these patches fix them:

Hi Jonathan,
 thanks for these.  The first 2 definitely make sense. I haven't
 completely thought through the third but it is probably good too.

 But it all seems rather ugly and I'm hoping there is a better way.
 I would like to avoid the hand-off altogether.

 There is a tiny window between the silly dentry being unhashed, and the
 ->d_iput() call which triggers the d_alloc_parallel of a new name.
 A new lookup has to happen in this window to cause a problem.
 I'd like to prevent that lookup from completing.

 I wonder if we could link all the nfs_unlinkdata for a given
 parent directory together and change nfs_lookup() to check that
 list first.  Hopefully it would mostly be empty...
 I think it would be safe to only add a nfs_unlink to the per-directory
 list in nfs_dentry_delete() if DCACHE_NFSFS_RENAMED is set.
 We then remove it once the d_alloc_parallel() completes.
 If nfs_lookup() ever finds the name it wants on the list it ... uhmmm
 maybe blocks on ->rmdir_sem.
 We would need to change the d_alloc_parallel() in nfs_call_unlink()
 to d_alloc_trylock().

 Then we wouldn't need the hand-off at all.  Once nfs_dentry_delete()
 adds the dentry to the per-directory list, the unlink must complete
 and no other dentry will be involved.

 Does that make any sense to you?  Do you think it is worth a try?

Thanks,
NeilBrown


> 
>   1/3  the hand-off can overwrite NFS_FSDATA_BLOCKED on an entry whose
>        REMOVE or RENAME is in flight (Sashiko's review of 2/9 reports
>        this too);
>   2/3  the other order: nfs_rename() tests its target before it takes
>        ->d_lock, so a hand-off landing in between is kfree()d by
>        block_revalidate(), leaving DCACHE_NFSFS_RENAMED set with no
>        data and a NULL dereference at the final dput(); a hand-off
>        that the VFS's unlocked test missed also trips the WARN_ON()s
>        in nfs_unlink() and nfs_rename().  nfs_rename() now tests under
>        ->d_lock, and both return -EBUSY, as may_delete_dentry() does;
>   3/3  nfs_call_unlink() holds rmdir_sem for read across its lookup and
>        across the dput() of the alias, and either can kill a dentry
>        carrying a silly-delete, which re-enters nfs_call_unlink() with
>        rmdir_sem still held: that deadlocks against a waiting rmdir()
>        (Sashiko's review of 2/9 reports the alias case).  It now takes
>        rmdir_sem only for the REMOVE it sends, which also closes an
>        older route to the same re-entry that does not need 2/9:
>        NFSv4's delegation return in ->unlink_setup().
> 
> They have to go wherever 2/9 goes, and its Fixes: tag will take it to
> stable.  Fold them into 2/9 if you prefer, as you did with the d_seq fix
> in 1/9.
> 
> One window is not covered, and you may prefer to close it another way.
> After nfs_unlink() or nfs_rename() clears the mark, and before the
> VFS's d_delete() or nfs_rename()'s d_move(), a hand-off can still land
> on the entry.  For an unlink, that costs only a second REMOVE, which
> finds nothing.  For a rename onto the silly name, the target is left
> owing a REMOVE of a name that now belongs to the renamed file.  That
> REMOVE is normally dropped, because nfs_call_unlink() then finds the
> renamed file's own entry and its file handle differs; but if that entry
> has left the dcache by the time the target's last reference goes, the
> REMOVE is sent and deletes the renamed file.  Closing the rename case
> would take the hand-off refusing an alias that is no longer hashed, and
> nfs_rename() dropping, under ->d_lock and after d_move(), a
> silly-delete that landed after its mark cleared.  I have a draft of
> that, and can test and send it if you want it.
> 
> Tested with your v4 and these patches on a 7.3-based kernel, over
> NFSv4.2 to a pNFS metadata server that is not knfsd.  The hand-off
> happens as described above.  With 3/3, the nested nfs_call_unlink()
> that follows it starts with rmdir_sem not held, where without 3/3 it
> ran with rmdir_sem held for read, and the REMOVE is sent once.  The
> removal race tests of my RFC, posted in reply to your 0/9, passed on
> this kernel, as did a PROVE_LOCKING and KASAN build running them,
> though lockdep does not see rmdir_sem's read hold, which is taken with
> down_read_non_owner().  No test here hits the races that 1/3 and 2/3
> close, or the deadlock itself, so for those the fixes rest on the code.
> 
> The series applies to 83f32c773255 in Linus's tree with your v4, and
> fs/ compiles without warnings at each patch.
> 
> Tools: these patches, their changelogs and this letter were written
> with Claude Code, using Anthropic's Claude Opus 5 and Claude Opus 5.5
> models, at my direction.  It found the problems fixed by 1/3 and 2/3,
> and two of the three routes in 3/3, reading the code with and without
> Sashiko's review prompts; Sashiko's own review of 2/9 reported the
> problem in 1/3 as well, and the route through the alias in 3/3.
> 
> Thanks,
> Jonathan
> 
> Jonathan Flynn (3):
>   nfs: do not transfer a sillydelete onto a blocked dentry
>   nfs: answer a handed-off silly-delete on a removal target with -EBUSY
>   nfs: take rmdir_sem for a silly-delete only when its REMOVE is sent
> 
>  fs/nfs/dir.c    | 32 ++++++++++++++++++++++++++------
>  fs/nfs/unlink.c | 39 ++++++++++++++++++++++++++++++++-------
>  2 files changed, 58 insertions(+), 13 deletions(-)
> 
> 
> base-commit: 83f32c773255720a661aa45d83e36278135f23f5
> prerequisite-patch-id: 3405b4d6ab8b0bf24ba63a898083d617deec4f24
> prerequisite-patch-id: 096657d6d80d7ca533ccb574d2af65611ede4af2
> prerequisite-patch-id: 1a2319c14739918aef86116647fea28ae85ad4df
> prerequisite-patch-id: 725cb18c3936d50141ef27dafc159bf50b64a5bd
> prerequisite-patch-id: b39cdd352b97e2cd247311fd5db620dbff227bd7
> prerequisite-patch-id: f9f3f0e945cadf96113d4ac9ab8f7b0c207af4e5
> prerequisite-patch-id: 4c09a15390bb556d79bea1ee733f52a58bec80ef
> prerequisite-patch-id: 7c026717a97b2c86a169539822f5d14650f68f8a
> prerequisite-patch-id: a702d54495d0f3d7f88b82411d884dcf5aacf9a0
> -- 
> 2.43.0
> 
>