NFS: add barriers when testing for NFS_FSDATA_BLOCKED

commit 99bc9f2eb3f79a2b4296d9bf43153e1d10ca50d3 upstream.

dentry->d_fsdata is set to NFS_FSDATA_BLOCKED while unlinking or
renaming-over a file to ensure that no open succeeds while the NFS
operation progressed on the server.

Setting dentry->d_fsdata to NFS_FSDATA_BLOCKED is done under ->d_lock
after checking the refcount is not elevated.  Any attempt to open the
file (through that name) will go through lookp_open() which will take
->d_lock while incrementing the refcount, we can be sure that once the
new value is set, __nfs_lookup_revalidate() *will* see the new value and
will block.

We don't have any locking guarantee that when we set ->d_fsdata to NULL,
the wait_var_event() in __nfs_lookup_revalidate() will notice.
wait/wake primitives do NOT provide barriers to guarantee order.  We
must use smp_load_acquire() in wait_var_event() to ensure we look at an
up-to-date value, and must use smp_store_release() before wake_up_var().

This patch adds those barrier functions and factors out
block_revalidate() and unblock_revalidate() far clarity.

There is also a hypothetical bug in that if memory allocation fails
(which never happens in practice) we might leave ->d_fsdata locked.
This patch adds the missing call to unblock_revalidate().

Reported-and-tested-by: Richard Kojedzinszky <richard+debian+bugreport@kojedz.in>
Closes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1071501
Fixes: 3c59366c207e ("NFS: don't unhash dentry during unlink/rename")
Signed-off-by: NeilBrown <neilb@suse.de>
Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index 7d5f82c2825631e1fb89efb7ee1255dac45f39de..fec395c0defc78a25787dfafe8cdcf1ddc9e60b9 100644 (file)
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -1636,9 +1636,10 @@ __nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags,
                if (parent != READ_ONCE(dentry->d_parent))
                        return -ECHILD;
        } else {
-               /* Wait for unlink to complete */
+               /* Wait for unlink to complete - see unblock_revalidate() */
                wait_var_event(&dentry->d_fsdata,
-                              dentry->d_fsdata != NFS_FSDATA_BLOCKED);
+                              smp_load_acquire(&dentry->d_fsdata)
+                              != NFS_FSDATA_BLOCKED);
                parent = dget_parent(dentry);
                ret = reval(d_inode(parent), dentry, flags);
                dput(parent);
@@ -1651,6 +1652,29 @@ static int nfs_lookup_revalidate(struct dentry *dentry, unsigned int flags)
        return __nfs_lookup_revalidate(dentry, flags, nfs_do_lookup_revalidate);
 }
 
+static void block_revalidate(struct dentry *dentry)
+{
+       /* old devname - just in case */
+       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.
+        */
+       lockdep_assert_held(&dentry->d_lock);
+
+       dentry->d_fsdata = NFS_FSDATA_BLOCKED;
+}
+
+static void unblock_revalidate(struct dentry *dentry)
+{
+       /* store_release ensures wait_var_event() sees the update */
+       smp_store_release(&dentry->d_fsdata, NULL);
+       wake_up_var(&dentry->d_fsdata);
+}
+
 /*
  * A weaker form of d_revalidate for revalidating just the d_inode(dentry)
  * when we don't really care about the dentry name. This is called when a
@@ -2336,16 +2360,12 @@ int nfs_unlink(struct inode *dir, struct dentry *dentry)
                spin_unlock(&dentry->d_lock);
                goto out;
        }
-       if (dentry->d_fsdata)
-               /* old devname */
-               kfree(dentry->d_fsdata);
-       dentry->d_fsdata = NFS_FSDATA_BLOCKED;
+       block_revalidate(dentry);
 
        spin_unlock(&dentry->d_lock);
        error = nfs_safe_remove(dentry);
        nfs_dentry_remove_handle_error(dir, dentry, error);
-       dentry->d_fsdata = NULL;
-       wake_up_var(&dentry->d_fsdata);
+       unblock_revalidate(dentry);
 out:
        trace_nfs_unlink_exit(dir, dentry, error);
        return error;
@@ -2454,8 +2474,7 @@ nfs_unblock_rename(struct rpc_task *task, struct nfs_renamedata *data)
 {
        struct dentry *new_dentry = data->new_dentry;
 
-       new_dentry->d_fsdata = NULL;
-       wake_up_var(&new_dentry->d_fsdata);
+       unblock_revalidate(new_dentry);
 }
 
 /*
@@ -2518,11 +2537,6 @@ int nfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
                if (WARN_ON(new_dentry->d_flags & DCACHE_NFSFS_RENAMED) ||
                    WARN_ON(new_dentry->d_fsdata == NFS_FSDATA_BLOCKED))
                        goto out;
-               if (new_dentry->d_fsdata) {
-                       /* old devname */
-                       kfree(new_dentry->d_fsdata);
-                       new_dentry->d_fsdata = NULL;
-               }
 
                spin_lock(&new_dentry->d_lock);
                if (d_count(new_dentry) > 2) {
@@ -2544,7 +2558,7 @@ int nfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
                        new_dentry = dentry;
                        new_inode = NULL;
                } else {
-                       new_dentry->d_fsdata = NFS_FSDATA_BLOCKED;
+                       block_revalidate(new_dentry);
                        must_unblock = true;
                        new_gencount = NFS_I(new_inode)->attr_gencount;
                        spin_unlock(&new_dentry->d_lock);
@@ -2557,6 +2571,8 @@ int nfs_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
        task = nfs_async_rename(old_dir, new_dir, old_dentry, new_dentry,
                                must_unblock ? nfs_unblock_rename : NULL);
        if (IS_ERR(task)) {
+               if (must_unblock)
+                       unblock_revalidate(new_dentry);
                error = PTR_ERR(task);
                goto out;
        }