From: NeilBrown The logic described in block_revalidate() that ->d_lock must be taken in lookup_open() missed the fact that sometimes lookup_open() isn't called. If open_last_lookups() is called while still in RCU-walk and lookup_fast() finds a dentry, it does not take ->d_lock or increment d_count() before calling d_revalidate() - only afterwards. So on that path d_count() can be incremented and the file opened *after* nfs_unlink() has checked d_count() and set NFS_FSDATA_BLOCKED. So an application could have the file open when it is unlinked on the server. To fix this we need block_revalidate() to ensure the RCU-walk path is blocked as well. This is easily done with write_seqcount_invalidate(&dentry->d_seq); legitimize_path() will increment d_count() and then check d_seq. The write_seqcount_invalidate() call with cause that check to fail if nfs_unlink() has already seen d_count() being 1. So the RCU-walk will fail with -ECHILD and it will be retried as a ref-walk. Also the smp_load_acquire() in __nfs_lookup_revalidate() is pointless. wait_var_event() contains the required barriers (a spinlock in prepare_to_wait_event()) to ensure the correct value is read after any wakeup. That barrier doesn't apply for the very first test of the condition in wait_var_event) but the locking of ->d_lock described above provides sufficient barriers. Fixes: 99bc9f2eb3f7 ("NFS: add barriers when testing for NFS_FSDATA_BLOCKED") Signed-off-by: NeilBrown --- fs/nfs/dir.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c index 49394123bd09..4f1bf45b0c70 100644 --- a/fs/nfs/dir.c +++ b/fs/nfs/dir.c @@ -1837,8 +1837,7 @@ __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags) } else { /* Wait for unlink to complete - see unblock_revalidate() */ wait_var_event(&dentry->d_fsdata, - smp_load_acquire(&dentry->d_fsdata) - != NFS_FSDATA_BLOCKED); + dentry->d_fsdata != NFS_FSDATA_BLOCKED); } return 0; } @@ -1857,12 +1856,15 @@ static void block_revalidate(struct dentry *dentry) kfree(dentry->d_fsdata); /* Any new reference that could lead to an open - * will take ->d_lock in lookup_open() -> d_lookup(). - * Holding this lock ensures we cannot race with - * __nfs_lookup_revalidate() and removes and need - * for further barriers. + * will either: + * - take ->d_lock in lookup_open() -> d_lookup() or + * - will check d_seq in legitimize_mnt() + * + * Holding this lock and invalidating ->d_seq ensures we cannot + * race with __nfs_lookup_revalidate(). */ lockdep_assert_held(&dentry->d_lock); + write_seqcount_invalidate(&dentry->d_seq); dentry->d_fsdata = NFS_FSDATA_BLOCKED; } base-commit: 3879f51857325da9bf3cfb073280257cd16ae067 -- 2.50.0.107.gf914562f5916.dirty