[PATCH] vfs: nfsd4: fix held lock on atomic_open() failure

[Jori Koolstra]

While working on O_CREAT|O_DIRECTORY I came across a possble unreleased
lock in nfsd4_create_file(). This functions passes the child dentry via
nfsd4_vfs_create() to dentry_create(). If the atomic_open() call in
dentry_create() fails, path->dentry is set to an ERR_PTR value and this
is passed back to nfsd4_create_file(), which ultimately calls
end_creating(child). But since end_creating() is a noop when passed an
error pointer, inode_unlock(de->d_parent->d_inode) is never called on
the parent.

There are several options to remedy this; in this patch I changed
dentry_create() so that path->dentry does not get an error pointer
assigned. This does mean that atomic_open() needs to be changed so that
it doesn't dput() on error. This does not seem to be the responsibility
of atomic_open() but should rather be left to the caller anyway, just
like with the vfs_* functions.

Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>
---
 fs/namei.c | 74 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 42 insertions(+), 32 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index c7fac83c9a85..b58d8be99838 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4342,14 +4342,14 @@ static int may_o_create(struct mnt_idmap *idmap,
  * Attempt to atomically look up, create and open a file from a negative
  * dentry.
  *
- * Returns 0 if successful.  The file will have been created and attached to
- * @file by the filesystem calling finish_open().
- *
- * If the file was looked up only or didn't need creating, FMODE_OPENED won't
- * be set.  The caller will need to perform the open themselves.  @path will
- * have been updated to point to the new dentry.  This may be negative.
- *
- * Returns an error code otherwise.
+ * If FMODE_OPENED is set, the file will have been created and attached to
+ * @file by the filesystem calling finish_open(). If the file was looked up
+ * only (no O_CREAT flag), FMODE_OPENED won't be set. The caller will need
+ * to perform the open themselves. @path will have been updated to point to
+ * the new dentry. This may be negative. atomic_open() does not consume
+ * @dentry on error, but will dput() it if the filesystem replaces it.
+ *
+ * Returns the opened/looked-up dentry on success or ERR_PTR(-E) on failure.
  */
 static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
 				  struct file *file,
@@ -4365,24 +4365,30 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 				       open_to_namei_flags(open_flag), mode);
 	d_lookup_done(dentry);
 	if (!error) {
-		if (file->f_mode & FMODE_OPENED) {
-			if (unlikely(dentry != file->f_path.dentry)) {
-				dput(dentry);
-				dentry = dget(file->f_path.dentry);
-			}
-		} else if (WARN_ON(file->f_path.dentry == DENTRY_NOT_SET)) {
+		struct dentry *new = file->f_path.dentry;
+
+		if (WARN_ON(new == DENTRY_NOT_SET)) {
 			error = -EIO;
-		} else {
-			if (file->f_path.dentry) {
+		} else if (file->f_mode & FMODE_OPENED) {
+			// finish_open() called
+			if (unlikely(dentry != new)) {
 				dput(dentry);
-				dentry = file->f_path.dentry;
+				dentry = dget(new);
 			}
-			if (unlikely(d_is_negative(dentry)))
+		} else if (new) {
+			// finish_no_open() called with dentry replaced
+			if (unlikely(d_is_negative(new))) {
+				dput(new);
 				error = -ENOENT;
+			} else {
+				dput(dentry);
+				dentry = new;
+			}
+		} else if (unlikely(d_is_negative(dentry))) {
+			error = -ENOENT;
 		}
 	}
 	if (error) {
-		dput(dentry);
 		dentry = ERR_PTR(error);
 	}
 	return dentry;
@@ -4412,7 +4418,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	struct inode *dir_inode = dir->d_inode;
 	int open_flag = op->open_flag;
 	struct dentry *dentry;
-	int error, create_error = 0;
+	int error = 0, create_error = 0;
 	umode_t mode = op->mode;
 	DECLARE_WAIT_QUEUE_HEAD_ONSTACK(wq);
 
@@ -4474,10 +4480,13 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (dir_inode->i_op->atomic_open) {
 		if (nd->flags & LOOKUP_DIRECTORY)
 			open_flag |= O_DIRECTORY;
-		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
-		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
-			dentry = ERR_PTR(create_error);
-		return dentry;
+		struct dentry *res = atomic_open(&nd->path, dentry, file, open_flag, mode);
+		error = PTR_ERR_OR_ZERO(res);
+		if (unlikely(create_error) && error == -ENOENT)
+			error = create_error;
+		if (error)
+			goto out_dput;
+		return res;
 	}
 
 	if (d_in_lookup(dentry)) {
@@ -5036,28 +5045,29 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
 	idmap = mnt_idmap(path->mnt);
 
 	if (dir_inode->i_op->atomic_open) {
-		path->dentry = dir;
+		const struct path dir_path = { .mnt = path->mnt, .dentry = dir };
+		struct dentry *res;
+
 		mode = vfs_prepare_mode(idmap, dir_inode, mode, S_IALLUGO, S_IFREG);
 
-		create_error = may_o_create(idmap, path, dentry, mode);
+		create_error = may_o_create(idmap, &dir_path, dentry, mode);
 		if (create_error)
 			flags &= ~O_CREAT;
 
-		dentry = atomic_open(path, dentry, file, flags, mode);
-		error = PTR_ERR_OR_ZERO(dentry);
+		res = atomic_open(&dir_path, dentry, file, flags, mode);
+		error = PTR_ERR_OR_ZERO(res);
 
 		if (unlikely(create_error) && error == -ENOENT)
 			error = create_error;
 
 		if (!error) {
 			if (file->f_mode & FMODE_CREATED)
-				fsnotify_create(dir->d_inode, dentry);
+				fsnotify_create(dir->d_inode, res);
 			if (file->f_mode & FMODE_OPENED)
 				fsnotify_open(file);
-		}
-
-		path->dentry = dentry;
 
+			path->dentry = res;
+		}
 	} else {
 		error = vfs_create(mnt_idmap(path->mnt), path->dentry, mode, NULL);
 		if (!error)
-- 
2.54.0