[PATCH 00/11] overlayfs fixes

[PATCH 00/11] overlayfs fixes

[Miklos Szeredi]

This series contains various fixes, including handling the case when
vfs_mkdir() returns success, but requires a new lookup for an instantiated
dentry.

---
Amir Goldstein (8):
  ovl: update documentation for unionmount-testsuite
  ovl: remove WARN_ON() real inode attributes mismatch
  ovl: strip debug argument from ovl_do_ helpers
  ovl: struct cattr cleanups
  ovl: return dentry from ovl_create_real()
  ovl: create helper ovl_create_temp()
  ovl: make ovl_create_real() cope with vfs_mkdir() safely
  ovl: use inode_insert5() to hash a newly created inode

Miklos Szeredi (2):
  ovl: clean up copy-up error paths
  vfs: factor out inode_insert5()

Vivek Goyal (1):
  ovl: Pass argument to ovl_get_inode() in a structure

 Documentation/filesystems/overlayfs.txt |   7 +-
 fs/inode.c                              | 164 ++++++++++++-------------
 fs/overlayfs/copy_up.c                  |  83 ++++---------
 fs/overlayfs/dir.c                      | 207 +++++++++++++++++++++-----------
 fs/overlayfs/export.c                   |   8 +-
 fs/overlayfs/inode.c                    |  28 +++--
 fs/overlayfs/namei.c                    |  10 +-
 fs/overlayfs/overlayfs.h                |  65 +++++-----
 fs/overlayfs/super.c                    |   9 +-
 include/linux/fs.h                      |   4 +
 10 files changed, 315 insertions(+), 270 deletions(-)

-- 
2.14.3

[PATCH 01/11] ovl: update documentation for unionmount-testsuite

[Miklos Szeredi]

From: Amir Goldstein <amir73il@gmail.com>

David's tree is no longer maintained, so point to my maintained fork.

Add --verify flag to the run example, which enables all latest features
and provides test coverage for constant st_ino/st_dev.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Acked-by: David Howells <dhowells@redhat.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 Documentation/filesystems/overlayfs.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.txt b/Documentation/filesystems/overlayfs.txt
index 961b287ef323..72615a2c0752 100644
--- a/Documentation/filesystems/overlayfs.txt
+++ b/Documentation/filesystems/overlayfs.txt
@@ -429,11 +429,12 @@ This verification may cause significant overhead in some cases.
 Testsuite
 ---------
 
-There's testsuite developed by David Howells at:
+There's a testsuite originally developed by David Howells and currently
+maintained by Amir Goldstein at:
 
-  git://git.infradead.org/users/dhowells/unionmount-testsuite.git
+  https://github.com/amir73il/unionmount-testsuite.git
 
 Run as root:
 
   # cd unionmount-testsuite
-  # ./run --ov
+  # ./run --ov --verify
-- 
2.14.3

[PATCH 02/11] ovl: remove WARN_ON() real inode attributes mismatch

[Miklos Szeredi]

From: Amir Goldstein <amir73il@gmail.com>

Overlayfs should cope with online changes to underlying layer
without crashing the kernel, which is what xfstest overlay/019
checks.

This test may sometimes trigger WARN_ON() in ovl_create_or_link()
when linking an overlay inode that has been changed on underlying
layer.

Remove those WARN_ON() to prevent the stress test from failing.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/dir.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 839709c7803a..01902adc7153 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -510,13 +510,6 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 	}
 out_revert_creds:
 	revert_creds(old_cred);
-	if (!err) {
-		struct inode *realinode = d_inode(ovl_dentry_upper(dentry));
-
-		WARN_ON(inode->i_mode != realinode->i_mode);
-		WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
-		WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));
-	}
 	return err;
 }
 
-- 
2.14.3

[PATCH 03/11] ovl: strip debug argument from ovl_do_ helpers

[Miklos Szeredi]

From: Amir Goldstein <amir73il@gmail.com>

It did not prove to be useful.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/copy_up.c   |  9 ++++-----
 fs/overlayfs/dir.c       | 20 ++++++++++----------
 fs/overlayfs/overlayfs.h | 42 ++++++++++++++++++------------------------
 fs/overlayfs/super.c     |  2 +-
 4 files changed, 33 insertions(+), 40 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8bede0742619..a8273dec0fb8 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -369,7 +369,7 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
 	if (IS_ERR(temp))
 		goto temp_err;
 
-	err = ovl_do_mkdir(dir, temp, S_IFDIR, true);
+	err = ovl_do_mkdir(dir, temp, S_IFDIR);
 	if (err)
 		goto out;
 
@@ -439,8 +439,7 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
 			       c->dentry->d_name.len);
 	err = PTR_ERR(upper);
 	if (!IS_ERR(upper)) {
-		err = ovl_do_link(ovl_dentry_upper(c->dentry), udir, upper,
-				  true);
+		err = ovl_do_link(ovl_dentry_upper(c->dentry), udir, upper);
 		dput(upper);
 
 		if (!err) {
@@ -470,7 +469,7 @@ static int ovl_install_temp(struct ovl_copy_up_ctx *c, struct dentry *temp,
 		return PTR_ERR(upper);
 
 	if (c->tmpfile)
-		err = ovl_do_link(temp, udir, upper, true);
+		err = ovl_do_link(temp, udir, upper);
 	else
 		err = ovl_do_rename(d_inode(c->workdir), temp, udir, upper, 0);
 
@@ -511,7 +510,7 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 			goto temp_err;
 
 		err = ovl_create_real(d_inode(c->workdir), temp, &cattr,
-				      NULL, true);
+				      NULL);
 		if (err) {
 			dput(temp);
 			goto out;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 01902adc7153..4d431a3f7a0a 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -115,7 +115,7 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 }
 
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
-		    struct cattr *attr, struct dentry *hardlink, bool debug)
+		    struct cattr *attr, struct dentry *hardlink)
 {
 	int err;
 
@@ -123,27 +123,27 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		return -ESTALE;
 
 	if (hardlink) {
-		err = ovl_do_link(hardlink, dir, newdentry, debug);
+		err = ovl_do_link(hardlink, dir, newdentry);
 	} else {
 		switch (attr->mode & S_IFMT) {
 		case S_IFREG:
-			err = ovl_do_create(dir, newdentry, attr->mode, debug);
+			err = ovl_do_create(dir, newdentry, attr->mode);
 			break;
 
 		case S_IFDIR:
-			err = ovl_do_mkdir(dir, newdentry, attr->mode, debug);
+			err = ovl_do_mkdir(dir, newdentry, attr->mode);
 			break;
 
 		case S_IFCHR:
 		case S_IFBLK:
 		case S_IFIFO:
 		case S_IFSOCK:
-			err = ovl_do_mknod(dir, newdentry,
-					   attr->mode, attr->rdev, debug);
+			err = ovl_do_mknod(dir, newdentry, attr->mode,
+					   attr->rdev);
 			break;
 
 		case S_IFLNK:
-			err = ovl_do_symlink(dir, newdentry, attr->link, debug);
+			err = ovl_do_symlink(dir, newdentry, attr->link);
 			break;
 
 		default:
@@ -229,7 +229,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	err = PTR_ERR(newdentry);
 	if (IS_ERR(newdentry))
 		goto out_unlock;
-	err = ovl_create_real(udir, newdentry, attr, hardlink, false);
+	err = ovl_create_real(udir, newdentry, attr, hardlink);
 	if (err)
 		goto out_dput;
 
@@ -286,7 +286,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 		goto out_unlock;
 
 	err = ovl_create_real(wdir, opaquedir,
-			      &(struct cattr){.mode = stat.mode}, NULL, true);
+			      &(struct cattr){.mode = stat.mode}, NULL);
 	if (err)
 		goto out_dput;
 
@@ -391,7 +391,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	if (IS_ERR(upper))
 		goto out_dput;
 
-	err = ovl_create_real(wdir, newdentry, cattr, hardlink, true);
+	err = ovl_create_real(wdir, newdentry, cattr, hardlink);
 	if (err)
 		goto out_dput2;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index e0b7de799f6b..1c30d60cc290 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -86,6 +86,7 @@ struct ovl_fh {
 static inline int ovl_do_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	int err = vfs_rmdir(dir, dentry);
+
 	pr_debug("rmdir(%pd2) = %i\n", dentry, err);
 	return err;
 }
@@ -93,56 +94,52 @@ static inline int ovl_do_rmdir(struct inode *dir, struct dentry *dentry)
 static inline int ovl_do_unlink(struct inode *dir, struct dentry *dentry)
 {
 	int err = vfs_unlink(dir, dentry, NULL);
+
 	pr_debug("unlink(%pd2) = %i\n", dentry, err);
 	return err;
 }
 
 static inline int ovl_do_link(struct dentry *old_dentry, struct inode *dir,
-			      struct dentry *new_dentry, bool debug)
+			      struct dentry *new_dentry)
 {
 	int err = vfs_link(old_dentry, dir, new_dentry, NULL);
-	if (debug) {
-		pr_debug("link(%pd2, %pd2) = %i\n",
-			 old_dentry, new_dentry, err);
-	}
+
+	pr_debug("link(%pd2, %pd2) = %i\n", old_dentry, new_dentry, err);
 	return err;
 }
 
 static inline int ovl_do_create(struct inode *dir, struct dentry *dentry,
-			     umode_t mode, bool debug)
+				umode_t mode)
 {
 	int err = vfs_create(dir, dentry, mode, true);
-	if (debug)
-		pr_debug("create(%pd2, 0%o) = %i\n", dentry, mode, err);
+
+	pr_debug("create(%pd2, 0%o) = %i\n", dentry, mode, err);
 	return err;
 }
 
 static inline int ovl_do_mkdir(struct inode *dir, struct dentry *dentry,
-			       umode_t mode, bool debug)
+			       umode_t mode)
 {
 	int err = vfs_mkdir(dir, dentry, mode);
-	if (debug)
-		pr_debug("mkdir(%pd2, 0%o) = %i\n", dentry, mode, err);
+	pr_debug("mkdir(%pd2, 0%o) = %i\n", dentry, mode, err);
 	return err;
 }
 
 static inline int ovl_do_mknod(struct inode *dir, struct dentry *dentry,
-			       umode_t mode, dev_t dev, bool debug)
+			       umode_t mode, dev_t dev)
 {
 	int err = vfs_mknod(dir, dentry, mode, dev);
-	if (debug) {
-		pr_debug("mknod(%pd2, 0%o, 0%o) = %i\n",
-			 dentry, mode, dev, err);
-	}
+
+	pr_debug("mknod(%pd2, 0%o, 0%o) = %i\n", dentry, mode, dev, err);
 	return err;
 }
 
 static inline int ovl_do_symlink(struct inode *dir, struct dentry *dentry,
-				 const char *oldname, bool debug)
+				 const char *oldname)
 {
 	int err = vfs_symlink(dir, dentry, oldname);
-	if (debug)
-		pr_debug("symlink(\"%s\", %pd2) = %i\n", oldname, dentry, err);
+
+	pr_debug("symlink(\"%s\", %pd2) = %i\n", oldname, dentry, err);
 	return err;
 }
 
@@ -168,11 +165,8 @@ static inline int ovl_do_rename(struct inode *olddir, struct dentry *olddentry,
 {
 	int err;
 
-	pr_debug("rename(%pd2, %pd2, 0x%x)\n",
-		 olddentry, newdentry, flags);
-
+	pr_debug("rename(%pd2, %pd2, 0x%x)\n", olddentry, newdentry, flags);
 	err = vfs_rename(olddir, olddentry, newdir, newdentry, NULL, flags);
-
 	if (err) {
 		pr_debug("...rename(%pd2, %pd2, ...) = %i\n",
 			 olddentry, newdentry, err);
@@ -362,7 +356,7 @@ struct cattr {
 };
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		    struct cattr *attr,
-		    struct dentry *hardlink, bool debug);
+		    struct dentry *hardlink);
 int ovl_cleanup(struct inode *dir, struct dentry *dentry);
 
 /* copy_up.c */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index e8551c97de51..94a7a654b0b8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -613,7 +613,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 
 		err = ovl_create_real(dir, work,
 				      &(struct cattr){.mode = S_IFDIR | 0},
-				      NULL, true);
+				      NULL);
 		if (err)
 			goto out_dput;
 
-- 
2.14.3

[PATCH 04/11] ovl: struct cattr cleanups

[Miklos Szeredi]

From: Amir Goldstein <amir73il@gmail.com>

* Rename to ovl_cattr

* Fold ovl_create_real() hardlink argument into struct ovl_cattr

* Create macro OVL_CATTR() to initialize struct ovl_cattr from mode

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/copy_up.c   |  5 ++---
 fs/overlayfs/dir.c       | 45 +++++++++++++++++++++------------------------
 fs/overlayfs/overlayfs.h |  9 ++++++---
 fs/overlayfs/super.c     |  4 +---
 4 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index a8273dec0fb8..5f4c78b1bbeb 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -486,7 +486,7 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 	struct dentry *temp;
 	const struct cred *old_creds = NULL;
 	struct cred *new_creds = NULL;
-	struct cattr cattr = {
+	struct ovl_cattr cattr = {
 		/* Can't properly set mode on creation because of the umask */
 		.mode = c->stat.mode & S_IFMT,
 		.rdev = c->stat.rdev,
@@ -509,8 +509,7 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 		if (IS_ERR(temp))
 			goto temp_err;
 
-		err = ovl_create_real(d_inode(c->workdir), temp, &cattr,
-				      NULL);
+		err = ovl_create_real(d_inode(c->workdir), temp, &cattr);
 		if (err) {
 			dput(temp);
 			goto out;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 4d431a3f7a0a..0fb3ef85f298 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -115,15 +115,15 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 }
 
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
-		    struct cattr *attr, struct dentry *hardlink)
+		    struct ovl_cattr *attr)
 {
 	int err;
 
 	if (newdentry->d_inode)
 		return -ESTALE;
 
-	if (hardlink) {
-		err = ovl_do_link(hardlink, dir, newdentry);
+	if (attr->hardlink) {
+		err = ovl_do_link(attr->hardlink, dir, newdentry);
 	} else {
 		switch (attr->mode & S_IFMT) {
 		case S_IFREG:
@@ -213,14 +213,14 @@ static bool ovl_type_origin(struct dentry *dentry)
 }
 
 static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
-			    struct cattr *attr, struct dentry *hardlink)
+			    struct ovl_cattr *attr)
 {
 	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
 	struct inode *udir = upperdir->d_inode;
 	struct dentry *newdentry;
 	int err;
 
-	if (!hardlink && !IS_POSIXACL(udir))
+	if (!attr->hardlink && !IS_POSIXACL(udir))
 		attr->mode &= ~current_umask();
 
 	inode_lock_nested(udir, I_MUTEX_PARENT);
@@ -229,7 +229,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	err = PTR_ERR(newdentry);
 	if (IS_ERR(newdentry))
 		goto out_unlock;
-	err = ovl_create_real(udir, newdentry, attr, hardlink);
+	err = ovl_create_real(udir, newdentry, attr);
 	if (err)
 		goto out_dput;
 
@@ -238,7 +238,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 		ovl_set_opaque(dentry, newdentry);
 	}
 
-	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
+	ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink);
 	newdentry = NULL;
 out_dput:
 	dput(newdentry);
@@ -285,8 +285,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	if (IS_ERR(opaquedir))
 		goto out_unlock;
 
-	err = ovl_create_real(wdir, opaquedir,
-			      &(struct cattr){.mode = stat.mode}, NULL);
+	err = ovl_create_real(wdir, opaquedir, OVL_CATTR(stat.mode));
 	if (err)
 		goto out_dput;
 
@@ -354,8 +353,7 @@ static int ovl_set_upper_acl(struct dentry *upperdentry, const char *name,
 }
 
 static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
-				    struct cattr *cattr,
-				    struct dentry *hardlink)
+				    struct ovl_cattr *cattr)
 {
 	struct dentry *workdir = ovl_workdir(dentry);
 	struct inode *wdir = workdir->d_inode;
@@ -365,6 +363,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	struct dentry *newdentry;
 	int err;
 	struct posix_acl *acl, *default_acl;
+	bool hardlink = !!cattr->hardlink;
 
 	if (WARN_ON(!workdir))
 		return -EROFS;
@@ -391,7 +390,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	if (IS_ERR(upper))
 		goto out_dput;
 
-	err = ovl_create_real(wdir, newdentry, cattr, hardlink);
+	err = ovl_create_real(wdir, newdentry, cattr);
 	if (err)
 		goto out_dput2;
 
@@ -439,7 +438,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		if (err)
 			goto out_cleanup;
 	}
-	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
+	ovl_instantiate(dentry, inode, newdentry, hardlink);
 	newdentry = NULL;
 out_dput2:
 	dput(upper);
@@ -460,8 +459,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 }
 
 static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
-			      struct cattr *attr, struct dentry *hardlink,
-			      bool origin)
+			      struct ovl_cattr *attr, bool origin)
 {
 	int err;
 	const struct cred *old_cred;
@@ -489,7 +487,7 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 	if (override_cred) {
 		override_cred->fsuid = inode->i_uid;
 		override_cred->fsgid = inode->i_gid;
-		if (!hardlink) {
+		if (!attr->hardlink) {
 			err = security_dentry_create_files_as(dentry,
 					attr->mode, &dentry->d_name, old_cred,
 					override_cred);
@@ -502,11 +500,9 @@ static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
 		put_cred(override_cred);
 
 		if (!ovl_dentry_is_whiteout(dentry))
-			err = ovl_create_upper(dentry, inode, attr,
-						hardlink);
+			err = ovl_create_upper(dentry, inode, attr);
 		else
-			err = ovl_create_over_whiteout(dentry, inode, attr,
-							hardlink);
+			err = ovl_create_over_whiteout(dentry, inode, attr);
 	}
 out_revert_creds:
 	revert_creds(old_cred);
@@ -518,7 +514,7 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
 {
 	int err;
 	struct inode *inode;
-	struct cattr attr = {
+	struct ovl_cattr attr = {
 		.rdev = rdev,
 		.link = link,
 	};
@@ -535,7 +531,7 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
 	inode_init_owner(inode, dentry->d_parent->d_inode, mode);
 	attr.mode = inode->i_mode;
 
-	err = ovl_create_or_link(dentry, inode, &attr, NULL, false);
+	err = ovl_create_or_link(dentry, inode, &attr, false);
 	if (err)
 		iput(inode);
 
@@ -594,8 +590,9 @@ static int ovl_link(struct dentry *old, struct inode *newdir,
 	inode = d_inode(old);
 	ihold(inode);
 
-	err = ovl_create_or_link(new, inode, NULL, ovl_dentry_upper(old),
-				 ovl_type_origin(old));
+	err = ovl_create_or_link(new, inode,
+			&(struct ovl_cattr) {.hardlink = ovl_dentry_upper(old)},
+			ovl_type_origin(old));
 	if (err)
 		iput(inode);
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 1c30d60cc290..aa8286419133 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -349,14 +349,17 @@ extern const struct inode_operations ovl_dir_inode_operations;
 struct dentry *ovl_lookup_temp(struct dentry *workdir);
 int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 			     struct dentry *dentry);
-struct cattr {
+struct ovl_cattr {
 	dev_t rdev;
 	umode_t mode;
 	const char *link;
+	struct dentry *hardlink;
 };
+
+#define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
+
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
-		    struct cattr *attr,
-		    struct dentry *hardlink);
+		    struct ovl_cattr *attr);
 int ovl_cleanup(struct inode *dir, struct dentry *dentry);
 
 /* copy_up.c */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 94a7a654b0b8..286d36772e9c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -611,9 +611,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 			goto retry;
 		}
 
-		err = ovl_create_real(dir, work,
-				      &(struct cattr){.mode = S_IFDIR | 0},
-				      NULL);
+		err = ovl_create_real(dir, work, OVL_CATTR(attr.ia_mode));
 		if (err)
 			goto out_dput;
 
-- 
2.14.3

[PATCH 05/11] ovl: return dentry from ovl_create_real()

[Miklos Szeredi]

From: Amir Goldstein <amir73il@gmail.com>

Al Viro suggested to simplify callers of ovl_create_real() by
returning the created dentry (or ERR_PTR) from ovl_create_real().
This prep patch makes Al's patch change less callers.

Also used ovl_create_temp() in ovl_create_index() instead of calling
ovl_do_mkdir() directly, so now all callers of ovl_do_mkdir() are routed
through ovl_create_real(), which paves the way for Al's fix for non-hashed
result from vfs_mkdir().

Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/copy_up.c   | 15 ++++--------
 fs/overlayfs/dir.c       | 62 +++++++++++++++++++++++-------------------------
 fs/overlayfs/overlayfs.h |  4 ++--
 fs/overlayfs/super.c     |  7 +++---
 4 files changed, 40 insertions(+), 48 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 5f4c78b1bbeb..d3e9c1eeb7a4 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -502,19 +502,12 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 
 	if (c->tmpfile) {
 		temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
-		if (IS_ERR(temp))
-			goto temp_err;
 	} else {
-		temp = ovl_lookup_temp(c->workdir);
-		if (IS_ERR(temp))
-			goto temp_err;
-
-		err = ovl_create_real(d_inode(c->workdir), temp, &cattr);
-		if (err) {
-			dput(temp);
-			goto out;
-		}
+		temp = ovl_create_real(d_inode(c->workdir),
+				       ovl_lookup_temp(c->workdir), &cattr);
 	}
+	if (IS_ERR(temp))
+		goto temp_err;
 	err = 0;
 	*tempp = temp;
 out:
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 0fb3ef85f298..425ddb098c4a 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -114,13 +114,17 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 	goto out;
 }
 
-int ovl_create_real(struct inode *dir, struct dentry *newdentry,
-		    struct ovl_cattr *attr)
+struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
+			       struct ovl_cattr *attr)
 {
 	int err;
 
+	if (IS_ERR(newdentry))
+		return newdentry;
+
+	err = -ESTALE;
 	if (newdentry->d_inode)
-		return -ESTALE;
+		goto out;
 
 	if (attr->hardlink) {
 		err = ovl_do_link(attr->hardlink, dir, newdentry);
@@ -157,7 +161,12 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		 */
 		err = -ENOENT;
 	}
-	return err;
+out:
+	if (err) {
+		dput(newdentry);
+		return ERR_PTR(err);
+	}
+	return newdentry;
 }
 
 static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
@@ -224,14 +233,14 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 		attr->mode &= ~current_umask();
 
 	inode_lock_nested(udir, I_MUTEX_PARENT);
-	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
-				   dentry->d_name.len);
+	newdentry = ovl_create_real(udir,
+				    lookup_one_len(dentry->d_name.name,
+						   upperdir,
+						   dentry->d_name.len),
+				    attr);
 	err = PTR_ERR(newdentry);
 	if (IS_ERR(newdentry))
 		goto out_unlock;
-	err = ovl_create_real(udir, newdentry, attr);
-	if (err)
-		goto out_dput;
 
 	if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry)) {
 		/* Setting opaque here is just an optimization, allow to fail */
@@ -239,9 +248,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	}
 
 	ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink);
-	newdentry = NULL;
-out_dput:
-	dput(newdentry);
+	err = 0;
 out_unlock:
 	inode_unlock(udir);
 	return err;
@@ -280,15 +287,12 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	if (upper->d_parent->d_inode != udir)
 		goto out_unlock;
 
-	opaquedir = ovl_lookup_temp(workdir);
+	opaquedir = ovl_create_real(wdir, ovl_lookup_temp(workdir),
+				    OVL_CATTR(stat.mode));
 	err = PTR_ERR(opaquedir);
 	if (IS_ERR(opaquedir))
 		goto out_unlock;
 
-	err = ovl_create_real(wdir, opaquedir, OVL_CATTR(stat.mode));
-	if (err)
-		goto out_dput;
-
 	err = ovl_copy_xattr(upper, opaquedir);
 	if (err)
 		goto out_cleanup;
@@ -318,7 +322,6 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 
 out_cleanup:
 	ovl_cleanup(wdir, opaquedir);
-out_dput:
 	dput(opaquedir);
 out_unlock:
 	unlock_rename(workdir, upperdir);
@@ -379,20 +382,16 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	if (err)
 		goto out;
 
-	newdentry = ovl_lookup_temp(workdir);
-	err = PTR_ERR(newdentry);
-	if (IS_ERR(newdentry))
-		goto out_unlock;
-
 	upper = lookup_one_len(dentry->d_name.name, upperdir,
 			       dentry->d_name.len);
 	err = PTR_ERR(upper);
 	if (IS_ERR(upper))
-		goto out_dput;
+		goto out_unlock;
 
-	err = ovl_create_real(wdir, newdentry, cattr);
-	if (err)
-		goto out_dput2;
+	newdentry = ovl_create_real(wdir, ovl_lookup_temp(workdir), cattr);
+	err = PTR_ERR(newdentry);
+	if (IS_ERR(newdentry))
+		goto out_dput;
 
 	/*
 	 * mode could have been mutilated due to umask (e.g. sgid directory)
@@ -439,11 +438,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 			goto out_cleanup;
 	}
 	ovl_instantiate(dentry, inode, newdentry, hardlink);
-	newdentry = NULL;
-out_dput2:
-	dput(upper);
+	err = 0;
 out_dput:
-	dput(newdentry);
+	dput(upper);
 out_unlock:
 	unlock_rename(workdir, upperdir);
 out:
@@ -455,7 +452,8 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 
 out_cleanup:
 	ovl_cleanup(wdir, newdentry);
-	goto out_dput2;
+	dput(newdentry);
+	goto out_dput;
 }
 
 static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index aa8286419133..6bbde513e068 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -358,8 +358,8 @@ struct ovl_cattr {
 
 #define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
 
-int ovl_create_real(struct inode *dir, struct dentry *newdentry,
-		    struct ovl_cattr *attr);
+struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
+			       struct ovl_cattr *attr);
 int ovl_cleanup(struct inode *dir, struct dentry *dentry);
 
 /* copy_up.c */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 286d36772e9c..704b37311467 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -611,9 +611,10 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 			goto retry;
 		}
 
-		err = ovl_create_real(dir, work, OVL_CATTR(attr.ia_mode));
-		if (err)
-			goto out_dput;
+		work = ovl_create_real(dir, work, OVL_CATTR(attr.ia_mode));
+		err = PTR_ERR(work);
+		if (IS_ERR(work))
+			goto out_err;
 
 		/*
 		 * Try to remove POSIX ACL xattrs from workdir.  We are good if:
-- 
2.14.3

Re: [PATCH 05/11] ovl: return dentry from ovl_create_real()

[Amir Goldstein]

On Tue, May 29, 2018 at 5:41 PM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> From: Amir Goldstein <amir73il@gmail.com>
>
> Al Viro suggested to simplify callers of ovl_create_real() by
> returning the created dentry (or ERR_PTR) from ovl_create_real().
> This prep patch makes Al's patch change less callers.
>
> Also used ovl_create_temp() in ovl_create_index() instead of calling
> ovl_do_mkdir() directly, so now all callers of ovl_do_mkdir() are routed
> through ovl_create_real(), which paves the way for Al's fix for non-hashed
> result from vfs_mkdir().
>

It's good that you split this patch from ovl_create_temp() patch, but
the commit message it utterly not related to the patch anymore and
possibly should retain Al's authorship cause there is no relic of my
work in this patch.

Thanks,
Amir.

> Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/copy_up.c   | 15 ++++--------
>  fs/overlayfs/dir.c       | 62 +++++++++++++++++++++++-------------------------
>  fs/overlayfs/overlayfs.h |  4 ++--
>  fs/overlayfs/super.c     |  7 +++---
>  4 files changed, 40 insertions(+), 48 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 5f4c78b1bbeb..d3e9c1eeb7a4 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -502,19 +502,12 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
>
>         if (c->tmpfile) {
>                 temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
> -               if (IS_ERR(temp))
> -                       goto temp_err;
>         } else {
> -               temp = ovl_lookup_temp(c->workdir);
> -               if (IS_ERR(temp))
> -                       goto temp_err;
> -
> -               err = ovl_create_real(d_inode(c->workdir), temp, &cattr);
> -               if (err) {
> -                       dput(temp);
> -                       goto out;
> -               }
> +               temp = ovl_create_real(d_inode(c->workdir),
> +                                      ovl_lookup_temp(c->workdir), &cattr);
>         }
> +       if (IS_ERR(temp))
> +               goto temp_err;
>         err = 0;
>         *tempp = temp;
>  out:
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 0fb3ef85f298..425ddb098c4a 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -114,13 +114,17 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
>         goto out;
>  }
>
> -int ovl_create_real(struct inode *dir, struct dentry *newdentry,
> -                   struct ovl_cattr *attr)
> +struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
> +                              struct ovl_cattr *attr)
>  {
>         int err;
>
> +       if (IS_ERR(newdentry))
> +               return newdentry;
> +
> +       err = -ESTALE;
>         if (newdentry->d_inode)
> -               return -ESTALE;
> +               goto out;
>
>         if (attr->hardlink) {
>                 err = ovl_do_link(attr->hardlink, dir, newdentry);
> @@ -157,7 +161,12 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>                  */
>                 err = -ENOENT;
>         }
> -       return err;
> +out:
> +       if (err) {
> +               dput(newdentry);
> +               return ERR_PTR(err);
> +       }
> +       return newdentry;
>  }
>
>  static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
> @@ -224,14 +233,14 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>                 attr->mode &= ~current_umask();
>
>         inode_lock_nested(udir, I_MUTEX_PARENT);
> -       newdentry = lookup_one_len(dentry->d_name.name, upperdir,
> -                                  dentry->d_name.len);
> +       newdentry = ovl_create_real(udir,
> +                                   lookup_one_len(dentry->d_name.name,
> +                                                  upperdir,
> +                                                  dentry->d_name.len),
> +                                   attr);
>         err = PTR_ERR(newdentry);
>         if (IS_ERR(newdentry))
>                 goto out_unlock;
> -       err = ovl_create_real(udir, newdentry, attr);
> -       if (err)
> -               goto out_dput;
>
>         if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry)) {
>                 /* Setting opaque here is just an optimization, allow to fail */
> @@ -239,9 +248,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>         }
>
>         ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink);
> -       newdentry = NULL;
> -out_dput:
> -       dput(newdentry);
> +       err = 0;
>  out_unlock:
>         inode_unlock(udir);
>         return err;
> @@ -280,15 +287,12 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>         if (upper->d_parent->d_inode != udir)
>                 goto out_unlock;
>
> -       opaquedir = ovl_lookup_temp(workdir);
> +       opaquedir = ovl_create_real(wdir, ovl_lookup_temp(workdir),
> +                                   OVL_CATTR(stat.mode));
>         err = PTR_ERR(opaquedir);
>         if (IS_ERR(opaquedir))
>                 goto out_unlock;
>
> -       err = ovl_create_real(wdir, opaquedir, OVL_CATTR(stat.mode));
> -       if (err)
> -               goto out_dput;
> -
>         err = ovl_copy_xattr(upper, opaquedir);
>         if (err)
>                 goto out_cleanup;
> @@ -318,7 +322,6 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
>
>  out_cleanup:
>         ovl_cleanup(wdir, opaquedir);
> -out_dput:
>         dput(opaquedir);
>  out_unlock:
>         unlock_rename(workdir, upperdir);
> @@ -379,20 +382,16 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>         if (err)
>                 goto out;
>
> -       newdentry = ovl_lookup_temp(workdir);
> -       err = PTR_ERR(newdentry);
> -       if (IS_ERR(newdentry))
> -               goto out_unlock;
> -
>         upper = lookup_one_len(dentry->d_name.name, upperdir,
>                                dentry->d_name.len);
>         err = PTR_ERR(upper);
>         if (IS_ERR(upper))
> -               goto out_dput;
> +               goto out_unlock;
>
> -       err = ovl_create_real(wdir, newdentry, cattr);
> -       if (err)
> -               goto out_dput2;
> +       newdentry = ovl_create_real(wdir, ovl_lookup_temp(workdir), cattr);
> +       err = PTR_ERR(newdentry);
> +       if (IS_ERR(newdentry))
> +               goto out_dput;
>
>         /*
>          * mode could have been mutilated due to umask (e.g. sgid directory)
> @@ -439,11 +438,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>                         goto out_cleanup;
>         }
>         ovl_instantiate(dentry, inode, newdentry, hardlink);
> -       newdentry = NULL;
> -out_dput2:
> -       dput(upper);
> +       err = 0;
>  out_dput:
> -       dput(newdentry);
> +       dput(upper);
>  out_unlock:
>         unlock_rename(workdir, upperdir);
>  out:
> @@ -455,7 +452,8 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>
>  out_cleanup:
>         ovl_cleanup(wdir, newdentry);
> -       goto out_dput2;
> +       dput(newdentry);
> +       goto out_dput;
>  }
>
>  static int ovl_create_or_link(struct dentry *dentry, struct inode *inode,
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index aa8286419133..6bbde513e068 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -358,8 +358,8 @@ struct ovl_cattr {
>
>  #define OVL_CATTR(m) (&(struct ovl_cattr) { .mode = (m) })
>
> -int ovl_create_real(struct inode *dir, struct dentry *newdentry,
> -                   struct ovl_cattr *attr);
> +struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
> +                              struct ovl_cattr *attr);
>  int ovl_cleanup(struct inode *dir, struct dentry *dentry);
>
>  /* copy_up.c */
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 286d36772e9c..704b37311467 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -611,9 +611,10 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>                         goto retry;
>                 }
>
> -               err = ovl_create_real(dir, work, OVL_CATTR(attr.ia_mode));
> -               if (err)
> -                       goto out_dput;
> +               work = ovl_create_real(dir, work, OVL_CATTR(attr.ia_mode));
> +               err = PTR_ERR(work);
> +               if (IS_ERR(work))
> +                       goto out_err;
>
>                 /*
>                  * Try to remove POSIX ACL xattrs from workdir.  We are good if:
> --
> 2.14.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/11] ovl: return dentry from ovl_create_real()

[Miklos Szeredi]

On Tue, May 29, 2018 at 5:24 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, May 29, 2018 at 5:41 PM, Miklos Szeredi <mszeredi@redhat.com> wrote:
>> From: Amir Goldstein <amir73il@gmail.com>
>>
>> Al Viro suggested to simplify callers of ovl_create_real() by
>> returning the created dentry (or ERR_PTR) from ovl_create_real().
>> This prep patch makes Al's patch change less callers.
>>
>> Also used ovl_create_temp() in ovl_create_index() instead of calling
>> ovl_do_mkdir() directly, so now all callers of ovl_do_mkdir() are routed
>> through ovl_create_real(), which paves the way for Al's fix for non-hashed
>> result from vfs_mkdir().
>>
>
> It's good that you split this patch from ovl_create_temp() patch, but
> the commit message it utterly not related to the patch anymore and
> possibly should retain Al's authorship cause there is no relic of my
> work in this patch.

Fixed headers and authorship assignments.

Thanks,
Miklos

[PATCH 06/11] ovl: create helper ovl_create_temp()

[Miklos Szeredi]

From: Amir Goldstein <amir73il@gmail.com>

Also used ovl_create_temp() in ovl_create_index() instead of calling
ovl_do_mkdir() directly, so now all callers of ovl_do_mkdir() are routed
through ovl_create_real(), which paves the way for Al's fix for non-hashed
result from vfs_mkdir().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/copy_up.c   | 14 ++++----------
 fs/overlayfs/dir.c       | 13 +++++++++----
 fs/overlayfs/overlayfs.h |  2 +-
 3 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d3e9c1eeb7a4..1b442c14c531 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -365,14 +365,10 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
 	if (err)
 		return err;
 
-	temp = ovl_lookup_temp(indexdir);
+	temp = ovl_create_temp(indexdir, OVL_CATTR(S_IFDIR | 0));
 	if (IS_ERR(temp))
 		goto temp_err;
 
-	err = ovl_do_mkdir(dir, temp, S_IFDIR);
-	if (err)
-		goto out;
-
 	err = ovl_set_upper_fh(upper, temp);
 	if (err)
 		goto out_cleanup;
@@ -500,12 +496,10 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 	if (new_creds)
 		old_creds = override_creds(new_creds);
 
-	if (c->tmpfile) {
+	if (c->tmpfile)
 		temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
-	} else {
-		temp = ovl_create_real(d_inode(c->workdir),
-				       ovl_lookup_temp(c->workdir), &cattr);
-	}
+	else
+		temp = ovl_create_temp(c->workdir, &cattr);
 	if (IS_ERR(temp))
 		goto temp_err;
 	err = 0;
diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 425ddb098c4a..1b181292a624 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -43,7 +43,7 @@ int ovl_cleanup(struct inode *wdir, struct dentry *wdentry)
 	return err;
 }
 
-struct dentry *ovl_lookup_temp(struct dentry *workdir)
+static struct dentry *ovl_lookup_temp(struct dentry *workdir)
 {
 	struct dentry *temp;
 	char name[20];
@@ -169,6 +169,12 @@ struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
 	return newdentry;
 }
 
+struct dentry *ovl_create_temp(struct dentry *workdir, struct ovl_cattr *attr)
+{
+	return ovl_create_real(d_inode(workdir), ovl_lookup_temp(workdir),
+			       attr);
+}
+
 static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
 			       int xerr)
 {
@@ -287,8 +293,7 @@ static struct dentry *ovl_clear_empty(struct dentry *dentry,
 	if (upper->d_parent->d_inode != udir)
 		goto out_unlock;
 
-	opaquedir = ovl_create_real(wdir, ovl_lookup_temp(workdir),
-				    OVL_CATTR(stat.mode));
+	opaquedir = ovl_create_temp(workdir, OVL_CATTR(stat.mode));
 	err = PTR_ERR(opaquedir);
 	if (IS_ERR(opaquedir))
 		goto out_unlock;
@@ -388,7 +393,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	if (IS_ERR(upper))
 		goto out_unlock;
 
-	newdentry = ovl_create_real(wdir, ovl_lookup_temp(workdir), cattr);
+	newdentry = ovl_create_temp(workdir, cattr);
 	err = PTR_ERR(newdentry);
 	if (IS_ERR(newdentry))
 		goto out_dput;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 6bbde513e068..3f13d0965e03 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -346,7 +346,6 @@ static inline void ovl_copyattr(struct inode *from, struct inode *to)
 
 /* dir.c */
 extern const struct inode_operations ovl_dir_inode_operations;
-struct dentry *ovl_lookup_temp(struct dentry *workdir);
 int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 			     struct dentry *dentry);
 struct ovl_cattr {
@@ -361,6 +360,7 @@ struct ovl_cattr {
 struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
 			       struct ovl_cattr *attr);
 int ovl_cleanup(struct inode *dir, struct dentry *dentry);
+struct dentry *ovl_create_temp(struct dentry *workdir, struct ovl_cattr *attr);
 
 /* copy_up.c */
 int ovl_copy_up(struct dentry *dentry);
-- 
2.14.3

[PATCH 07/11] ovl: make ovl_create_real() cope with vfs_mkdir() safely

[Miklos Szeredi]

From: Amir Goldstein <amir73il@gmail.com>

vfs_mkdir() may succeed and leave the dentry passed to it unhashed and
negative.  ovl_create_real() is the last caller breaking when that
happens.

[amir: split re-factoring of ovl_create_temp() to prep patch
       add comment about unhashed dir after mkdir
       add pr_warn() if mkdir succeeds and lookup fails]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/dir.c | 35 +++++++++++++++++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 1b181292a624..b33d37d1a87c 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -114,6 +114,37 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 	goto out;
 }
 
+static struct dentry *ovl_mkdir_real(struct inode *dir, struct dentry *dentry,
+				     umode_t mode)
+{
+	int err;
+
+	err = ovl_do_mkdir(dir, dentry, mode);
+	if (err) {
+		dput(dentry);
+		return ERR_PTR(err);
+	}
+
+	/*
+	 * vfs_mkdir() may succeed and leave the dentry passed
+	 * to it unhashed and negative. If that happens, try to
+	 * lookup a new hashed and positive dentry.
+	 */
+	if (unlikely(d_unhashed(dentry))) {
+		struct dentry *d;
+
+		d = lookup_one_len(dentry->d_name.name, dentry->d_parent,
+				   dentry->d_name.len);
+		if (IS_ERR(d)) {
+			pr_warn("overlayfs: failed lookup after mkdir (%pd2, err=%i).\n",
+				dentry, err);
+		}
+		dput(dentry);
+		dentry = d;
+	}
+	return dentry;
+}
+
 struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
 			       struct ovl_cattr *attr)
 {
@@ -135,8 +166,8 @@ struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
 			break;
 
 		case S_IFDIR:
-			err = ovl_do_mkdir(dir, newdentry, attr->mode);
-			break;
+			/* mkdir is special... */
+			return ovl_mkdir_real(dir, newdentry, attr->mode);
 
 		case S_IFCHR:
 		case S_IFBLK:
-- 
2.14.3

Re: [PATCH 07/11] ovl: make ovl_create_real() cope with vfs_mkdir() safely

[Amir Goldstein]

On Tue, May 29, 2018 at 5:41 PM, Miklos Szeredi <mszeredi@redhat.com> wrote:
> From: Amir Goldstein <amir73il@gmail.com>
>
> vfs_mkdir() may succeed and leave the dentry passed to it unhashed and
> negative.  ovl_create_real() is the last caller breaking when that
> happens.
>
> [amir: split re-factoring of ovl_create_temp() to prep patch
>        add comment about unhashed dir after mkdir
>        add pr_warn() if mkdir succeeds and lookup fails]
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/overlayfs/dir.c | 35 +++++++++++++++++++++++++++++++++--
>  1 file changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 1b181292a624..b33d37d1a87c 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -114,6 +114,37 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
>         goto out;
>  }
>
> +static struct dentry *ovl_mkdir_real(struct inode *dir, struct dentry *dentry,
> +                                    umode_t mode)
> +{
> +       int err;
> +
> +       err = ovl_do_mkdir(dir, dentry, mode);
> +       if (err) {
> +               dput(dentry);
> +               return ERR_PTR(err);
> +       }
> +
> +       /*
> +        * vfs_mkdir() may succeed and leave the dentry passed
> +        * to it unhashed and negative. If that happens, try to
> +        * lookup a new hashed and positive dentry.
> +        */
> +       if (unlikely(d_unhashed(dentry))) {
> +               struct dentry *d;
> +
> +               d = lookup_one_len(dentry->d_name.name, dentry->d_parent,
> +                                  dentry->d_name.len);
> +               if (IS_ERR(d)) {
> +                       pr_warn("overlayfs: failed lookup after mkdir (%pd2, err=%i).\n",
> +                               dentry, err);
> +               }
> +               dput(dentry);
> +               dentry = d;
> +       }
> +       return dentry;
> +}
> +
>  struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
>                                struct ovl_cattr *attr)
>  {
> @@ -135,8 +166,8 @@ struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
>                         break;
>
>                 case S_IFDIR:
> -                       err = ovl_do_mkdir(dir, newdentry, attr->mode);
> -                       break;
> +                       /* mkdir is special... */
> +                       return ovl_mkdir_real(dir, newdentry, attr->mode);
>

I like your version with the helper.
So do we not care about the WARN_ON below on non-instantiated dentry
for directories? or we don't need it at all?

Thanks,
Amir.

Re: [PATCH 07/11] ovl: make ovl_create_real() cope with vfs_mkdir() safely

[Miklos Szeredi]

On Tue, May 29, 2018 at 5:29 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Tue, May 29, 2018 at 5:41 PM, Miklos Szeredi <mszeredi@redhat.com> wrote:
>> From: Amir Goldstein <amir73il@gmail.com>
>>
>> vfs_mkdir() may succeed and leave the dentry passed to it unhashed and
>> negative.  ovl_create_real() is the last caller breaking when that
>> happens.
>>
>> [amir: split re-factoring of ovl_create_temp() to prep patch
>>        add comment about unhashed dir after mkdir
>>        add pr_warn() if mkdir succeeds and lookup fails]
>>
>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
>> ---
>>  fs/overlayfs/dir.c | 35 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 33 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
>> index 1b181292a624..b33d37d1a87c 100644
>> --- a/fs/overlayfs/dir.c
>> +++ b/fs/overlayfs/dir.c
>> @@ -114,6 +114,37 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
>>         goto out;
>>  }
>>
>> +static struct dentry *ovl_mkdir_real(struct inode *dir, struct dentry *dentry,
>> +                                    umode_t mode)
>> +{
>> +       int err;
>> +
>> +       err = ovl_do_mkdir(dir, dentry, mode);
>> +       if (err) {
>> +               dput(dentry);
>> +               return ERR_PTR(err);
>> +       }
>> +
>> +       /*
>> +        * vfs_mkdir() may succeed and leave the dentry passed
>> +        * to it unhashed and negative. If that happens, try to
>> +        * lookup a new hashed and positive dentry.
>> +        */
>> +       if (unlikely(d_unhashed(dentry))) {
>> +               struct dentry *d;
>> +
>> +               d = lookup_one_len(dentry->d_name.name, dentry->d_parent,
>> +                                  dentry->d_name.len);
>> +               if (IS_ERR(d)) {
>> +                       pr_warn("overlayfs: failed lookup after mkdir (%pd2, err=%i).\n",
>> +                               dentry, err);
>> +               }
>> +               dput(dentry);
>> +               dentry = d;
>> +       }
>> +       return dentry;
>> +}
>> +
>>  struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
>>                                struct ovl_cattr *attr)
>>  {
>> @@ -135,8 +166,8 @@ struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
>>                         break;
>>
>>                 case S_IFDIR:
>> -                       err = ovl_do_mkdir(dir, newdentry, attr->mode);
>> -                       break;
>> +                       /* mkdir is special... */
>> +                       return ovl_mkdir_real(dir, newdentry, attr->mode);
>>
>
> I like your version with the helper.
> So do we not care about the WARN_ON below on non-instantiated dentry
> for directories? or we don't need it at all?

I think we do.  Filesystems do all sorts of weird an unexpected
things.   We should definitely not assume underlying filesystem
returns in any way sane result.

Fixed, thanks for spotting.

Miklos

[PATCH 08/11] ovl: clean up copy-up error paths

[Miklos Szeredi]

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/copy_up.c | 54 +++++++++++++++++---------------------------------
 1 file changed, 18 insertions(+), 36 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 1b442c14c531..ddaddb4ce4c3 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -366,12 +366,13 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
 		return err;
 
 	temp = ovl_create_temp(indexdir, OVL_CATTR(S_IFDIR | 0));
+	err = PTR_ERR(temp);
 	if (IS_ERR(temp))
-		goto temp_err;
+		goto free_name;
 
 	err = ovl_set_upper_fh(upper, temp);
 	if (err)
-		goto out_cleanup;
+		goto out;
 
 	index = lookup_one_len(name.name, indexdir, name.len);
 	if (IS_ERR(index)) {
@@ -380,23 +381,13 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
 		err = ovl_do_rename(dir, temp, dir, index, 0);
 		dput(index);
 	}
-
-	if (err)
-		goto out_cleanup;
-
 out:
+	if (err)
+		ovl_cleanup(dir, temp);
 	dput(temp);
+free_name:
 	kfree(name.name);
 	return err;
-
-temp_err:
-	err = PTR_ERR(temp);
-	temp = NULL;
-	goto out;
-
-out_cleanup:
-	ovl_cleanup(dir, temp);
-	goto out;
 }
 
 struct ovl_copy_up_ctx {
@@ -476,7 +467,7 @@ static int ovl_install_temp(struct ovl_copy_up_ctx *c, struct dentry *temp,
 	return err;
 }
 
-static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
+static struct dentry *ovl_get_tmpfile(struct ovl_copy_up_ctx *c)
 {
 	int err;
 	struct dentry *temp;
@@ -490,6 +481,7 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 	};
 
 	err = security_inode_copy_up(c->dentry, &new_creds);
+	temp = ERR_PTR(err);
 	if (err < 0)
 		goto out;
 
@@ -500,21 +492,13 @@ static int ovl_get_tmpfile(struct ovl_copy_up_ctx *c, struct dentry **tempp)
 		temp = ovl_do_tmpfile(c->workdir, c->stat.mode);
 	else
 		temp = ovl_create_temp(c->workdir, &cattr);
-	if (IS_ERR(temp))
-		goto temp_err;
-	err = 0;
-	*tempp = temp;
 out:
 	if (new_creds) {
 		revert_creds(old_creds);
 		put_cred(new_creds);
 	}
 
-	return err;
-
-temp_err:
-	err = PTR_ERR(temp);
-	goto out;
+	return temp;
 }
 
 static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
@@ -564,21 +548,21 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
 	struct inode *udir = c->destdir->d_inode;
 	struct inode *inode;
 	struct dentry *newdentry = NULL;
-	struct dentry *temp = NULL;
+	struct dentry *temp;
 	int err;
 
-	err = ovl_get_tmpfile(c, &temp);
-	if (err)
-		goto out;
+	temp = ovl_get_tmpfile(c);
+	if (IS_ERR(temp))
+		return PTR_ERR(temp);
 
 	err = ovl_copy_up_inode(c, temp);
 	if (err)
-		goto out_cleanup;
+		goto out;
 
 	if (S_ISDIR(c->stat.mode) && c->indexed) {
 		err = ovl_create_index(c->dentry, c->lowerpath.dentry, temp);
 		if (err)
-			goto out_cleanup;
+			goto out;
 	}
 
 	if (c->tmpfile) {
@@ -589,7 +573,7 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
 		err = ovl_install_temp(c, temp, &newdentry);
 	}
 	if (err)
-		goto out_cleanup;
+		goto out;
 
 	inode = d_inode(c->dentry);
 	ovl_inode_update(inode, newdentry);
@@ -597,13 +581,11 @@ static int ovl_copy_up_locked(struct ovl_copy_up_ctx *c)
 		ovl_set_flag(OVL_WHITEOUTS, inode);
 
 out:
+	if (err && !c->tmpfile)
+		ovl_cleanup(d_inode(c->workdir), temp);
 	dput(temp);
 	return err;
 
-out_cleanup:
-	if (!c->tmpfile)
-		ovl_cleanup(d_inode(c->workdir), temp);
-	goto out;
 }
 
 /*
-- 
2.14.3

[PATCH 09/11] vfs: factor out inode_insert5()

[Miklos Szeredi]

From: Miklos Szeredi <miklos@szeredi.hu>

Split out common helper for race free insertion of an already allocated
inode into the cache.  Use this from iget5_locked() and
insert_inode_locked4().  Make iget5_locked() use new_inode()/iput() instead
of alloc_inode()/destroy_inode() directly.

Also export to modules for use by filesystems which want to preallocate an
inode before file/directory creation.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/inode.c         | 164 ++++++++++++++++++++++++-----------------------------
 include/linux/fs.h |   4 ++
 2 files changed, 79 insertions(+), 89 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 13ceb98c3bd3..1bea65d37afe 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1002,6 +1002,70 @@ void unlock_two_nondirectories(struct inode *inode1, struct inode *inode2)
 }
 EXPORT_SYMBOL(unlock_two_nondirectories);
 
+/**
+ * inode_insert5 - obtain an inode from a mounted file system
+ * @inode:	pre-allocated inode to use for insert to cache
+ * @hashval:	hash value (usually inode number) to get
+ * @test:	callback used for comparisons between inodes
+ * @set:	callback used to initialize a new struct inode
+ * @data:	opaque data pointer to pass to @test and @set
+ *
+ * Search for the inode specified by @hashval and @data in the inode cache,
+ * and if present it is return it with an increased reference count. This is
+ * a variant of iget5_locked() for callers that don't want to fail on memory
+ * allocation of inode.
+ *
+ * If the inode is not in cache, insert the pre-allocated inode to cache and
+ * return it locked, hashed, and with the I_NEW flag set. The file system gets
+ * to fill it in before unlocking it via unlock_new_inode().
+ *
+ * Note both @test and @set are called with the inode_hash_lock held, so can't
+ * sleep.
+ */
+struct inode *inode_insert5(struct inode *inode, unsigned long hashval,
+			    int (*test)(struct inode *, void *),
+			    int (*set)(struct inode *, void *), void *data)
+{
+	struct hlist_head *head = inode_hashtable + hash(inode->i_sb, hashval);
+	struct inode *old;
+
+again:
+	spin_lock(&inode_hash_lock);
+	old = find_inode(inode->i_sb, head, test, data);
+	if (unlikely(old)) {
+		/*
+		 * Uhhuh, somebody else created the same inode under us.
+		 * Use the old inode instead of the preallocated one.
+		 */
+		spin_unlock(&inode_hash_lock);
+		wait_on_inode(old);
+		if (unlikely(inode_unhashed(old))) {
+			iput(old);
+			goto again;
+		}
+		return old;
+	}
+
+	if (set && unlikely(set(inode, data))) {
+		inode = NULL;
+		goto unlock;
+	}
+
+	/*
+	 * Return the locked inode with I_NEW set, the
+	 * caller is responsible for filling in the contents
+	 */
+	spin_lock(&inode->i_lock);
+	inode->i_state |= I_NEW;
+	hlist_add_head(&inode->i_hash, head);
+	spin_unlock(&inode->i_lock);
+unlock:
+	spin_unlock(&inode_hash_lock);
+
+	return inode;
+}
+EXPORT_SYMBOL(inode_insert5);
+
 /**
  * iget5_locked - obtain an inode from a mounted file system
  * @sb:		super block of file system
@@ -1026,66 +1090,18 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
 		int (*test)(struct inode *, void *),
 		int (*set)(struct inode *, void *), void *data)
 {
-	struct hlist_head *head = inode_hashtable + hash(sb, hashval);
-	struct inode *inode;
-again:
-	spin_lock(&inode_hash_lock);
-	inode = find_inode(sb, head, test, data);
-	spin_unlock(&inode_hash_lock);
+	struct inode *inode = ilookup5(sb, hashval, test, data);
 
-	if (inode) {
-		wait_on_inode(inode);
-		if (unlikely(inode_unhashed(inode))) {
-			iput(inode);
-			goto again;
-		}
-		return inode;
-	}
+	if (!inode) {
+		struct inode *new = new_inode(sb);
 
-	inode = alloc_inode(sb);
-	if (inode) {
-		struct inode *old;
-
-		spin_lock(&inode_hash_lock);
-		/* We released the lock, so.. */
-		old = find_inode(sb, head, test, data);
-		if (!old) {
-			if (set(inode, data))
-				goto set_failed;
-
-			spin_lock(&inode->i_lock);
-			inode->i_state = I_NEW;
-			hlist_add_head(&inode->i_hash, head);
-			spin_unlock(&inode->i_lock);
-			inode_sb_list_add(inode);
-			spin_unlock(&inode_hash_lock);
-
-			/* Return the locked inode with I_NEW set, the
-			 * caller is responsible for filling in the contents
-			 */
-			return inode;
-		}
-
-		/*
-		 * Uhhuh, somebody else created the same inode under
-		 * us. Use the old inode instead of the one we just
-		 * allocated.
-		 */
-		spin_unlock(&inode_hash_lock);
-		destroy_inode(inode);
-		inode = old;
-		wait_on_inode(inode);
-		if (unlikely(inode_unhashed(inode))) {
-			iput(inode);
-			goto again;
+		if (new) {
+			inode = inode_insert5(new, hashval, test, set, data);
+			if (unlikely(inode != new))
+				iput(new);
 		}
 	}
 	return inode;
-
-set_failed:
-	spin_unlock(&inode_hash_lock);
-	destroy_inode(inode);
-	return NULL;
 }
 EXPORT_SYMBOL(iget5_locked);
 
@@ -1426,43 +1442,13 @@ EXPORT_SYMBOL(insert_inode_locked);
 int insert_inode_locked4(struct inode *inode, unsigned long hashval,
 		int (*test)(struct inode *, void *), void *data)
 {
-	struct super_block *sb = inode->i_sb;
-	struct hlist_head *head = inode_hashtable + hash(sb, hashval);
+	struct inode *old = inode_insert5(inode, hashval, test, NULL, data);
 
-	while (1) {
-		struct inode *old = NULL;
-
-		spin_lock(&inode_hash_lock);
-		hlist_for_each_entry(old, head, i_hash) {
-			if (old->i_sb != sb)
-				continue;
-			if (!test(old, data))
-				continue;
-			spin_lock(&old->i_lock);
-			if (old->i_state & (I_FREEING|I_WILL_FREE)) {
-				spin_unlock(&old->i_lock);
-				continue;
-			}
-			break;
-		}
-		if (likely(!old)) {
-			spin_lock(&inode->i_lock);
-			inode->i_state |= I_NEW;
-			hlist_add_head(&inode->i_hash, head);
-			spin_unlock(&inode->i_lock);
-			spin_unlock(&inode_hash_lock);
-			return 0;
-		}
-		__iget(old);
-		spin_unlock(&old->i_lock);
-		spin_unlock(&inode_hash_lock);
-		wait_on_inode(old);
-		if (unlikely(!inode_unhashed(old))) {
-			iput(old);
-			return -EBUSY;
-		}
+	if (old != inode) {
 		iput(old);
+		return -EBUSY;
 	}
+	return 0;
 }
 EXPORT_SYMBOL(insert_inode_locked4);
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..4f637a9b213d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2879,6 +2879,10 @@ extern struct inode *ilookup5(struct super_block *sb, unsigned long hashval,
 		int (*test)(struct inode *, void *), void *data);
 extern struct inode *ilookup(struct super_block *sb, unsigned long ino);
 
+extern struct inode *inode_insert5(struct inode *inode, unsigned long hashval,
+		int (*test)(struct inode *, void *),
+		int (*set)(struct inode *, void *),
+		void *data);
 extern struct inode * iget5_locked(struct super_block *, unsigned long, int (*test)(struct inode *, void *), int (*set)(struct inode *, void *), void *);
 extern struct inode * iget_locked(struct super_block *, unsigned long);
 extern struct inode *find_inode_nowait(struct super_block *,
-- 
2.14.3

Re: [PATCH 09/11] vfs: factor out inode_insert5()

[Al Viro]

On Tue, May 29, 2018 at 04:41:41PM +0200, Miklos Szeredi wrote:
> From: Miklos Szeredi <miklos@szeredi.hu>
> 
> Split out common helper for race free insertion of an already allocated
> inode into the cache.  Use this from iget5_locked() and
> insert_inode_locked4().  Make iget5_locked() use new_inode()/iput() instead
> of alloc_inode()/destroy_inode() directly.

... thus hitting the sucker with ->evict_inode(), in condition that is quite
likely to be unfit to be seen by that.

NAK.

Re: [PATCH 09/11] vfs: factor out inode_insert5()

[Al Viro]

On Sun, Jun 10, 2018 at 06:49:10AM +0100, Al Viro wrote:
> On Tue, May 29, 2018 at 04:41:41PM +0200, Miklos Szeredi wrote:
> > From: Miklos Szeredi <miklos@szeredi.hu>
> > 
> > Split out common helper for race free insertion of an already allocated
> > inode into the cache.  Use this from iget5_locked() and
> > insert_inode_locked4().  Make iget5_locked() use new_inode()/iput() instead
> > of alloc_inode()/destroy_inode() directly.
> 
> ... thus hitting the sucker with ->evict_inode(), in condition that is quite
> likely to be unfit to be seen by that.
> 
> NAK.

To clarify: objection here is against the switch to new_inode/iput.  The rest is
sane.  What makes new_inode() better here, anyway?

Re: [PATCH 09/11] vfs: factor out inode_insert5()

[Miklos Szeredi]

On Sun, Jun 10, 2018 at 8:02 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Sun, Jun 10, 2018 at 06:49:10AM +0100, Al Viro wrote:
>> On Tue, May 29, 2018 at 04:41:41PM +0200, Miklos Szeredi wrote:
>> > From: Miklos Szeredi <miklos@szeredi.hu>
>> >
>> > Split out common helper for race free insertion of an already allocated
>> > inode into the cache.  Use this from iget5_locked() and
>> > insert_inode_locked4().  Make iget5_locked() use new_inode()/iput() instead
>> > of alloc_inode()/destroy_inode() directly.
>>
>> ... thus hitting the sucker with ->evict_inode(), in condition that is quite
>> likely to be unfit to be seen by that.
>>
>> NAK.
>
> To clarify: objection here is against the switch to new_inode/iput.  The rest is
> sane.  What makes new_inode() better here, anyway?

Umm, got to look into this.  The patch has already been pulled by
Linus, but I hope it's salvageable.

Thanks,
Miklos

Re: [PATCH 09/11] vfs: factor out inode_insert5()

[Miklos Szeredi]

On Mon, Jun 11, 2018 at 11:15:55AM +0200, Miklos Szeredi wrote:
> On Sun, Jun 10, 2018 at 8:02 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Sun, Jun 10, 2018 at 06:49:10AM +0100, Al Viro wrote:
> >> On Tue, May 29, 2018 at 04:41:41PM +0200, Miklos Szeredi wrote:
> >> > From: Miklos Szeredi <miklos@szeredi.hu>
> >> >
> >> > Split out common helper for race free insertion of an already allocated
> >> > inode into the cache.  Use this from iget5_locked() and
> >> > insert_inode_locked4().  Make iget5_locked() use new_inode()/iput() instead
> >> > of alloc_inode()/destroy_inode() directly.
> >>
> >> ... thus hitting the sucker with ->evict_inode(), in condition that is quite
> >> likely to be unfit to be seen by that.
> >>
> >> NAK.
> >
> > To clarify: objection here is against the switch to new_inode/iput.  The rest is
> > sane.  What makes new_inode() better here, anyway?
> 
> Umm, got to look into this.  The patch has already been pulled by
> Linus, but I hope it's salvageable.

Incremental follows.  I think it's cleaner to initialize i_state and i_sb_list
up front (hence the use of new_inode()), but could just as well add to sb list
afterwards.

---
diff --git a/fs/inode.c b/fs/inode.c
index 0df41bb77e0f..03c0d7c1296f 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1098,8 +1098,10 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
 
 		if (new) {
 			inode = inode_insert5(new, hashval, test, set, data);
-			if (unlikely(inode != new))
-				iput(new);
+			if (unlikely(inode != new)) {
+				inode_sb_list_del(inode);
+				destroy_inode(new);
+			}
 		}
 	}
 	return inode;

Re: [PATCH 09/11] vfs: factor out inode_insert5()

[Al Viro]

On Mon, Jun 11, 2018 at 01:32:30PM +0200, Miklos Szeredi wrote:

> Incremental follows.  I think it's cleaner to initialize i_state and i_sb_list
> up front (hence the use of new_inode()), but could just as well add to sb list
> afterwards.

> ---
> diff --git a/fs/inode.c b/fs/inode.c
> index 0df41bb77e0f..03c0d7c1296f 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1098,8 +1098,10 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
>  
>  		if (new) {
>  			inode = inode_insert5(new, hashval, test, set, data);
> -			if (unlikely(inode != new))
> -				iput(new);
> +			if (unlikely(inode != new)) {
> +				inode_sb_list_del(inode);
> +				destroy_inode(new);
> +			}

The thing is, until you put it into the list, it's invisible to everyone other
than iget5_locked() - no references in any shared data structures.  Which
outweighs the "it's somewhat irregular in not being on the list" considerably,
as far as the complexity of analysis goes, especially since there are inodes
that never get on that list and it's not something exotic - all sockets and
pipes are that way, for starters.  So IMO that should be dealt with in
inode_insert5().

Re: [PATCH 09/11] vfs: factor out inode_insert5()

[Miklos Szeredi]

On Mon, Jun 11, 2018 at 05:43:55PM +0100, Al Viro wrote:
> On Mon, Jun 11, 2018 at 01:32:30PM +0200, Miklos Szeredi wrote:
> 
> > Incremental follows.  I think it's cleaner to initialize i_state and i_sb_list
> > up front (hence the use of new_inode()), but could just as well add to sb list
> > afterwards.
> 
> > ---
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 0df41bb77e0f..03c0d7c1296f 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -1098,8 +1098,10 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
> >  
> >  		if (new) {
> >  			inode = inode_insert5(new, hashval, test, set, data);
> > -			if (unlikely(inode != new))
> > -				iput(new);
> > +			if (unlikely(inode != new)) {
> > +				inode_sb_list_del(inode);
> > +				destroy_inode(new);
> > +			}
> 
> The thing is, until you put it into the list, it's invisible to everyone other
> than iget5_locked() - no references in any shared data structures.  Which
> outweighs the "it's somewhat irregular in not being on the list" considerably,
> as far as the complexity of analysis goes, especially since there are inodes
> that never get on that list and it's not something exotic - all sockets and
> pipes are that way, for starters.  So IMO that should be dealt with in
> inode_insert5().

Not sure I understand.

We can put inode_sb_list_add() into inode_insert5().  Then what about users of
new_inode() + insert_inode_locked4()?  Those supply an inode that is already on
the sb list.

What about adding to the list after inode_insert5() in the new inode case.

This should be equivalent to what we do currently, AFAICS.

---
diff --git a/fs/inode.c b/fs/inode.c
index 0df41bb77e0f..ad03d4abc600 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1094,12 +1094,14 @@ struct inode *iget5_locked(struct super_block *sb, unsigned long hashval,
 	struct inode *inode = ilookup5(sb, hashval, test, data);
 
 	if (!inode) {
-		struct inode *new = new_inode(sb);
+		struct inode *new = new_inode_pseudo(sb);
 
 		if (new) {
 			inode = inode_insert5(new, hashval, test, set, data);
 			if (unlikely(inode != new))
-				iput(new);
+				destroy_inode(new);
+			else
+				inode_sb_list_add(inode);
 		}
 	}
 	return inode;

[PATCH 10/11] ovl: Pass argument to ovl_get_inode() in a structure

[Miklos Szeredi]

From: Vivek Goyal <vgoyal@redhat.com>

ovl_get_inode() right now has 5 parameters. Soon this patch series will
add 2 more and suddenly argument list starts looking too long.

Hence pass arguments to ovl_get_inode() in a structure and it looks
little cleaner.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/export.c    |  8 +++++++-
 fs/overlayfs/inode.c     | 20 +++++++++++---------
 fs/overlayfs/namei.c     | 10 ++++++++--
 fs/overlayfs/overlayfs.h | 11 ++++++++---
 4 files changed, 34 insertions(+), 15 deletions(-)

diff --git a/fs/overlayfs/export.c b/fs/overlayfs/export.c
index 425a94672300..9941ece61a14 100644
--- a/fs/overlayfs/export.c
+++ b/fs/overlayfs/export.c
@@ -300,12 +300,18 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
 	struct dentry *dentry;
 	struct inode *inode;
 	struct ovl_entry *oe;
+	struct ovl_inode_params oip = {
+		.lowerpath = lowerpath,
+		.index = index,
+		.numlower = !!lower
+	};
 
 	/* We get overlay directory dentries with ovl_lookup_real() */
 	if (d_is_dir(upper ?: lower))
 		return ERR_PTR(-EIO);
 
-	inode = ovl_get_inode(sb, dget(upper), lowerpath, index, !!lower);
+	oip.upperdentry = dget(upper);
+	inode = ovl_get_inode(sb, &oip);
 	if (IS_ERR(inode)) {
 		dput(upper);
 		return ERR_CAST(inode);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 6e3815fb006b..2b9e8370500c 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -749,15 +749,17 @@ static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
 	return true;
 }
 
-struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
-			    struct ovl_path *lowerpath, struct dentry *index,
-			    unsigned int numlower)
+struct inode *ovl_get_inode(struct super_block *sb,
+			    struct ovl_inode_params *oip)
 {
+	struct dentry *upperdentry = oip->upperdentry;
+	struct ovl_path *lowerpath = oip->lowerpath;
 	struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
 	struct inode *inode;
 	struct dentry *lowerdentry = lowerpath ? lowerpath->dentry : NULL;
-	bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, index);
-	int fsid = bylower ? lowerpath->layer->fsid : 0;
+	bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry,
+					oip->index);
+	int fsid = bylower ? oip->lowerpath->layer->fsid : 0;
 	bool is_dir;
 	unsigned long ino = 0;
 
@@ -774,8 +776,8 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
 						      upperdentry);
 		unsigned int nlink = is_dir ? 1 : realinode->i_nlink;
 
-		inode = iget5_locked(sb, (unsigned long) key,
-				     ovl_inode_test, ovl_inode_set, key);
+		inode = iget5_locked(sb, (unsigned long) key, ovl_inode_test,
+				     ovl_inode_set, key);
 		if (!inode)
 			goto out_nomem;
 		if (!(inode->i_state & I_NEW)) {
@@ -811,12 +813,12 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
 	if (upperdentry && ovl_is_impuredir(upperdentry))
 		ovl_set_flag(OVL_IMPURE, inode);
 
-	if (index)
+	if (oip->index)
 		ovl_set_flag(OVL_INDEX, inode);
 
 	/* Check for non-merge dir that may have whiteouts */
 	if (is_dir) {
-		if (((upperdentry && lowerdentry) || numlower > 1) ||
+		if (((upperdentry && lowerdentry) || oip->numlower > 1) ||
 		    ovl_check_origin_xattr(upperdentry ?: lowerdentry)) {
 			ovl_set_flag(OVL_WHITEOUTS, inode);
 		}
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 2dba29eadde6..08801b45df00 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1004,8 +1004,14 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		upperdentry = dget(index);
 
 	if (upperdentry || ctr) {
-		inode = ovl_get_inode(dentry->d_sb, upperdentry, stack, index,
-				      ctr);
+		struct ovl_inode_params oip = {
+			.upperdentry = upperdentry,
+			.lowerpath = stack,
+			.index = index,
+			.numlower = ctr,
+		};
+
+		inode = ovl_get_inode(dentry->d_sb, &oip);
 		err = PTR_ERR(inode);
 		if (IS_ERR(inode))
 			goto out_free_oe;
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 3f13d0965e03..b8a0160742b2 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -328,12 +328,17 @@ int ovl_open_maybe_copy_up(struct dentry *dentry, unsigned int file_flags);
 int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
 bool ovl_is_private_xattr(const char *name);
 
+struct ovl_inode_params {
+	struct dentry *upperdentry;
+	struct ovl_path *lowerpath;
+	struct dentry *index;
+	unsigned int numlower;
+};
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
 struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
 			       bool is_upper);
-struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
-			    struct ovl_path *lowerpath, struct dentry *index,
-			    unsigned int numlower);
+struct inode *ovl_get_inode(struct super_block *sb,
+			    struct ovl_inode_params *oip);
 static inline void ovl_copyattr(struct inode *from, struct inode *to)
 {
 	to->i_uid = from->i_uid;
-- 
2.14.3

[PATCH 11/11] ovl: use inode_insert5() to hash a newly created inode

[Miklos Szeredi]

From: Amir Goldstein <amir73il@gmail.com>

Currently, there is a small window where ovl_obtain_alias() can
race with ovl_instantiate() and create two different overlay inodes
with the same underlying real non-dir non-hardlink inode.

The race requires an adversary to guess the file handle of the
yet to be created upper inode and decode the guessed file handle
after ovl_creat_real(), but before ovl_instantiate().
This race does not affect overlay directory inodes, because those
are decoded via ovl_lookup_real() and not with ovl_obtain_alias().

This patch fixes the race, by using inode_insert5() to add a newly
created inode to cache.

If the newly created inode apears to already exist in cache (hashed
by the same real upper inode), we instantiate the dentry with the old
inode and drop the new inode, instead of silently not hashing the new
inode.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
 fs/overlayfs/dir.c       | 59 ++++++++++++++++++++++++++++++++++++++++--------
 fs/overlayfs/inode.c     | 12 ++++++++--
 fs/overlayfs/overlayfs.h |  1 +
 3 files changed, 60 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index b33d37d1a87c..b2bc313241a6 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -228,24 +228,54 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
 	return ovl_set_opaque_xerr(dentry, upperdentry, -EIO);
 }
 
-/* Common operations required to be done after creation of file on upper */
-static void ovl_instantiate(struct dentry *dentry, struct inode *inode,
-			    struct dentry *newdentry, bool hardlink)
+/*
+ * Common operations required to be done after creation of file on upper.
+ * If @hardlink is false, then @inode is a pre-allocated inode, we may or
+ * may not use to instantiate the new dentry.
+ */
+static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
+			   struct dentry *newdentry, bool hardlink)
 {
+	struct ovl_inode_params oip = {
+		.upperdentry = newdentry,
+		.newinode = inode,
+	};
+
 	ovl_dentry_version_inc(dentry->d_parent, false);
 	ovl_dentry_set_upper_alias(dentry);
 	if (!hardlink) {
-		ovl_inode_update(inode, newdentry);
-		ovl_copyattr(newdentry->d_inode, inode);
+		/*
+		 * ovl_obtain_alias() can be called after ovl_create_real()
+		 * and before we get here, so we may get an inode from cache
+		 * with the same real upperdentry that is not the inode we
+		 * pre-allocated.  In this case we will use the cached inode
+		 * to instantiate the new dentry.
+		 *
+		 * XXX: if we ever use ovl_obtain_alias() to decode directory
+		 * file handles, need to use ovl_get_inode_locked() and
+		 * d_instantiate_new() here to prevent from creating two
+		 * hashed directory inode aliases.
+		 */
+		inode = ovl_get_inode(dentry->d_sb, &oip);
+		if (WARN_ON(IS_ERR(inode)))
+			return PTR_ERR(inode);
 	} else {
 		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
 		dput(newdentry);
 		inc_nlink(inode);
 	}
+
 	d_instantiate(dentry, inode);
+	if (inode != oip.newinode) {
+		pr_warn_ratelimited("overlayfs: newly created inode found in cache (%pd2)\n",
+				    dentry);
+	}
+
 	/* Force lookup of new upper hardlink to find its lower */
 	if (hardlink)
 		d_drop(dentry);
+
+	return 0;
 }
 
 static bool ovl_type_merge(struct dentry *dentry)
@@ -284,11 +314,17 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 		ovl_set_opaque(dentry, newdentry);
 	}
 
-	ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink);
-	err = 0;
+	err = ovl_instantiate(dentry, inode, newdentry, !!attr->hardlink);
+	if (err)
+		goto out_cleanup;
 out_unlock:
 	inode_unlock(udir);
 	return err;
+
+out_cleanup:
+	ovl_cleanup(udir, newdentry);
+	dput(newdentry);
+	goto out_unlock;
 }
 
 static struct dentry *ovl_clear_empty(struct dentry *dentry,
@@ -473,8 +509,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		if (err)
 			goto out_cleanup;
 	}
-	ovl_instantiate(dentry, inode, newdentry, hardlink);
-	err = 0;
+	err = ovl_instantiate(dentry, inode, newdentry, hardlink);
+	if (err)
+		goto out_cleanup;
 out_dput:
 	dput(upper);
 out_unlock:
@@ -557,6 +594,7 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
 	if (err)
 		goto out;
 
+	/* Preallocate inode to be used by ovl_get_inode() */
 	err = -ENOMEM;
 	inode = ovl_new_inode(dentry->d_sb, mode, rdev);
 	if (!inode)
@@ -566,7 +604,8 @@ static int ovl_create_object(struct dentry *dentry, int mode, dev_t rdev,
 	attr.mode = inode->i_mode;
 
 	err = ovl_create_or_link(dentry, inode, &attr, false);
-	if (err)
+	/* Did we end up using the preallocated inode? */
+	if (inode != d_inode(dentry))
 		iput(inode);
 
 out_drop_write:
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 2b9e8370500c..1db5b3b458a1 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -749,6 +749,15 @@ static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
 	return true;
 }
 
+static struct inode *ovl_iget5(struct super_block *sb, struct inode *newinode,
+			       struct inode *key)
+{
+	return newinode ? inode_insert5(newinode, (unsigned long) key,
+					 ovl_inode_test, ovl_inode_set, key) :
+			  iget5_locked(sb, (unsigned long) key,
+				       ovl_inode_test, ovl_inode_set, key);
+}
+
 struct inode *ovl_get_inode(struct super_block *sb,
 			    struct ovl_inode_params *oip)
 {
@@ -776,8 +785,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 						      upperdentry);
 		unsigned int nlink = is_dir ? 1 : realinode->i_nlink;
 
-		inode = iget5_locked(sb, (unsigned long) key, ovl_inode_test,
-				     ovl_inode_set, key);
+		inode = ovl_iget5(sb, oip->newinode, key);
 		if (!inode)
 			goto out_nomem;
 		if (!(inode->i_state & I_NEW)) {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index b8a0160742b2..3c5e9f18b0d9 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -329,6 +329,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
 bool ovl_is_private_xattr(const char *name);
 
 struct ovl_inode_params {
+	struct inode *newinode;
 	struct dentry *upperdentry;
 	struct ovl_path *lowerpath;
 	struct dentry *index;
-- 
2.14.3