[PATCH 0/5 RFC] VFS: introduce new APIs to be used for directory locking

[PATCH 0/5 RFC] VFS: introduce new APIs to be used for directory locking

[NeilBrown]

The following 5 patches, which may depend on some of the earlier patches
I have sent, introduce new interfaces for requesting name-based
operations on directories (create, remove, rename).  They generally
combine the lookup and the lock operations and return a dentry which is
"locked".

Currently a dentry is always locked by locking the parent directory.
Once all clients are converted to use these interfaces we will be free to
change the details of the locking.  My proposal is to lock just the
relevant dentrys in a manner somewhat similar to the way that
d_in_lookup() dentrys are currently locked.

After the intended operation is completed (or aborted) the dentry is
unlocked with dentry_unlock().  This currently unlocks the parent
directory and dputs the dentry.  It is because the unlock is given just
the dentry, that one of my earlier patches changed vfs_mkdir() to drop
the lock when it consumes the dentry without replacing it (i.e.  in case
of error).  In that case there is nothing to pass to dentry_unlock().

I have a follow-on set of patches which change various parts of the
kernel to use these APIs instead of directly locking the parent.
They cover smb/server, nfsd, cachefiles, debugfs, binderfs, binfmt_misc,
kernel/bpf, devpts, fuse, infiniband.../qib_fs, ipc/mqueue, fs/proc,
s390/hypfs, security, sunrpc/rpc_pipe, xfs, tracefs.  overlayfs needs
some rearrangement of locking before it can be converted - that is on my
short-list of tasks.  I don't plan on a full submission of these APIs
until that is completed, in case I find a need for changes.


Thanks,
NeilBrown


 [PATCH 1/5] VFS: introduce lookup_and_lock() and friends
 [PATCH 2/5] VFS/btrfs: add lookup_and_lock_killable()
 [PATCH 3/5] VFS: change old_dir and new_dir in struct renamedata to
 [PATCH 4/5] VFS: add lookup_and_lock_rename()
 [PATCH 5/5] VFS: introduce lock_and_check_dentry()

[PATCH 1/5] VFS: introduce lookup_and_lock() and friends

[NeilBrown]

lookup_and_lock() combines locking the directory and performing a lookup
prior to a change to the directory.
Abstracting this prepares for changing the locking requirements.

lookup_and_lock_noperm() does the same without needing a mnt_idmap and
without checking permissions.  This is useful for internal filesystem
management (e.g.  creating virtual files in response to events) and in
other cases similar to lookup_noperm().

lookup_and_lock_hashed() also does no permissions checking and assumes
that the hash of the name has already been stored in the qstr.  This is
useful following filename_parentat().

For "silly_rename" we will need to lookup_and_lock() in a directory that
is already locked.  For this purpose we add lookup_and_lock_noperm_locked()
and dentry_unlock_dir_locked().  It is planned for these function to
be removed once locking is performed on the dentry only.

dentry_unlock() provides the inverse of putting the dentry and
unlocking.

Like lookup_one_qstr_excl(), lookup_and_lock() returns -ENOENT if
LOOKUP_CREATE was NOT given and the name cannot be found,, and returns
-EEXIST if LOOKUP_EXCL WAS given and the name CAN be found.

These functions replace all uses of lookup_one_qstr_excl() in namei.c
except for those used for rename.

The name might seem backwards as the lock happens before the lookup.
A future patch will change this so that only a shared lock is taken
before the lookup, and an exclusive lock on the dentry is taken after a
successful lookup.  So the order "lookup" then "lock" will make sense.

Some of the variants should possibly be inlines in a header.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c            | 186 ++++++++++++++++++++++++++++++++----------
 include/linux/namei.h |  11 +++
 2 files changed, 156 insertions(+), 41 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index cefbb681d2f5..5e8fe2d78486 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1714,6 +1714,130 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
 }
 EXPORT_SYMBOL(lookup_one_qstr_excl);
 
+/**
+ * lookup_and_lock_hashed - lookup and lock a name prior to dir ops
+ * @last: the name in the given directory
+ * @base: the directory in which the name is to be found
+ * @lookup_flags: %LOOKUP_xxx flags
+ *
+ * The name is looked up and necessary locks are taken so that
+ * the name can be created or removed.
+ * The "necessary locks" are currently the inode node lock on @base.
+ * The name @last is expected to already have the hash calculated.
+ * No permission checks are performed.
+ * Returns: the dentry, suitably locked, or an ERR_PTR().
+ */
+struct dentry *lookup_and_lock_hashed(struct qstr *last,
+				      struct dentry *base,
+				      unsigned int lookup_flags)
+{
+	struct dentry *dentry;
+
+	inode_lock_nested(base->d_inode, I_MUTEX_PARENT);
+
+	dentry = lookup_one_qstr_excl(last, base, lookup_flags);
+	if (IS_ERR(dentry))
+		inode_unlock(base->d_inode);
+	return dentry;
+}
+EXPORT_SYMBOL(lookup_and_lock_hashed);
+
+static int lookup_noperm_common(struct qstr *qname, struct dentry *base);
+static int lookup_one_common(struct mnt_idmap *idmap,
+			     struct qstr *qname, struct dentry *base);
+struct dentry *lookup_and_lock_noperm_locked(struct qstr *last,
+					     struct dentry *base,
+					     unsigned int lookup_flags)
+{
+	int err;
+
+	err = lookup_noperm_common(last, base);
+	if (err < 0)
+		return ERR_PTR(err);
+
+	return lookup_one_qstr_excl(last, base, lookup_flags);
+}
+EXPORT_SYMBOL(lookup_and_lock_noperm_locked);
+
+/**
+ * lookup_and_lock_noperm - lookup and lock a name prior to dir ops
+ * @last: the name in the given directory
+ * @base: the directory in which the name is to be found
+ * @lookup_flags: %LOOKUP_xxx flags
+ *
+ * The name is looked up and necessary locks are taken so that
+ * the name can be created or removed.
+ * The "necessary locks" are currently the inode node lock on @base.
+ * The name @last is NOT expected to have the hash calculated.
+ * No permission checks are performed.
+ * Returns: the dentry, suitably locked, or an ERR_PTR().
+ */
+struct dentry *lookup_and_lock_noperm(struct qstr *last,
+				      struct dentry *base,
+				      unsigned int lookup_flags)
+{
+	struct dentry *dentry;
+
+	inode_lock_nested(base->d_inode, I_MUTEX_PARENT);
+
+	dentry = lookup_and_lock_noperm_locked(last, base, lookup_flags);
+	if (IS_ERR(dentry))
+		inode_unlock(base->d_inode);
+	return dentry;
+}
+EXPORT_SYMBOL(lookup_and_lock_noperm);
+
+/**
+ * lookup_and_lock - lookup and lock a name prior to dir ops
+ * @last: the name in the given directory
+ * @base: the directory in which the name is to be found
+ * @lookup_flags: %LOOKUP_xxx flags
+ *
+ * The name is looked up and necessary locks are taken so that
+ * the name can be created or removed.
+ * The "necessary locks" are currently the inode node lock on @base.
+ * The name @last is NOT expected to already have the hash calculated.
+ * Permission checks are performed to ensure %MAY_EXEC access to @base.
+ * Returns: the dentry, suitably locked, or an ERR_PTR().
+ */
+struct dentry *lookup_and_lock(struct mnt_idmap *idmap,
+			       struct qstr *last,
+			       struct dentry *base,
+			       unsigned int lookup_flags)
+{
+	int err;
+
+	err = lookup_one_common(idmap, last, base);
+	if (err < 0)
+		return ERR_PTR(err);
+	return lookup_and_lock_hashed(last, base, lookup_flags);
+}
+EXPORT_SYMBOL(lookup_and_lock);
+
+void dentry_unlock_dir_locked(struct dentry *dentry)
+{
+	d_lookup_done(dentry);
+	dput(dentry);
+}
+EXPORT_SYMBOL(dentry_unlock_dir_locked);
+
+/**
+ * dentry_unlock - unlock a dentry retured by lookup_and_lock()
+ * @dentry - the target dentry
+ *
+ * Reverse the effects of lookup_and_lock() or similar.  If the
+ * @dentry is not an error, the lock and the reference to the dentry
+ * are dropped.
+ */
+void dentry_unlock(struct dentry *dentry)
+{
+	if (!IS_ERR(dentry)) {
+		inode_unlock(dentry->d_parent->d_inode);
+		dentry_unlock_dir_locked(dentry);
+	}
+}
+EXPORT_SYMBOL(dentry_unlock);
+
 /**
  * lookup_fast - do fast lockless (but racy) lookup of a dentry
  * @nd: current nameidata
@@ -2756,12 +2880,9 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct
 		return ERR_PTR(error);
 	if (unlikely(type != LAST_NORM))
 		return ERR_PTR(-EINVAL);
-	inode_lock_nested(parent_path.dentry->d_inode, I_MUTEX_PARENT);
-	d = lookup_one_qstr_excl(&last, parent_path.dentry, 0);
-	if (IS_ERR(d)) {
-		inode_unlock(parent_path.dentry->d_inode);
+	d = lookup_and_lock_hashed(&last, parent_path.dentry, 0);
+	if (IS_ERR(d))
 		return d;
-	}
 	path->dentry = no_free_ptr(parent_path.dentry);
 	path->mnt = no_free_ptr(parent_path.mnt);
 	return d;
@@ -2780,12 +2901,9 @@ struct dentry *kern_path_locked_negative(const char *name, struct path *path)
 		return ERR_PTR(error);
 	if (unlikely(type != LAST_NORM))
 		return ERR_PTR(-EINVAL);
-	inode_lock_nested(parent_path.dentry->d_inode, I_MUTEX_PARENT);
-	d = lookup_one_qstr_excl(&last, parent_path.dentry, LOOKUP_CREATE);
-	if (IS_ERR(d)) {
-		inode_unlock(parent_path.dentry->d_inode);
+	d = lookup_and_lock_hashed(&last, parent_path.dentry, LOOKUP_CREATE);
+	if (IS_ERR(d))
 		return d;
-	}
 	path->dentry = no_free_ptr(parent_path.dentry);
 	path->mnt = no_free_ptr(parent_path.mnt);
 	return d;
@@ -4105,7 +4223,6 @@ static struct dentry *filename_create(int dfd, struct filename *name,
 	unsigned int reval_flag = lookup_flags & LOOKUP_REVAL;
 	unsigned int create_flags = LOOKUP_CREATE | LOOKUP_EXCL;
 	int type;
-	int err2;
 	int error;
 
 	error = filename_parentat(dfd, name, reval_flag, path, &last, &type);
@@ -4117,35 +4234,30 @@ static struct dentry *filename_create(int dfd, struct filename *name,
 	 * (foo/., foo/.., /////)
 	 */
 	if (unlikely(type != LAST_NORM))
-		goto out;
+		goto put;
 
 	/* don't fail immediately if it's r/o, at least try to report other errors */
-	err2 = mnt_want_write(path->mnt);
+	error = mnt_want_write(path->mnt);
 	/*
 	 * Do the final lookup.  Suppress 'create' if there is a trailing
 	 * '/', and a directory wasn't requested.
 	 */
 	if (last.name[last.len] && !want_dir)
 		create_flags &= ~LOOKUP_CREATE;
-	inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
-	dentry = lookup_one_qstr_excl(&last, path->dentry,
-				      reval_flag | create_flags);
+	dentry = lookup_and_lock_hashed(&last, path->dentry, reval_flag | create_flags);
 	if (IS_ERR(dentry))
-		goto unlock;
+		goto drop;
 
-	if (unlikely(err2)) {
-		error = err2;
+	if (unlikely(error))
 		goto fail;
-	}
 	return dentry;
 fail:
-	dput(dentry);
+	dentry_unlock(dentry);
 	dentry = ERR_PTR(error);
-unlock:
-	inode_unlock(path->dentry->d_inode);
-	if (!err2)
+drop:
+	if (!error)
 		mnt_drop_write(path->mnt);
-out:
+put:
 	path_put(path);
 	return dentry;
 }
@@ -4163,16 +4275,13 @@ EXPORT_SYMBOL(kern_path_create);
 
 void done_path_create(struct path *path, struct dentry *dentry)
 {
-	if (!IS_ERR(dentry)) {
-		dput(dentry);
-		inode_unlock(path->dentry->d_inode);
-	}
+	dentry_unlock(dentry);
 	mnt_drop_write(path->mnt);
 	path_put(path);
 }
 EXPORT_SYMBOL(done_path_create);
 
-inline struct dentry *user_path_create(int dfd, const char __user *pathname,
+struct dentry *user_path_create(int dfd, const char __user *pathname,
 				struct path *path, unsigned int lookup_flags)
 {
 	struct filename *filename = getname(pathname);
@@ -4369,8 +4478,7 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 
 err:
 	/* Caller only needs to unlock if dentry is not an error */
-	inode_unlock(dir);
-	dput(dentry);
+	dentry_unlock(dentry);
 	return ERR_PTR(error);
 }
 EXPORT_SYMBOL(vfs_mkdir);
@@ -4500,19 +4608,18 @@ int do_rmdir(int dfd, struct filename *name)
 	if (error)
 		goto exit2;
 
-	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
-	dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
+	dentry = lookup_and_lock_hashed(&last, path.dentry, lookup_flags);
 	error = PTR_ERR(dentry);
 	if (IS_ERR(dentry))
 		goto exit3;
+
 	error = security_path_rmdir(&path, dentry);
 	if (error)
 		goto exit4;
 	error = vfs_rmdir(mnt_idmap(path.mnt), path.dentry->d_inode, dentry);
 exit4:
-	dput(dentry);
+	dentry_unlock(dentry);
 exit3:
-	inode_unlock(path.dentry->d_inode);
 	mnt_drop_write(path.mnt);
 exit2:
 	path_put(&path);
@@ -4629,11 +4736,9 @@ int do_unlinkat(int dfd, struct filename *name)
 	if (error)
 		goto exit2;
 retry_deleg:
-	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
-	dentry = lookup_one_qstr_excl(&last, path.dentry, lookup_flags);
+	dentry = lookup_and_lock_hashed(&last, path.dentry, lookup_flags);
 	error = PTR_ERR(dentry);
 	if (!IS_ERR(dentry)) {
-
 		/* Why not before? Because we want correct error value */
 		if (last.name[last.len])
 			goto slashes;
@@ -4645,9 +4750,8 @@ int do_unlinkat(int dfd, struct filename *name)
 		error = vfs_unlink(mnt_idmap(path.mnt), path.dentry->d_inode,
 				   dentry, &delegated_inode);
 exit3:
-		dput(dentry);
+		dentry_unlock(dentry);
 	}
-	inode_unlock(path.dentry->d_inode);
 	if (inode)
 		iput(inode);	/* truncate the inode here */
 	inode = NULL;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 5d085428e471..378ee72b57f4 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -80,6 +80,17 @@ struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
 struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
 					    struct qstr *name,
 					    struct dentry *base);
+struct dentry *lookup_and_lock(struct mnt_idmap *idmap,
+			       struct qstr *last, struct dentry *base,
+			       unsigned int lookup_flags);
+struct dentry *lookup_and_lock_noperm(struct qstr *name, struct dentry *base,
+				      unsigned int lookup_flags);
+struct dentry *lookup_and_lock_noperm_locked(struct qstr *name, struct dentry *base,
+					     unsigned int lookup_flags);
+struct dentry *lookup_and_lock_hashed(struct qstr *last, struct dentry *base,
+				      unsigned int lookup_flags);
+void dentry_unlock(struct dentry *dentry);
+void dentry_unlock_dir_locked(struct dentry *dentry);
 
 extern int follow_down_one(struct path *);
 extern int follow_down(struct path *path, unsigned int flags);
-- 
2.49.0

[PATCH 2/5] VFS/btrfs: add lookup_and_lock_killable()

[NeilBrown]

btrfs/ioctl.c uses a "killable" lock on the directory when creating an
destroying subvols.

This patch adds lookup_and_lock_killable() and uses it in btrfs.

Possibly all look_and_lock should be killable as there is no down-side,
but that can come in a later patch.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/btrfs/ioctl.c      | 49 ++++++++++++++-----------------------------
 fs/namei.c            | 36 +++++++++++++++++++++++++++++++
 include/linux/namei.h |  3 +++
 3 files changed, 55 insertions(+), 33 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 913acef3f0a9..9a3af4049c60 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -905,18 +905,15 @@ static noinline int btrfs_mksubvol(const struct path *parent,
 	struct fscrypt_str name_str = FSTR_INIT((char *)name, namelen);
 	int error;
 
-	error = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
-	if (error == -EINTR)
-		return error;
-
-	dentry = lookup_one(idmap, &QSTR_LEN(name, namelen), parent->dentry);
-	error = PTR_ERR(dentry);
+	dentry = lookup_and_lock_killable(idmap, &QSTR_LEN(name, namelen),
+					  parent->dentry,
+					  LOOKUP_CREATE|LOOKUP_EXCL);
 	if (IS_ERR(dentry))
-		goto out_unlock;
+		return PTR_ERR(dentry);
 
 	error = btrfs_may_create(idmap, dir, dentry);
 	if (error)
-		goto out_dput;
+		goto out_unlock;
 
 	/*
 	 * even if this name doesn't exist, we may get hash collisions.
@@ -925,7 +922,7 @@ static noinline int btrfs_mksubvol(const struct path *parent,
 	error = btrfs_check_dir_item_collision(BTRFS_I(dir)->root,
 					       dir->i_ino, &name_str);
 	if (error)
-		goto out_dput;
+		goto out_unlock;
 
 	down_read(&fs_info->subvol_sem);
 
@@ -941,10 +938,8 @@ static noinline int btrfs_mksubvol(const struct path *parent,
 		fsnotify_mkdir(dir, dentry);
 out_up_read:
 	up_read(&fs_info->subvol_sem);
-out_dput:
-	dput(dentry);
 out_unlock:
-	btrfs_inode_unlock(BTRFS_I(dir), 0);
+	dentry_unlock(dentry);
 	return error;
 }
 
@@ -2421,19 +2416,9 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		goto free_subvol_name;
 	}
 
-	ret = down_write_killable_nested(&dir->i_rwsem, I_MUTEX_PARENT);
-	if (ret == -EINTR)
+	dentry = lookup_and_lock_killable(idmap, &QSTR(subvol_name), parent, 0);
+	if (IS_ERR(dentry))
 		goto free_subvol_name;
-	dentry = lookup_one(idmap, &QSTR(subvol_name), parent);
-	if (IS_ERR(dentry)) {
-		ret = PTR_ERR(dentry);
-		goto out_unlock_dir;
-	}
-
-	if (d_really_is_negative(dentry)) {
-		ret = -ENOENT;
-		goto out_dput;
-	}
 
 	inode = d_inode(dentry);
 	dest = BTRFS_I(inode)->root;
@@ -2453,7 +2438,7 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		 */
 		ret = -EPERM;
 		if (!btrfs_test_opt(fs_info, USER_SUBVOL_RM_ALLOWED))
-			goto out_dput;
+			goto out_unlock;
 
 		/*
 		 * Do not allow deletion if the parent dir is the same
@@ -2464,21 +2449,21 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 		 */
 		ret = -EINVAL;
 		if (root == dest)
-			goto out_dput;
+			goto out_unlock;
 
 		ret = inode_permission(idmap, inode, MAY_WRITE | MAY_EXEC);
 		if (ret)
-			goto out_dput;
+			goto out_unlock;
 	}
 
 	/* check if subvolume may be deleted by a user */
 	ret = btrfs_may_delete(idmap, dir, dentry, 1);
 	if (ret)
-		goto out_dput;
+		goto out_unlock;
 
 	if (btrfs_ino(BTRFS_I(inode)) != BTRFS_FIRST_FREE_OBJECTID) {
 		ret = -EINVAL;
-		goto out_dput;
+		goto out_unlock;
 	}
 
 	btrfs_inode_lock(BTRFS_I(inode), 0);
@@ -2487,10 +2472,8 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	if (!ret)
 		d_delete_notify(dir, dentry);
 
-out_dput:
-	dput(dentry);
-out_unlock_dir:
-	btrfs_inode_unlock(BTRFS_I(dir), 0);
+out_unlock:
+	dentry_unlock(dentry);
 free_subvol_name:
 	kfree(subvol_name_ptr);
 free_parent:
diff --git a/fs/namei.c b/fs/namei.c
index 5e8fe2d78486..55ce5700ba0e 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1814,6 +1814,42 @@ struct dentry *lookup_and_lock(struct mnt_idmap *idmap,
 }
 EXPORT_SYMBOL(lookup_and_lock);
 
+/**
+ * lookup_and_lock_killable - lookup and lock a name prior to dir ops
+ * @last: the name in the given directory
+ * @base: the directory in which the name is to be found
+ * @lookup_flags: %LOOKUP_xxx flags
+ *
+ * The name is looked up and necessary locks are taken so that
+ * the name can be created or removed.
+ * The "necessary locks" are currently the inode node lock on @base.
+ * If a fatal signal arrives or is already pending the operation is aborted.
+ * The name @last is NOT expected to already have the hash calculated.
+ * Permission checks are performed to ensure %MAY_EXEC access to @base.
+ * Returns: the dentry, suitably locked, or an ERR_PTR().
+ */
+struct dentry *lookup_and_lock_killable(struct mnt_idmap *idmap,
+					struct qstr *last,
+					struct dentry *base,
+					unsigned int lookup_flags)
+{
+	struct dentry *dentry;
+	int err;
+
+	err = down_write_killable_nested(&base->d_inode->i_rwsem, I_MUTEX_PARENT);
+	if (err)
+		return ERR_PTR(err);
+	err = lookup_one_common(idmap, last, base);
+	if (err < 0)
+		return ERR_PTR(err);
+
+	dentry = lookup_one_qstr_excl(last, base, lookup_flags);
+	if (IS_ERR(dentry))
+		inode_unlock(base->d_inode);
+	return dentry;
+}
+EXPORT_SYMBOL(lookup_and_lock_killable);
+
 void dentry_unlock_dir_locked(struct dentry *dentry)
 {
 	d_lookup_done(dentry);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 378ee72b57f4..5177499a2f6b 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -83,6 +83,9 @@ struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
 struct dentry *lookup_and_lock(struct mnt_idmap *idmap,
 			       struct qstr *last, struct dentry *base,
 			       unsigned int lookup_flags);
+struct dentry *lookup_and_lock_killable(struct mnt_idmap *idmap,
+					struct qstr *last, struct dentry *base,
+					unsigned int lookup_flags);
 struct dentry *lookup_and_lock_noperm(struct qstr *name, struct dentry *base,
 				      unsigned int lookup_flags);
 struct dentry *lookup_and_lock_noperm_locked(struct qstr *name, struct dentry *base,
-- 
2.49.0

[PATCH 3/5] VFS: change old_dir and new_dir in struct renamedata to dentrys

[NeilBrown]

all users of 'struct renamedata' have the dentry for the old and new
directories, and often have no use for the inode except to store it in
the renamedata.

This patch changes struct renamedata to hold the dentry, rather than
the inode, for the old and new directories, and changes callers to
match.

This results in the removal of several local variables and several
dereferences of ->d_inode at the cost of adding ->d_inode dereferences
to vfs_rename().

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/cachefiles/namei.c    |  4 ++--
 fs/ecryptfs/inode.c      |  4 ++--
 fs/namei.c               |  6 +++---
 fs/nfsd/vfs.c            |  7 ++-----
 fs/overlayfs/copy_up.c   |  6 +++---
 fs/overlayfs/dir.c       | 16 ++++++++--------
 fs/overlayfs/overlayfs.h |  6 +++---
 fs/overlayfs/readdir.c   |  2 +-
 fs/overlayfs/super.c     |  2 +-
 fs/overlayfs/util.c      |  2 +-
 fs/smb/server/vfs.c      |  4 ++--
 include/linux/fs.h       |  4 ++--
 12 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index 6644f0694169..abc689d3df54 100644
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -392,10 +392,10 @@ int cachefiles_bury_object(struct cachefiles_cache *cache,
 	} else {
 		struct renamedata rd = {
 			.old_mnt_idmap	= &nop_mnt_idmap,
-			.old_dir	= d_inode(dir),
+			.old_dir	= dir,
 			.old_dentry	= rep,
 			.new_mnt_idmap	= &nop_mnt_idmap,
-			.new_dir	= d_inode(cache->graveyard),
+			.new_dir	= cache->graveyard,
 			.new_dentry	= grave,
 		};
 		trace_cachefiles_rename(object, d_inode(rep)->i_ino, why);
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index c513e912ae3c..3e627bcbaff1 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -636,10 +636,10 @@ ecryptfs_rename(struct mnt_idmap *idmap, struct inode *old_dir,
 	}
 
 	rd.old_mnt_idmap	= &nop_mnt_idmap;
-	rd.old_dir		= d_inode(lower_old_dir_dentry);
+	rd.old_dir		= lower_old_dir_dentry;
 	rd.old_dentry		= lower_old_dentry;
 	rd.new_mnt_idmap	= &nop_mnt_idmap;
-	rd.new_dir		= d_inode(lower_new_dir_dentry);
+	rd.new_dir		= lower_new_dir_dentry;
 	rd.new_dentry		= lower_new_dentry;
 	rc = vfs_rename(&rd);
 	if (rc)
diff --git a/fs/namei.c b/fs/namei.c
index 55ce5700ba0e..32895140abde 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -5142,7 +5142,7 @@ SYSCALL_DEFINE2(link, const char __user *, oldname, const char __user *, newname
 int vfs_rename(struct renamedata *rd)
 {
 	int error;
-	struct inode *old_dir = rd->old_dir, *new_dir = rd->new_dir;
+	struct inode *old_dir = d_inode(rd->old_dir), *new_dir = d_inode(rd->new_dir);
 	struct dentry *old_dentry = rd->old_dentry;
 	struct dentry *new_dentry = rd->new_dentry;
 	struct inode **delegated_inode = rd->delegated_inode;
@@ -5401,10 +5401,10 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 	if (error)
 		goto exit5;
 
-	rd.old_dir	   = old_path.dentry->d_inode;
+	rd.old_dir	   = old_path.dentry;
 	rd.old_dentry	   = old_dentry;
 	rd.old_mnt_idmap   = mnt_idmap(old_path.mnt);
-	rd.new_dir	   = new_path.dentry->d_inode;
+	rd.new_dir	   = new_path.dentry;
 	rd.new_dentry	   = new_dentry;
 	rd.new_mnt_idmap   = mnt_idmap(new_path.mnt);
 	rd.delegated_inode = &delegated_inode;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index be29a18a23b2..a957e2631f49 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1872,7 +1872,6 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 			    struct svc_fh *tfhp, char *tname, int tlen)
 {
 	struct dentry	*fdentry, *tdentry, *odentry, *ndentry, *trap;
-	struct inode	*fdir, *tdir;
 	int		type = S_IFDIR;
 	__be32		err;
 	int		host_err;
@@ -1888,10 +1887,8 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 		goto out;
 
 	fdentry = ffhp->fh_dentry;
-	fdir = d_inode(fdentry);
 
 	tdentry = tfhp->fh_dentry;
-	tdir = d_inode(tdentry);
 
 	err = nfserr_perm;
 	if (!flen || isdotent(fname, flen) || !tlen || isdotent(tname, tlen))
@@ -1952,10 +1949,10 @@ nfsd_rename(struct svc_rqst *rqstp, struct svc_fh *ffhp, char *fname, int flen,
 	} else {
 		struct renamedata rd = {
 			.old_mnt_idmap	= &nop_mnt_idmap,
-			.old_dir	= fdir,
+			.old_dir	= fdentry,
 			.old_dentry	= odentry,
 			.new_mnt_idmap	= &nop_mnt_idmap,
-			.new_dir	= tdir,
+			.new_dir	= tdentry,
 			.new_dentry	= ndentry,
 		};
 		int retries;
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 324429d02569..b14f9ca99d26 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -569,7 +569,7 @@ static int ovl_create_index(struct dentry *dentry, const struct ovl_fh *fh,
 	if (IS_ERR(index)) {
 		err = PTR_ERR(index);
 	} else {
-		err = ovl_do_rename(ofs, dir, temp, dir, index, 0);
+		err = ovl_do_rename(ofs, indexdir, temp, indexdir, index, 0);
 		dput(index);
 	}
 out:
@@ -770,7 +770,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 {
 	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
 	struct inode *inode;
-	struct inode *udir = d_inode(c->destdir), *wdir = d_inode(c->workdir);
+	struct inode *wdir = d_inode(c->workdir);
 	struct path path = { .mnt = ovl_upper_mnt(ofs) };
 	struct dentry *temp, *upper, *trap;
 	struct ovl_cu_creds cc;
@@ -838,7 +838,7 @@ static int ovl_copy_up_workdir(struct ovl_copy_up_ctx *c)
 	if (IS_ERR(upper))
 		goto cleanup;
 
-	err = ovl_do_rename(ofs, wdir, temp, udir, upper, 0);
+	err = ovl_do_rename(ofs, c->workdir, temp, c->destdir, upper, 0);
 	dput(upper);
 	if (err)
 		goto cleanup;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index b4d92b51b63f..c95e3f5684f7 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -107,7 +107,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
 }
 
 /* Caller must hold i_mutex on both workdir and dir */
-int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
+int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir, // FIXME
 			     struct dentry *dentry)
 {
 	struct inode *wdir = ofs->workdir->d_inode;
@@ -123,7 +123,7 @@ int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
 	if (d_is_dir(dentry))
 		flags = RENAME_EXCHANGE;
 
-	err = ovl_do_rename(ofs, wdir, whiteout, dir, dentry, flags);
+	err = ovl_do_rename(ofs, ofs->workdir, whiteout, dir, dentry, flags);
 	if (err)
 		goto kill_whiteout;
 	if (flags)
@@ -394,7 +394,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	if (err)
 		goto out_cleanup;
 
-	err = ovl_do_rename(ofs, wdir, opaquedir, udir, upper, RENAME_EXCHANGE);
+	err = ovl_do_rename(ofs, workdir, opaquedir, upperdir, upper, RENAME_EXCHANGE);
 	if (err)
 		goto out_cleanup;
 
@@ -506,14 +506,14 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		if (err)
 			goto out_cleanup;
 
-		err = ovl_do_rename(ofs, wdir, newdentry, udir, upper,
+		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper,
 				    RENAME_EXCHANGE);
 		if (err)
 			goto out_cleanup;
 
 		ovl_cleanup(ofs, wdir, upper);
 	} else {
-		err = ovl_do_rename(ofs, wdir, newdentry, udir, upper, 0);
+		err = ovl_do_rename(ofs, workdir, newdentry, upperdir, upper, 0);
 		if (err)
 			goto out_cleanup;
 	}
@@ -789,7 +789,7 @@ static int ovl_remove_and_whiteout(struct dentry *dentry,
 		goto out_dput_upper;
 	}
 
-	err = ovl_cleanup_and_whiteout(ofs, d_inode(upperdir), upper);
+	err = ovl_cleanup_and_whiteout(ofs, upperdir, upper);
 	if (err)
 		goto out_d_drop;
 
@@ -1261,8 +1261,8 @@ static int ovl_rename(struct mnt_idmap *idmap, struct inode *olddir,
 	if (err)
 		goto out_dput;
 
-	err = ovl_do_rename(ofs, old_upperdir->d_inode, olddentry,
-			    new_upperdir->d_inode, newdentry, flags);
+	err = ovl_do_rename(ofs, old_upperdir, olddentry,
+			    new_upperdir, newdentry, flags);
 	if (err)
 		goto out_dput;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 44df3a2449e7..0e4e5ed4cdc0 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -354,8 +354,8 @@ static inline int ovl_do_remove_acl(struct ovl_fs *ofs, struct dentry *dentry,
 	return vfs_remove_acl(ovl_upper_mnt_idmap(ofs), dentry, acl_name);
 }
 
-static inline int ovl_do_rename(struct ovl_fs *ofs, struct inode *olddir,
-				struct dentry *olddentry, struct inode *newdir,
+static inline int ovl_do_rename(struct ovl_fs *ofs, struct dentry *olddir,
+				struct dentry *olddentry, struct dentry *newdir,
 				struct dentry *newdentry, unsigned int flags)
 {
 	int err;
@@ -827,7 +827,7 @@ static inline void ovl_copyflags(struct inode *from, struct inode *to)
 
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
-int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct inode *dir,
+int ovl_cleanup_and_whiteout(struct ovl_fs *ofs, struct dentry *dir,
 			     struct dentry *dentry);
 struct ovl_cattr {
 	dev_t rdev;
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 474c80d210d1..68cca52ae2ac 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -1235,7 +1235,7 @@ int ovl_indexdir_cleanup(struct ovl_fs *ofs)
 			 * Whiteout orphan index to block future open by
 			 * handle after overlay nlink dropped to zero.
 			 */
-			err = ovl_cleanup_and_whiteout(ofs, dir, index);
+			err = ovl_cleanup_and_whiteout(ofs, indexdir, index);
 		} else {
 			/* Cleanup orphan index entries */
 			err = ovl_cleanup(ofs, dir, index);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 5f3267e919dd..ce4d6874fcc6 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -583,7 +583,7 @@ static int ovl_check_rename_whiteout(struct ovl_fs *ofs)
 
 	/* Name is inline and stable - using snapshot as a copy helper */
 	take_dentry_name_snapshot(&name, temp);
-	err = ovl_do_rename(ofs, dir, temp, dir, dest, RENAME_WHITEOUT);
+	err = ovl_do_rename(ofs, workdir, temp, workdir, dest, RENAME_WHITEOUT);
 	if (err) {
 		if (err == -EINVAL)
 			err = 0;
diff --git a/fs/overlayfs/util.c b/fs/overlayfs/util.c
index dcccb4b4a66c..2b4754c645ee 100644
--- a/fs/overlayfs/util.c
+++ b/fs/overlayfs/util.c
@@ -1115,7 +1115,7 @@ static void ovl_cleanup_index(struct dentry *dentry)
 	} else if (ovl_index_all(dentry->d_sb)) {
 		/* Whiteout orphan index to block future open by handle */
 		err = ovl_cleanup_and_whiteout(OVL_FS(dentry->d_sb),
-					       dir, index);
+					       indexdir, index);
 	} else {
 		/* Cleanup orphan index entries */
 		err = ovl_cleanup(ofs, dir, index);
diff --git a/fs/smb/server/vfs.c b/fs/smb/server/vfs.c
index a5599d2e9ab0..7e382763b496 100644
--- a/fs/smb/server/vfs.c
+++ b/fs/smb/server/vfs.c
@@ -770,10 +770,10 @@ int ksmbd_vfs_rename(struct ksmbd_work *work, const struct path *old_path,
 	}
 
 	rd.old_mnt_idmap	= mnt_idmap(old_path->mnt),
-	rd.old_dir		= d_inode(old_parent),
+	rd.old_dir		= old_parent,
 	rd.old_dentry		= old_child,
 	rd.new_mnt_idmap	= mnt_idmap(new_path.mnt),
-	rd.new_dir		= new_path.dentry->d_inode,
+	rd.new_dir		= new_path.dentry,
 	rd.new_dentry		= new_dentry,
 	rd.flags		= flags,
 	rd.delegated_inode	= NULL,
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 96c7925a6551..9b54b4e7dbb7 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2014,10 +2014,10 @@ int vfs_unlink(struct mnt_idmap *, struct inode *, struct dentry *,
  */
 struct renamedata {
 	struct mnt_idmap *old_mnt_idmap;
-	struct inode *old_dir;
+	struct dentry *old_dir;
 	struct dentry *old_dentry;
 	struct mnt_idmap *new_mnt_idmap;
-	struct inode *new_dir;
+	struct dentry *new_dir;
 	struct dentry *new_dentry;
 	struct inode **delegated_inode;
 	unsigned int flags;
-- 
2.49.0

[PATCH 4/5] VFS: add lookup_and_lock_rename()

[NeilBrown]

lookup_and_lock_rename() combines locking and lookup for two names.

Two names - new_last and old_last - are added to struct renamedata so it
can be passed to lookup_and_lock_rename() to have the old and new
dentries filled in.

lookup_and_lock_rename_hashed() assumes that the names are already hashed
and skips permission checking.  This is appropriate for use after
filename_parentat().

lookup_and_lock_rename_noperm() does hash the name but avoids permission
checking.  This will be used by debugfs.

dentry_unlock_rename() unlocks.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/namei.c            | 311 +++++++++++++++++++++++++++++++++---------
 include/linux/fs.h    |   4 +
 include/linux/namei.h |   4 +
 3 files changed, 257 insertions(+), 62 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 32895140abde..39868ee40f03 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3475,6 +3475,226 @@ void unlock_rename(struct dentry *p1, struct dentry *p2)
 }
 EXPORT_SYMBOL(unlock_rename);
 
+/**
+ * lookup_and_lock_rename_hashed - lookup and lock names for rename
+ * @rd:           rename data containing relevant details
+ * @lookup_flags: optional LOOKUP_REVAL to pass to ->lookup
+ *
+ * Optionally look up two names and ensure locks are in place for
+ * rename.
+ * Normally @rd.old_dentry and @rd.new_dentry are %NULL and the
+ * old and new directories and last names are given in @rd.  In this
+ * case the names are looked up with appropriate locking and the
+ * results stored in @rd.old_dentry and @rd.new_dentry.
+ *
+ * If either are not NULL, then the corresponding lookup is avoided
+ * but the required locks are still taken.  In this case @rd.old_dir
+ * may be %NULL, otherwise @rd.old_dentry must have that as its d_parent
+ * pointer after the locks are obtained.  @rd.new_dir must always
+ * be non-NULL, and must always be the correct parent after locking.
+ *
+ * On success a reference is held on @rd.old_dentry, @rd.new_dentry,
+ * and @rd.old_dir whether they were originally %NULL or not.  These
+ * references are dropped by dentry_unlock_rename().  @rd.new_dir
+ * must always be non-NULL and no extra reference is taken.
+ *
+ * The passed in qstrs must have the hash calculated, and no permission
+ * checking is performed.
+ *
+ * Returns: zero or an error.
+ */
+int
+lookup_and_lock_rename_hashed(struct renamedata *rd, int lookup_flags)
+{
+	struct dentry *p;
+	struct dentry *d1, *d2;
+	int target_flags = LOOKUP_RENAME_TARGET | LOOKUP_CREATE;
+	int err;
+
+	if (rd->flags & RENAME_EXCHANGE)
+		target_flags = 0;
+	if (rd->flags & RENAME_NOREPLACE)
+		target_flags |= LOOKUP_EXCL;
+
+	if (rd->old_dentry) {
+		/* Already have the dentry - need to be sure to lock the correct parent */
+		p = lock_rename_child(rd->old_dentry, rd->new_dir);
+		if (d_unhashed(rd->old_dentry) ||
+		    (rd->old_dir && rd->old_dir != rd->old_dentry->d_parent)) {
+			/* dentry was removed, or moved and  explicit parent requested */
+			unlock_rename(rd->old_dentry->d_parent, rd->new_dir);
+			return -EINVAL;
+		}
+		rd->old_dir = dget(rd->old_dentry->d_parent);
+		d1 = dget(rd->old_dentry);
+	} else {
+		p = lock_rename(rd->old_dir, rd->new_dir);
+		dget(rd->old_dir);
+
+		d1 = lookup_one_qstr_excl(&rd->old_last, rd->old_dir,
+					  lookup_flags);
+		if (IS_ERR(d1))
+			goto out_unlock_1;
+	}
+	if (rd->new_dentry) {
+		if (d_unhashed(rd->new_dentry) ||
+		    rd->new_dentry->d_parent != rd->new_dir) {
+			/* new_dentry was moved or removed! */
+			goto out_unlock_2;
+		}
+		d2 = dget(rd->new_dentry);
+	} else {
+		d2 = lookup_one_qstr_excl(&rd->new_last, rd->new_dir,
+				  lookup_flags | target_flags);
+		if (IS_ERR(d2))
+			goto out_unlock_2;
+	}
+
+	if (d1 == p) {
+		/* source is an ancestor of target */
+		err = -EINVAL;
+		goto out_unlock_3;
+	}
+
+	if (d2 == p) {
+		/* target is an ancestor of source */
+		if (rd->flags & RENAME_EXCHANGE)
+			err = -EINVAL;
+		else
+			err = -ENOTEMPTY;
+		goto out_unlock_3;
+	}
+
+	rd->old_dentry = d1;
+	rd->new_dentry = d2;
+	return 0;
+
+out_unlock_3:
+	d_lookup_done(d2);
+	dput(d2);
+	d2 = ERR_PTR(err);
+out_unlock_2:
+	d_lookup_done(d1);
+	dput(d1);
+	d1 = d2;
+out_unlock_1:
+	unlock_rename(rd->old_dir, rd->new_dir);
+	dput(rd->old_dir);
+	return PTR_ERR(d1);
+}
+EXPORT_SYMBOL(lookup_and_lock_rename_hashed);
+
+/**
+ * lookup_and_lock_rename_noperm - lookup and lock names for rename
+ * @rd:           rename data containing relevant details
+ * @lookup_flags: optional LOOKUP_REVAL to pass to ->lookup
+ *
+ * Optionally look up two names and ensure locks are in place for
+ * rename.
+ * Normally @rd.old_dentry and @rd.new_dentry are %NULL and the
+ * old and new directories and last names are given in @rd.  In this
+ * case the names are looked up with appropriate locking and the
+ * results stored in @rd.old_dentry and @rd.new_dentry.
+ *
+ * If either are not NULL, then the corresponding lookup is avoided
+ * but the required locks are still taken.  In this case @rd.old_dir
+ * may be %NULL, otherwise @rd.old_dentry must have that as its d_parent
+ * pointer after the locks are obtained.  @rd.new_dir must always
+ * be non-NULL, and must always be the correct parent after locking.
+ *
+ * On success a reference is held on @rd.old_dentry, @rd.new_dentry,
+ * and @rd.old_dir whether they were originally %NULL or not.  These
+ * references are dropped by dentry_unlock_rename().  @rd.new_dir
+ * must always be non-NULL and no extra reference is taken.
+ *
+ * The passed in qstrs need not have the hash calculated, and no
+ * permission checking is performed.
+ *
+ * Returns: zero or an error.
+ */
+int lookup_and_lock_rename_noperm(struct renamedata *rd, int lookup_flags)
+{
+	int err;
+
+	if (!rd->old_dentry) {
+		err = lookup_noperm_common(&rd->old_last, rd->old_dir);
+		if (err)
+			return err;
+	}
+	if (!rd->new_dentry) {
+		err = lookup_noperm_common(&rd->new_last, rd->new_dir);
+		if (err)
+			return err;
+	}
+	return lookup_and_lock_rename_hashed(rd, lookup_flags);
+}
+EXPORT_SYMBOL(lookup_and_lock_rename_noperm);
+
+/**
+ * lookup_and_lock_rename - lookup and lock names for rename
+ * @rd:           rename data containing relevant details
+ * @lookup_flags: optional LOOKUP_REVAL to pass to ->lookup
+ *
+ * Optionally look up two names and ensure locks are in place for
+ * rename.
+ * Normally @rd.old_dentry and @rd.new_dentry are %NULL and the
+ * old and new directories and last names are given in @rd.  In this
+ * case the names are looked up with appropriate locking and the
+ * results stored in @rd.old_dentry and @rd.new_dentry.
+ *
+ * If either are not NULL, then the corresponding lookup is avoided
+ * but the required locks are still taken.  In this case @rd.old_dir
+ * may be %NULL, otherwise @rd.old_dentry must have that as its d_parent
+ * pointer after the locks are obtained.  @rd.new_dir must always
+ * be non-NULL, and must always be the correct parent after locking.
+ *
+ * On success a reference is held on @rd.old_dentry, @rd.new_dentry,
+ * and @rd.old_dir whether they were originally %NULL or not.  These
+ * references are dropped by dentry_unlock_rename().  @rd.new_dir
+ * must always be non-NULL and no extra reference is taken.
+ *
+ * The passed in qstrs need not have the hash calculated, and normal
+ * permission checking for MAY_EXEC is performed.
+ *
+ * Returns: zero or an error.
+ */
+int lookup_and_lock_rename(struct renamedata *rd, int lookup_flags)
+{
+	int err;
+
+	if (!rd->old_dentry) {
+		err = lookup_one_common(rd->old_mnt_idmap, &rd->old_last, rd->old_dir);
+		if (err)
+			return err;
+	}
+	if (!rd->new_dentry) {
+		err = lookup_one_common(rd->new_mnt_idmap, &rd->new_last, rd->new_dir);
+		if (err)
+			return err;
+	}
+	return lookup_and_lock_rename_hashed(rd, lookup_flags);
+}
+EXPORT_SYMBOL(lookup_and_lock_rename);
+
+/**
+ * dentry_unlock_rename - unlock dentries after rename
+ * @rd: the struct renamedata that was passed to lookup_and_lock_rename()
+ *
+ * After a successful lookup_and_lock_rename() (or similar) call, and after
+ * any required renaming is performed, dentry_unlock_rename() must be called
+ * to drop any locks and references that were obtained by the earlier function.
+ */
+void dentry_unlock_rename(struct renamedata *rd)
+{
+	d_lookup_done(rd->old_dentry);
+	d_lookup_done(rd->new_dentry);
+	unlock_rename(rd->old_dir, rd->new_dir);
+	dput(rd->old_dir);
+	dput(rd->old_dentry);
+	dput(rd->new_dentry);
+}
+EXPORT_SYMBOL(dentry_unlock_rename);
+
 /**
  * vfs_prepare_mode - prepare the mode to be used for a new inode
  * @idmap:	idmap of the mount the inode was found from
@@ -5303,14 +5523,10 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 		 struct filename *to, unsigned int flags)
 {
 	struct renamedata rd;
-	struct dentry *old_dentry, *new_dentry;
-	struct dentry *trap;
 	struct path old_path, new_path;
-	struct qstr old_last, new_last;
 	int old_type, new_type;
 	struct inode *delegated_inode = NULL;
-	unsigned int lookup_flags = 0, target_flags =
-		LOOKUP_RENAME_TARGET | LOOKUP_CREATE;
+	unsigned int lookup_flags = 0;
 	bool should_retry = false;
 	int error = -EINVAL;
 
@@ -5321,19 +5537,14 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 	    (flags & RENAME_EXCHANGE))
 		goto put_names;
 
-	if (flags & RENAME_EXCHANGE)
-		target_flags = 0;
-	if (flags & RENAME_NOREPLACE)
-		target_flags |= LOOKUP_EXCL;
-
 retry:
 	error = filename_parentat(olddfd, from, lookup_flags, &old_path,
-				  &old_last, &old_type);
+				  &rd.old_last, &old_type);
 	if (error)
 		goto put_names;
 
-	error = filename_parentat(newdfd, to, lookup_flags, &new_path, &new_last,
-				  &new_type);
+	error = filename_parentat(newdfd, to, lookup_flags, &new_path,
+				  &rd.new_last, &new_type);
 	if (error)
 		goto exit1;
 
@@ -5355,67 +5566,43 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 		goto exit2;
 
 retry_deleg:
-	trap = lock_rename(new_path.dentry, old_path.dentry);
-	if (IS_ERR(trap)) {
-		error = PTR_ERR(trap);
+	rd.old_dir	   = old_path.dentry;
+	rd.old_mnt_idmap   = mnt_idmap(old_path.mnt);
+	rd.old_dentry	   = NULL;
+	rd.new_dir	   = new_path.dentry;
+	rd.new_mnt_idmap   = mnt_idmap(new_path.mnt);
+	rd.new_dentry	   = NULL;
+	rd.delegated_inode = &delegated_inode;
+	rd.flags	   = flags;
+
+	error = lookup_and_lock_rename_hashed(&rd, lookup_flags);
+	if (error)
 		goto exit_lock_rename;
-	}
 
-	old_dentry = lookup_one_qstr_excl(&old_last, old_path.dentry,
-					  lookup_flags);
-	error = PTR_ERR(old_dentry);
-	if (IS_ERR(old_dentry))
-		goto exit3;
-	new_dentry = lookup_one_qstr_excl(&new_last, new_path.dentry,
-					  lookup_flags | target_flags);
-	error = PTR_ERR(new_dentry);
-	if (IS_ERR(new_dentry))
-		goto exit4;
 	if (flags & RENAME_EXCHANGE) {
-		if (!d_is_dir(new_dentry)) {
+		if (!d_is_dir(rd.new_dentry)) {
 			error = -ENOTDIR;
-			if (new_last.name[new_last.len])
-				goto exit5;
+			if (rd.new_last.name[rd.new_last.len])
+				goto exit_unlock;
 		}
 	}
 	/* unless the source is a directory trailing slashes give -ENOTDIR */
-	if (!d_is_dir(old_dentry)) {
+	if (!d_is_dir(rd.old_dentry)) {
 		error = -ENOTDIR;
-		if (old_last.name[old_last.len])
-			goto exit5;
-		if (!(flags & RENAME_EXCHANGE) && new_last.name[new_last.len])
-			goto exit5;
-	}
-	/* source should not be ancestor of target */
-	error = -EINVAL;
-	if (old_dentry == trap)
-		goto exit5;
-	/* target should not be an ancestor of source */
-	if (!(flags & RENAME_EXCHANGE))
-		error = -ENOTEMPTY;
-	if (new_dentry == trap)
-		goto exit5;
+		if (rd.old_last.name[rd.old_last.len])
+			goto exit_unlock;
+		if (!(flags & RENAME_EXCHANGE) && rd.new_last.name[rd.new_last.len])
+			goto exit_unlock;
+	}
 
-	error = security_path_rename(&old_path, old_dentry,
-				     &new_path, new_dentry, flags);
+	error = security_path_rename(&old_path, rd.old_dentry,
+				     &new_path, rd.new_dentry, flags);
 	if (error)
-		goto exit5;
+		goto exit_unlock;
 
-	rd.old_dir	   = old_path.dentry;
-	rd.old_dentry	   = old_dentry;
-	rd.old_mnt_idmap   = mnt_idmap(old_path.mnt);
-	rd.new_dir	   = new_path.dentry;
-	rd.new_dentry	   = new_dentry;
-	rd.new_mnt_idmap   = mnt_idmap(new_path.mnt);
-	rd.delegated_inode = &delegated_inode;
-	rd.flags	   = flags;
 	error = vfs_rename(&rd);
-exit5:
-	dput(new_dentry);
-exit4:
-	dput(old_dentry);
-exit3:
-	unlock_rename(new_path.dentry, old_path.dentry);
+exit_unlock:
+	dentry_unlock_rename(&rd);
 exit_lock_rename:
 	if (delegated_inode) {
 		error = break_deleg_wait(&delegated_inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 9b54b4e7dbb7..24bc29efecd5 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2006,9 +2006,11 @@ int vfs_unlink(struct mnt_idmap *, struct inode *, struct dentry *,
  * @old_mnt_idmap:     idmap of the old mount the inode was found from
  * @old_dir:           parent of source
  * @old_dentry:                source
+ * @old_last:          name for old_dentry in old_dir, if old_dentry not given
  * @new_mnt_idmap:     idmap of the new mount the inode was found from
  * @new_dir:           parent of destination
  * @new_dentry:                destination
+ * @new_last:          name for new_dentry in new_dir, if new_dentry not given
  * @delegated_inode:   returns an inode needing a delegation break
  * @flags:             rename flags
  */
@@ -2016,9 +2018,11 @@ struct renamedata {
 	struct mnt_idmap *old_mnt_idmap;
 	struct dentry *old_dir;
 	struct dentry *old_dentry;
+	struct qstr old_last;
 	struct mnt_idmap *new_mnt_idmap;
 	struct dentry *new_dir;
 	struct dentry *new_dentry;
+	struct qstr new_last;
 	struct inode **delegated_inode;
 	unsigned int flags;
 } __randomize_layout;
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 5177499a2f6b..a51f3caad106 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -102,6 +102,10 @@ extern int follow_up(struct path *);
 extern struct dentry *lock_rename(struct dentry *, struct dentry *);
 extern struct dentry *lock_rename_child(struct dentry *, struct dentry *);
 extern void unlock_rename(struct dentry *, struct dentry *);
+int lookup_and_lock_rename(struct renamedata *rd, int lookup_flags);
+int lookup_and_lock_rename_noperm(struct renamedata *rd, int lookup_flags);
+int lookup_and_lock_rename_hashed(struct renamedata *rd, int lookup_flags);
+void dentry_unlock_rename(struct renamedata *rd);
 
 /**
  * mode_strip_umask - handle vfs umask stripping
-- 
2.49.0

[PATCH 5/5] VFS: introduce lock_and_check_dentry()

[NeilBrown]

A few callers operate on a dentry which they already have - unlike the
normal case where a lookup proceeds an operation.

For these callers lock_and_check_dentry() is provided where other
callers would use lookup_and_lock().  The call will fail if, after the
lock was gained, the child is no longer a child of the given parent.

When the operation completes dentry_unlock() must be called.  An
extra reference is taken when the lock_and_check_dentry() call succeeds
and will be dropped by dentry_unlock().

This patch changes ecryptfs to make use of this new interface.
cachefiles and smb/server can also benefit as will be seen in later
patches.

Note that lock_parent() in ecryptfs is changed to return with the lock
NOT held when an error occurs.

Signed-off-by: NeilBrown <neil@brown.name>
---
 fs/ecryptfs/inode.c   | 124 +++++++++++++++++++++++-------------------
 fs/namei.c            |  26 +++++++++
 include/linux/namei.h |   1 +
 3 files changed, 96 insertions(+), 55 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 3e627bcbaff1..3173ba89bc20 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -26,16 +26,15 @@
 
 static int lock_parent(struct dentry *dentry,
 		       struct dentry **lower_dentry,
-		       struct inode **lower_dir)
+		       struct dentry **lower_dir_dentry)
 {
-	struct dentry *lower_dir_dentry;
 
-	lower_dir_dentry = ecryptfs_dentry_to_lower(dentry->d_parent);
-	*lower_dir = d_inode(lower_dir_dentry);
+	*lower_dir_dentry = ecryptfs_dentry_to_lower(dentry->d_parent);
 	*lower_dentry = ecryptfs_dentry_to_lower(dentry);
 
-	inode_lock_nested(*lower_dir, I_MUTEX_PARENT);
-	return (*lower_dentry)->d_parent == lower_dir_dentry ? 0 : -EINVAL;
+	if (!lock_and_check_dentry(*lower_dir_dentry, *lower_dentry))
+		return -EINVAL;
+	return 0;
 }
 
 static int ecryptfs_inode_test(struct inode *inode, void *lower_inode)
@@ -138,28 +137,30 @@ static int ecryptfs_do_unlink(struct inode *dir, struct dentry *dentry,
 			      struct inode *inode)
 {
 	struct dentry *lower_dentry;
-	struct inode *lower_dir;
+	struct dentry *lower_dir_dentry;
 	int rc;
 
-	rc = lock_parent(dentry, &lower_dentry, &lower_dir);
-	dget(lower_dentry);	// don't even try to make the lower negative
+	rc = lock_parent(dentry, &lower_dentry, &lower_dir_dentry);
+	if (rc) {
+		printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
+		return rc;
+	}
 	if (!rc) {
 		if (d_unhashed(lower_dentry))
 			rc = -EINVAL;
 		else
-			rc = vfs_unlink(&nop_mnt_idmap, lower_dir, lower_dentry,
-					NULL);
+			rc = vfs_unlink(&nop_mnt_idmap, d_inode(lower_dir_dentry),
+					lower_dentry, NULL);
 	}
 	if (rc) {
 		printk(KERN_ERR "Error in vfs_unlink; rc = [%d]\n", rc);
 		goto out_unlock;
 	}
-	fsstack_copy_attr_times(dir, lower_dir);
+	fsstack_copy_attr_times(dir, d_inode(lower_dir_dentry));
 	set_nlink(inode, ecryptfs_inode_to_lower(inode)->i_nlink);
 	inode_set_ctime_to_ts(inode, inode_get_ctime(dir));
 out_unlock:
-	dput(lower_dentry);
-	inode_unlock(lower_dir);
+	dentry_unlock(lower_dentry);
 	if (!rc)
 		d_drop(dentry);
 	return rc;
@@ -182,14 +183,18 @@ ecryptfs_do_create(struct inode *directory_inode,
 		   struct dentry *ecryptfs_dentry, umode_t mode)
 {
 	int rc;
-	struct dentry *lower_dentry;
+	struct dentry *lower_dentry, *lower_dir_dentry;
 	struct inode *lower_dir;
 	struct inode *inode;
 
-	rc = lock_parent(ecryptfs_dentry, &lower_dentry, &lower_dir);
-	if (!rc)
-		rc = vfs_create(&nop_mnt_idmap, lower_dir,
-				lower_dentry, mode, true);
+	rc = lock_parent(ecryptfs_dentry, &lower_dentry, &lower_dir_dentry);
+	if (rc) {
+		printk(KERN_ERR "%s: Failure to create dentry in lower fs; "
+		       "rc = [%d]\n", __func__, rc);
+		return ERR_PTR(rc);
+	}
+	lower_dir = d_inode(lower_dir_dentry);
+	rc = vfs_create(&nop_mnt_idmap, lower_dir, lower_dentry, mode, true);
 	if (rc) {
 		printk(KERN_ERR "%s: Failure to create dentry in lower fs; "
 		       "rc = [%d]\n", __func__, rc);
@@ -205,7 +210,7 @@ ecryptfs_do_create(struct inode *directory_inode,
 	fsstack_copy_attr_times(directory_inode, lower_dir);
 	fsstack_copy_inode_size(directory_inode, lower_dir);
 out_lock:
-	inode_unlock(lower_dir);
+	dentry_unlock(lower_dentry);
 	return inode;
 }
 
@@ -436,16 +441,19 @@ static int ecryptfs_link(struct dentry *old_dentry, struct inode *dir,
 {
 	struct dentry *lower_old_dentry;
 	struct dentry *lower_new_dentry;
+	struct dentry *lower_dir_dentry;
 	struct inode *lower_dir;
 	u64 file_size_save;
 	int rc;
 
 	file_size_save = i_size_read(d_inode(old_dentry));
 	lower_old_dentry = ecryptfs_dentry_to_lower(old_dentry);
-	rc = lock_parent(new_dentry, &lower_new_dentry, &lower_dir);
-	if (!rc)
-		rc = vfs_link(lower_old_dentry, &nop_mnt_idmap, lower_dir,
-			      lower_new_dentry, NULL);
+	rc = lock_parent(new_dentry, &lower_new_dentry, &lower_dir_dentry);
+	if (rc)
+		return rc;
+	lower_dir = d_inode(lower_dir_dentry);
+	rc = vfs_link(lower_old_dentry, &nop_mnt_idmap, lower_dir,
+		      lower_new_dentry, NULL);
 	if (rc || d_really_is_negative(lower_new_dentry))
 		goto out_lock;
 	rc = ecryptfs_interpose(lower_new_dentry, new_dentry, dir->i_sb);
@@ -457,7 +465,7 @@ static int ecryptfs_link(struct dentry *old_dentry, struct inode *dir,
 		  ecryptfs_inode_to_lower(d_inode(old_dentry))->i_nlink);
 	i_size_write(d_inode(new_dentry), file_size_save);
 out_lock:
-	inode_unlock(lower_dir);
+	dentry_unlock(lower_new_dentry);
 	return rc;
 }
 
@@ -472,14 +480,16 @@ static int ecryptfs_symlink(struct mnt_idmap *idmap,
 {
 	int rc;
 	struct dentry *lower_dentry;
+	struct dentry *lower_dir_dentry;
 	struct inode *lower_dir;
 	char *encoded_symname;
 	size_t encoded_symlen;
 	struct ecryptfs_mount_crypt_stat *mount_crypt_stat = NULL;
 
-	rc = lock_parent(dentry, &lower_dentry, &lower_dir);
+	rc = lock_parent(dentry, &lower_dentry, &lower_dir_dentry);
 	if (rc)
-		goto out_lock;
+		goto out;
+	lower_dir = d_inode(lower_dir_dentry);
 	mount_crypt_stat = &ecryptfs_superblock_to_private(
 		dir->i_sb)->mount_crypt_stat;
 	rc = ecryptfs_encrypt_and_encode_filename(&encoded_symname,
@@ -499,7 +509,8 @@ static int ecryptfs_symlink(struct mnt_idmap *idmap,
 	fsstack_copy_attr_times(dir, lower_dir);
 	fsstack_copy_inode_size(dir, lower_dir);
 out_lock:
-	inode_unlock(lower_dir);
+	dentry_unlock(lower_dentry);
+out:
 	if (d_really_is_negative(dentry))
 		d_drop(dentry);
 	return rc;
@@ -509,30 +520,30 @@ static struct dentry *ecryptfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 				     struct dentry *dentry, umode_t mode)
 {
 	int rc;
-	struct dentry *lower_dentry;
+	struct dentry *lower_dentry, *lower_dir_dentry;
 	struct inode *lower_dir;
 
-	rc = lock_parent(dentry, &lower_dentry, &lower_dir);
+	rc = lock_parent(dentry, &lower_dentry, &lower_dir_dentry);
 	if (rc)
 		goto out;
-
+	lower_dir = d_inode(lower_dir_dentry);
 	lower_dentry = vfs_mkdir(&nop_mnt_idmap, lower_dir,
 				 lower_dentry, mode);
 	rc = PTR_ERR(lower_dentry);
 	if (IS_ERR(lower_dentry))
-		goto out_unlocked;
+		goto out;
 	rc = 0;
 	if (d_unhashed(lower_dentry))
-		goto out;
+		goto out_unlock;
 	rc = ecryptfs_interpose(lower_dentry, dentry, dir->i_sb);
 	if (rc)
-		goto out;
+		goto out_unlock;
 	fsstack_copy_attr_times(dir, lower_dir);
 	fsstack_copy_inode_size(dir, lower_dir);
 	set_nlink(dir, lower_dir->i_nlink);
+out_unlock:
+	dentry_unlock(lower_dentry);
 out:
-	inode_unlock(lower_dir);
-out_unlocked:
 	if (d_really_is_negative(dentry))
 		d_drop(dentry);
 	return ERR_PTR(rc);
@@ -540,25 +551,25 @@ static struct dentry *ecryptfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 
 static int ecryptfs_rmdir(struct inode *dir, struct dentry *dentry)
 {
-	struct dentry *lower_dentry;
+	struct dentry *lower_dentry, *lower_dir_dentry;
 	struct inode *lower_dir;
 	int rc;
 
-	rc = lock_parent(dentry, &lower_dentry, &lower_dir);
-	dget(lower_dentry);	// don't even try to make the lower negative
-	if (!rc) {
-		if (d_unhashed(lower_dentry))
-			rc = -EINVAL;
-		else
-			rc = vfs_rmdir(&nop_mnt_idmap, lower_dir, lower_dentry);
-	}
+	rc = lock_parent(dentry, &lower_dentry, &lower_dir_dentry);
+	if (rc)
+		return rc;
+	lower_dir = d_inode(lower_dir_dentry);
+	if (d_unhashed(lower_dentry))
+		rc = -EINVAL;
+	else
+		rc = vfs_rmdir(&nop_mnt_idmap, lower_dir, lower_dentry);
+
 	if (!rc) {
 		clear_nlink(d_inode(dentry));
 		fsstack_copy_attr_times(dir, lower_dir);
 		set_nlink(dir, lower_dir->i_nlink);
 	}
-	dput(lower_dentry);
-	inode_unlock(lower_dir);
+	dentry_unlock(lower_dentry);
 	if (!rc)
 		d_drop(dentry);
 	return rc;
@@ -569,22 +580,25 @@ ecryptfs_mknod(struct mnt_idmap *idmap, struct inode *dir,
 	       struct dentry *dentry, umode_t mode, dev_t dev)
 {
 	int rc;
-	struct dentry *lower_dentry;
+	struct dentry *lower_dentry, *lower_dir_dentry;
 	struct inode *lower_dir;
 
-	rc = lock_parent(dentry, &lower_dentry, &lower_dir);
-	if (!rc)
-		rc = vfs_mknod(&nop_mnt_idmap, lower_dir,
-			       lower_dentry, mode, dev);
-	if (rc || d_really_is_negative(lower_dentry))
+	rc = lock_parent(dentry, &lower_dentry, &lower_dir_dentry);
+	if (rc)
 		goto out;
+	lower_dir = d_inode(lower_dir_dentry);
+	rc = vfs_mknod(&nop_mnt_idmap, lower_dir,
+		       lower_dentry, mode, dev);
+	if (rc || d_really_is_negative(lower_dentry))
+		goto out_unlock;
 	rc = ecryptfs_interpose(lower_dentry, dentry, dir->i_sb);
 	if (rc)
-		goto out;
+		goto out_unlock;
 	fsstack_copy_attr_times(dir, lower_dir);
 	fsstack_copy_inode_size(dir, lower_dir);
+out_unlock:
+	dentry_unlock(lower_dentry);
 out:
-	inode_unlock(lower_dir);
 	if (d_really_is_negative(dentry))
 		d_drop(dentry);
 	return rc;
diff --git a/fs/namei.c b/fs/namei.c
index 39868ee40f03..65f1d50c5a5b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1850,6 +1850,32 @@ struct dentry *lookup_and_lock_killable(struct mnt_idmap *idmap,
 }
 EXPORT_SYMBOL(lookup_and_lock_killable);
 
+/**
+ * lock_and_check_dentry: lock a dentry in given parent prior to dir ops
+ * @child: the dentry to lock
+ * @parent: the dentry of the assumed parent
+ *
+ * The child is locked - currently by taking i_rwsem on the parent - to
+ * prepare for create/remove operations.  If the given parent is no longer
+ * the parent of the dentry after the lock is gained, the lock is released
+ * and the call failed (returns %false).
+ *
+ * A reference is taken to the child on success.  The lock and references
+ * must both be dropped by dentry_unlock() after the operation completes.
+ */
+bool lock_and_check_dentry(struct dentry *child, struct dentry *parent)
+{
+	inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
+	if (child->d_parent == parent) {
+		/* get the child to balance with dentry_unlock which puts it. */
+		dget(child);
+		return true;
+	}
+	inode_unlock(d_inode(parent));
+	return false;
+}
+EXPORT_SYMBOL(lock_and_check_dentry);
+
 void dentry_unlock_dir_locked(struct dentry *dentry)
 {
 	d_lookup_done(dentry);
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a51f3caad106..67c82caa4676 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -106,6 +106,7 @@ int lookup_and_lock_rename(struct renamedata *rd, int lookup_flags);
 int lookup_and_lock_rename_noperm(struct renamedata *rd, int lookup_flags);
 int lookup_and_lock_rename_hashed(struct renamedata *rd, int lookup_flags);
 void dentry_unlock_rename(struct renamedata *rd);
+bool lock_and_check_dentry(struct dentry *child, struct dentry *parent);
 
 /**
  * mode_strip_umask - handle vfs umask stripping
-- 
2.49.0