[PATCH v4 1/9] nfs: fix open-blocking with d_fsdata

[NeilBrown]

From: NeilBrown <neil@xxxxxxxxxx>

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 will cause that check to fail if
nfs_unlink() has already seen d_count() being only 1.  So the RCU-walk
will fail with -ECHILD and it will be retried as a ref-walk.
This requires that ->d_fsdata is loaded *after* read_seqcount_begin()
loaded ->d_seq, so the smp_load_acquire() in __nfs_lookup_revalidate()
is important.

Acked-by: Anna Schumaker <anna.schumaker@xxxxxxxxxxxxxxx>
Co-developed-by: Jonathan Flynn <jonathan.flynn@xxxxxxxxxxxxxxx>
Signed-off-by: Jonathan Flynn <jonathan.flynn@xxxxxxxxxxxxxxx>
Fixes: 99bc9f2eb3f7 ("NFS: add barriers when testing for NFS_FSDATA_BLOCKED")
Signed-off-by: NeilBrown <neil@xxxxxxxxxx>
---
 fs/nfs/dir.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 354f986e60c4..cde963e2bcb8 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1835,7 +1835,11 @@ __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
 		if (dentry->d_fsdata == NFS_FSDATA_BLOCKED)
 			return -ECHILD;
 	} else {
-		/* Wait for unlink to complete - see unblock_revalidate() */
+		/* Wait for unlink to complete - see unblock_revalidate().
+		 * smp_load_acquire() ensures ->d_fsdata is read after
+		 * ->d_seq was read, matching block_revalidate() which
+		 * sets ->d_fsdata before ->d_seq.
+		 */
 		wait_var_event(&dentry->d_fsdata,
 			       smp_load_acquire(&dentry->d_fsdata)
 			       != NFS_FSDATA_BLOCKED);
@@ -1856,15 +1860,29 @@ static void block_revalidate(struct dentry *dentry)
 	/* old devname - just in case */
 	kfree(dentry->d_fsdata);
 
+	dentry->d_fsdata = NFS_FSDATA_BLOCKED;
+
 	/* 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_path() or similar
+	 *    after taking the reference.
+	 *
+	 * Holding this lock and invalidating ->d_seq *after* recording
+	 * NFS_FSDATA_BLOCKED ensures we cannot race with
+	 * __nfs_lookup_revalidate().
+	 * If __nfs_lookup_revalidate() doesn't see NFS_FSDATA_BLOCKED then:
+	 * In the ref-walk case - d_lookup() will have incremented
+	 * d_count() before we check it.
+	 * In the RCU-walk case - ->d_seq must have been sampled before
+	 * we call write_seqcount_invalidate() below (because it was sampled
+	 * before we set NFS_FSDATA_BLOCKED) so d_count() will have been
+	 * incremented or the d_seq check will fail in legitimize_path()
+	 * (or both).
 	 */
 	lockdep_assert_held(&dentry->d_lock);
+	write_seqcount_invalidate(&dentry->d_seq);
 
-	dentry->d_fsdata = NFS_FSDATA_BLOCKED;
 }
 
 static void unblock_revalidate(struct dentry *dentry)

base-commit: 864c07de0c7dcbca58b61625dfa2e461ae63b242
-- 
2.50.0.107.gf914562f5916.dirty