Commit 99bc9f2e authored by NeilBrown's avatar NeilBrown Committed by Trond Myklebust
Browse files

NFS: add barriers when testing for NFS_FSDATA_BLOCKED 



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: default avatarRichard Kojedzinszky <richard+debian+bugreport@kojedz.in>
Closes: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1071501


Fixes: 3c59366c ("NFS: don't unhash dentry during unlink/rename")
Signed-off-by: default avatarNeilBrown <neilb@suse.de>
Signed-off-by: default avatarTrond Myklebust <trond.myklebust@hammerspace.com>
parent 33c94d7e

fs/nfs/dir.c

+32 −15
Original line number Diff line number Diff line
@@ -1803,9 +1803,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);
@@ -1818,6 +1819,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
@@ -2551,15 +2575,12 @@ int nfs_unlink(struct inode *dir, struct dentry *dentry) 
		spin_unlock(&dentry->d_lock);

		goto out;

	}

	/* 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;
@@ -2666,8 +2687,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);

}



/*
@@ -2729,11 +2749,6 @@ int nfs_rename(struct mnt_idmap *idmap, 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) {
@@ -2755,7 +2770,7 @@ int nfs_rename(struct mnt_idmap *idmap, 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;

			spin_unlock(&new_dentry->d_lock);

		}
@@ -2767,6 +2782,8 @@ int nfs_rename(struct mnt_idmap *idmap, 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;

	}