[PATCH] nfs: set NFS_FSDATA_BLOCKED before invalidating d_seq

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

block_revalidate() calls write_seqcount_invalidate(&dentry->d_seq) and
only then sets dentry->d_fsdata to NFS_FSDATA_BLOCKED.  Because ->d_seq
is invalidated before NFS_FSDATA_BLOCKED is set, an RCU-walk open can
get past an unlink that is in progress.

The barrier in write_seqcount_invalidate() is issued before the
increment, so it orders only the stores made before the call.  An
RCU-walk lookup that samples ->d_seq after the increment can still read
the old ->d_fsdata, and so ->d_revalidate() accepts the dentry.  The
walker then waits for ->d_lock in lockref_get_not_dead(), takes its
reference, and passes the ->d_seq recheck in __legitimize_path(),
because ->d_seq still holds the value the walker sampled.  By then
nfs_unlink() has seen d_count() == 1 and sends the REMOVE instead of
silly-renaming, so the open proceeds on a file the server is removing.

Set NFS_FSDATA_BLOCKED first, then call write_seqcount_invalidate().  A
walker that samples the new ->d_seq then also sees NFS_FSDATA_BLOCKED
and returns -ECHILD.  A walker that sampled the old ->d_seq fails the
recheck and retries in ref-walk, which waits for the unlink to finish.
__d_drop() follows the same order: unhash the dentry, then invalidate
->d_seq.

Also correct the comment in block_revalidate(): the ->d_seq recheck is
in __legitimize_path() and try_to_unlazy_next(), not in
legitimize_mnt().

Signed-off-by: Jonathan Flynn <jonathan.flynn@hammerspace.com>
---
Neil, this answers Sashiko's second point on 1/9: v3 still calls
write_seqcount_invalidate() before it sets NFS_FSDATA_BLOCKED.  This
patch applies on top of v3; fold it into 1/9 if you prefer.

The window with the v3 order:

  CPU0: nfs_unlink(), ->d_lock held     CPU1: RCU-walk open, same name
  d_count() == 1, no silly-rename
  write_seqcount_invalidate(&d_seq)
                                        __d_lookup_rcu() samples the
                                          new ->d_seq
                                        ->d_revalidate() reads ->d_fsdata,
                                          not yet NFS_FSDATA_BLOCKED
  d_fsdata = NFS_FSDATA_BLOCKED
  spin_unlock(&dentry->d_lock)
  REMOVE sent                           lockref_get_not_dead() takes a ref
                                        read_seqcount_retry() passes
                                        the open proceeds

Your afs 1/5 also sets DCACHE_BLOCKED after write_seqcount_invalidate().

 fs/nfs/dir.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 53cd3adf3ffe..79331e0fa044 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1879,15 +1879,20 @@ static void block_revalidate(struct dentry *dentry)
 	/* Any new reference that could lead to an open
 	 * will either:
 	 *  - take ->d_lock in lookup_open() -> d_lookup() or
-	 *  - will check d_seq in legitimize_mnt()
+	 *  - will check d_seq in __legitimize_path() or try_to_unlazy_next()
 	 *
 	 * 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;
+
+	/*
+	 * Set NFS_FSDATA_BLOCKED before invalidating ->d_seq: the barrier in
+	 * write_seqcount_invalidate() orders only earlier stores, so an
+	 * RCU-walk that samples the new ->d_seq also sees NFS_FSDATA_BLOCKED.
+	 */
+	write_seqcount_invalidate(&dentry->d_seq);
 }
 
 static void unblock_revalidate(struct dentry *dentry)
-- 
2.43.0