[PATCH v2 0/3] Overlayfs mkdir related fixes

[PATCH v2 0/3] Overlayfs mkdir related fixes

[Amir Goldstein]

Miklos,

This series supersedes the patch
"ovl: be more prudent when instantiating a new dentry",
which is based on overlayfs-rorw.

The series is based and tested on top of a merge of overlayfs-rorw and
Al's fixes branch and should apply pretty cleanly to fixes branch.

BTW, I noticed there is a small conflict between Al's for-next and
your dedupe-cleanup branch, so you might want to rebase that.

Thanks,
Amir.

Changes since v1:
- Rebase on top of Al's fixes branch
- Use d_instantiate_new() helper
- Add re-factoring + Al's vfs_mkdir() safely patch

Al Viro (1):
  ovl: make ovl_create_real() cope with vfs_mkdir() safely

Amir Goldstein (2):
  ovl: use d_instantiate_new() to instantiate a new dentry
  ovl: create helper ovl_create_temp()

 fs/overlayfs/copy_up.c   | 27 ++++----------
 fs/overlayfs/dir.c       | 96 +++++++++++++++++++++++++++++++++---------------
 fs/overlayfs/inode.c     |  6 +++
 fs/overlayfs/overlayfs.h | 12 +++++-
 fs/overlayfs/super.c     |  2 +-
 5 files changed, 90 insertions(+), 53 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] ovl: use d_instantiate_new() to instantiate a new dentry

[Amir Goldstein]

Currently, there is a small window where ovl_obtain_alias() can
race with ovl_instantiate() and create two aliases for an overlay
directory inode, see Al's explanation in this post:
https://marc.info/?l=linux-fsdevel&m=152599914515224&w=2

This patch fixes the race, by using the d_instantiate_new() helper.

Another logic change by this patch is that if there is an
inconcsistency and a new created upper inode apears to already
exist in icache (hashed by the same real upper inode), we will
export this error to user instead of silently not hashing the new
inode.

Backporting only makes sense for v4.16 where NFS export was introduced.

Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: <stable@vger.kernel.org> #v4.16
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       | 27 +++++++++++++++++----------
 fs/overlayfs/inode.c     |  6 ++++++
 fs/overlayfs/overlayfs.h |  1 +
 3 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 47dc980e8b33..c41825a5ed5f 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -183,23 +183,30 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
 }
 
 /* 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)
+static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
+			   struct dentry *newdentry, bool hardlink)
 {
 	ovl_dir_modified(dentry->d_parent, false);
-	ovl_copyattr(d_inode(newdentry), inode);
 	ovl_dentry_set_upper_alias(dentry);
 	if (!hardlink) {
-		ovl_inode_update(inode, newdentry);
+		int err;
+
+		ovl_inode_init(inode, newdentry, NULL);
+		err = ovl_insert_inode_locked(inode, d_inode(newdentry));
+		if (err)
+			return err;
+
+		d_instantiate_new(dentry, inode);
 	} else {
 		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
 		dput(newdentry);
 		inc_nlink(inode);
-	}
-	d_instantiate(dentry, inode);
-	/* Force lookup of new upper hardlink to find its lower */
-	if (hardlink)
+		d_instantiate(dentry, inode);
+		/* Force lookup of new upper hardlink to find its lower */
 		d_drop(dentry);
+	}
+
+	return 0;
 }
 
 static bool ovl_type_merge(struct dentry *dentry)
@@ -238,7 +245,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 		ovl_set_opaque(dentry, newdentry);
 	}
 
-	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
+	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
 	newdentry = NULL;
 out_dput:
 	dput(newdentry);
@@ -439,7 +446,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 		if (err)
 			goto out_cleanup;
 	}
-	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
+	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
 	newdentry = NULL;
 out_dput2:
 	dput(upper);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 7abcf96e94fc..70c966b1bb5a 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -741,6 +741,12 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
 	return true;
 }
 
+int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode)
+{
+	return insert_inode_locked4(inode, (unsigned long) realinode,
+				    ovl_inode_test, realinode);
+}
+
 struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
 			       bool is_upper)
 {
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index caaa47cea2aa..dd320be86600 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -343,6 +343,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
 bool ovl_is_private_xattr(const char *name);
 
 struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
+int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode);
 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,
-- 
2.7.4

[PATCH v2 2/3] ovl: create helper ovl_create_temp()

[Amir Goldstein]

Al Viro suggested to simplify callers of ovl_create_real() by
returning the created dentry (or ERR_PTR) from ovl_create_real().
This is a spin off of his suggestion, which is cleaner in my opinion.

Also created a wrapper ovl_create_temp_dir() and used it in
ovl_create_index() instead of calling ovl_do_mkdir(), 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>
---
 fs/overlayfs/copy_up.c   | 27 +++++++--------------------
 fs/overlayfs/dir.c       | 47 +++++++++++++++++++++++++++++------------------
 fs/overlayfs/overlayfs.h |  9 ++++++++-
 3 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8bede0742619..4ba16cbeaec2 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_dir(indexdir);
 	if (IS_ERR(temp))
 		goto temp_err;
 
-	err = ovl_do_mkdir(dir, temp, S_IFDIR, true);
-	if (err)
-		goto out;
-
 	err = ovl_set_upper_fh(upper, temp);
 	if (err)
 		goto out_cleanup;
@@ -501,22 +497,13 @@ 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);
-		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,
-				      NULL, true);
-		if (err) {
-			dput(temp);
-			goto out;
-		}
-	}
+	else
+		temp = ovl_create_temp(c->workdir, &cattr, NULL);
+	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 c41825a5ed5f..eb3fed19b1af 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -160,6 +160,26 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 	return err;
 }
 
+struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
+			       struct dentry *hardlink)
+{
+	struct inode *wdir = workdir->d_inode;
+	struct dentry *temp;
+	int err;
+
+	temp = ovl_lookup_temp(workdir);
+	if (IS_ERR(temp))
+		return temp;
+
+	err = ovl_create_real(wdir, temp, attr, hardlink, true);
+	if (err) {
+		dput(temp);
+		return ERR_PTR(err);
+	}
+
+	return temp;
+}
+
 static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
 			       int xerr)
 {
@@ -287,15 +307,11 @@ 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_temp_dir(workdir);
 	err = PTR_ERR(opaquedir);
 	if (IS_ERR(opaquedir))
-		goto out_unlock;
-
-	err = ovl_create_real(wdir, opaquedir,
-			      &(struct cattr){.mode = stat.mode}, NULL, true);
 	if (err)
-		goto out_dput;
+		goto out_unlock;
 
 	err = ovl_copy_xattr(upper, opaquedir);
 	if (err)
@@ -326,7 +342,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);
@@ -387,20 +402,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, hardlink, true);
-	if (err)
-		goto out_dput2;
+	newdentry = ovl_create_temp(workdir, cattr, hardlink);
+	err = PTR_ERR(newdentry);
+	if (IS_ERR(newdentry))
+		goto out_dput;
 
 	/*
 	 * mode could have been mutilated due to umask (e.g. sgid directory)
@@ -449,9 +460,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
 	newdentry = NULL;
 out_dput2:
-	dput(upper);
-out_dput:
 	dput(newdentry);
+out_dput:
+	dput(upper);
 out_unlock:
 	unlock_rename(workdir, upperdir);
 out:
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index dd320be86600..fc0b3e876636 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -369,6 +369,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(struct inode *dir, struct dentry *dentry);
 struct dentry *ovl_lookup_temp(struct dentry *workdir);
 int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 			     struct dentry *dentry);
@@ -380,7 +381,13 @@ struct cattr {
 int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		    struct cattr *attr,
 		    struct dentry *hardlink, bool debug);
-int ovl_cleanup(struct inode *dir, struct dentry *dentry);
+struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
+			       struct dentry *hardlink);
+
+static inline struct dentry *ovl_create_temp_dir(struct dentry *workdir)
+{
+	return ovl_create_temp(workdir, &(struct cattr){.mode = S_IFDIR}, NULL);
+}
 
 /* file.c */
 extern const struct file_operations ovl_file_operations;
-- 
2.7.4

[PATCH v2 3/3] ovl: make ovl_create_real() cope with vfs_mkdir() safely

[Amir Goldstein]

From: Al Viro <viro@zeniv.linux.org.uk>

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.

Pass newdentry to ovl_create_real() by ref, so in the case above, if
lookup finds a good dentry, newdentry will be replaced by the positive
hashed one.

[amir: split re-factoring to prep patch and pass dentry by ref]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/dir.c       | 24 +++++++++++++++++++++---
 fs/overlayfs/overlayfs.h |  2 +-
 fs/overlayfs/super.c     |  2 +-
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index eb3fed19b1af..aeb40dcd9dd4 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -114,9 +114,10 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 	goto out;
 }
 
-int ovl_create_real(struct inode *dir, struct dentry *newdentry,
+int ovl_create_real(struct inode *dir, struct dentry **pnewdentry,
 		    struct cattr *attr, struct dentry *hardlink, bool debug)
 {
+	struct dentry *newdentry = *pnewdentry;
 	int err;
 
 	if (newdentry->d_inode)
@@ -140,6 +141,23 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		case S_IFSOCK:
 			err = ovl_do_mknod(dir, newdentry,
 					   attr->mode, attr->rdev, debug);
+			/*
+			 * 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 (!err && unlikely(d_unhashed(newdentry))) {
+				struct dentry *d;
+
+				d = lookup_one_len(newdentry->d_name.name,
+						   newdentry->d_parent,
+						   newdentry->d_name.len);
+				if (IS_ERR(d))
+					return PTR_ERR(d);
+
+				dput(newdentry);
+				*pnewdentry = newdentry = d;
+			}
 			break;
 
 		case S_IFLNK:
@@ -171,7 +189,7 @@ struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
 	if (IS_ERR(temp))
 		return temp;
 
-	err = ovl_create_real(wdir, temp, attr, hardlink, true);
+	err = ovl_create_real(wdir, &temp, attr, hardlink, true);
 	if (err) {
 		dput(temp);
 		return ERR_PTR(err);
@@ -256,7 +274,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, false);
 	if (err)
 		goto out_dput;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index fc0b3e876636..2fb738d3ff6a 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -378,7 +378,7 @@ struct cattr {
 	umode_t mode;
 	const char *link;
 };
-int ovl_create_real(struct inode *dir, struct dentry *newdentry,
+int ovl_create_real(struct inode *dir, struct dentry **pnewdentry,
 		    struct cattr *attr,
 		    struct dentry *hardlink, bool debug);
 struct dentry *ovl_create_temp(struct dentry *workdir, struct cattr *attr,
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 492d534058ae..d83c0543c03c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -603,7 +603,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 			goto retry;
 		}
 
-		err = ovl_create_real(dir, work,
+		err = ovl_create_real(dir, &work,
 				      &(struct cattr){.mode = S_IFDIR | 0},
 				      NULL, true);
 		if (err)
-- 
2.7.4

Re: [PATCH v2 1/3] ovl: use d_instantiate_new() to instantiate a new dentry

[Amir Goldstein]

On Sat, May 12, 2018 at 12:17 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Currently, there is a small window where ovl_obtain_alias() can
> race with ovl_instantiate() and create two aliases for an overlay
> directory inode, see Al's explanation in this post:
> https://marc.info/?l=linux-fsdevel&m=152599914515224&w=2
>
> This patch fixes the race, by using the d_instantiate_new() helper.
>
> Another logic change by this patch is that if there is an
> inconcsistency and a new created upper inode apears to already
> exist in icache (hashed by the same real upper inode), we will
> export this error to user instead of silently not hashing the new
> inode.
>
> Backporting only makes sense for v4.16 where NFS export was introduced.
>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: <stable@vger.kernel.org> #v4.16
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/dir.c       | 27 +++++++++++++++++----------
>  fs/overlayfs/inode.c     |  6 ++++++
>  fs/overlayfs/overlayfs.h |  1 +
>  3 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 47dc980e8b33..c41825a5ed5f 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -183,23 +183,30 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
>  }
>
>  /* 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)
> +static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
> +                          struct dentry *newdentry, bool hardlink)
>  {
>         ovl_dir_modified(dentry->d_parent, false);
> -       ovl_copyattr(d_inode(newdentry), inode);

Miklos,

Note that removal of ovl_copyattr() here (for hardlink case) causes xfstest
overlay/019 to hit WARN_ON in ovl_create_or_link() about 2 minutes into
the test and causes the test to fail.

WARN_ON(!uid_eq(inode->i_uid, realinode->i_uid));
WARN_ON(!gid_eq(inode->i_gid, realinode->i_gid));

This is a stress tests that makes modification to underlying layer
while overlay is mounted, so it is not surprising.

I think the correct solution is to turn those WARN_ON to
pr_warn_ratelimited(). There are many of those fired by the stress test.
Do you agree this is the correct fix?

Thanks,
Amir.


>         ovl_dentry_set_upper_alias(dentry);
>         if (!hardlink) {
> -               ovl_inode_update(inode, newdentry);
> +               int err;
> +
> +               ovl_inode_init(inode, newdentry, NULL);
> +               err = ovl_insert_inode_locked(inode, d_inode(newdentry));
> +               if (err)
> +                       return err;
> +
> +               d_instantiate_new(dentry, inode);
>         } else {
>                 WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
>                 dput(newdentry);
>                 inc_nlink(inode);
> -       }
> -       d_instantiate(dentry, inode);
> -       /* Force lookup of new upper hardlink to find its lower */
> -       if (hardlink)
> +               d_instantiate(dentry, inode);
> +               /* Force lookup of new upper hardlink to find its lower */
>                 d_drop(dentry);
> +       }
> +
> +       return 0;
>  }
>
>  static bool ovl_type_merge(struct dentry *dentry)
> @@ -238,7 +245,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>                 ovl_set_opaque(dentry, newdentry);
>         }
>
> -       ovl_instantiate(dentry, inode, newdentry, !!hardlink);
> +       err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
>         newdentry = NULL;
>  out_dput:
>         dput(newdentry);
> @@ -439,7 +446,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>                 if (err)
>                         goto out_cleanup;
>         }
> -       ovl_instantiate(dentry, inode, newdentry, !!hardlink);
> +       err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
>         newdentry = NULL;
>  out_dput2:
>         dput(upper);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 7abcf96e94fc..70c966b1bb5a 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -741,6 +741,12 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
>         return true;
>  }
>
> +int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode)
> +{
> +       return insert_inode_locked4(inode, (unsigned long) realinode,
> +                                   ovl_inode_test, realinode);
> +}
> +
>  struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
>                                bool is_upper)
>  {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index caaa47cea2aa..dd320be86600 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -343,6 +343,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
>  bool ovl_is_private_xattr(const char *name);
>
>  struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
> +int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode);
>  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,
> --
> 2.7.4
>

Re: [PATCH v2 1/3] ovl: use d_instantiate_new() to instantiate a new dentry

[Vivek Goyal]

On Sat, May 12, 2018 at 12:17:09PM +0300, Amir Goldstein wrote:
> Currently, there is a small window where ovl_obtain_alias() can
> race with ovl_instantiate() and create two aliases for an overlay
> directory inode, see Al's explanation in this post:
> https://marc.info/?l=linux-fsdevel&m=152599914515224&w=2
> 
> This patch fixes the race, by using the d_instantiate_new() helper.
> 
> Another logic change by this patch is that if there is an
> inconcsistency and a new created upper inode apears to already
> exist in icache (hashed by the same real upper inode), we will
> export this error to user instead of silently not hashing the new
> inode.
> 
> Backporting only makes sense for v4.16 where NFS export was introduced.

Hi Amir,

So for ovlerlay, this race does not exist for directories as you fall
back to lookup path and don't call ovl_obtain_alias(). Does that mean it
exists for non-dir only and that's what you are trying to fix.

I would be nice to add few more lines describing exact race. It makes
it easier and one does not have to find another email thread and
dig out a specific email to figure out what was the race exactly and
how does apply to overlayfs.

Thanks
Vivek


> 
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: <stable@vger.kernel.org> #v4.16
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/dir.c       | 27 +++++++++++++++++----------
>  fs/overlayfs/inode.c     |  6 ++++++
>  fs/overlayfs/overlayfs.h |  1 +
>  3 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 47dc980e8b33..c41825a5ed5f 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -183,23 +183,30 @@ static int ovl_set_opaque(struct dentry *dentry, struct dentry *upperdentry)
>  }
>  
>  /* 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)
> +static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
> +			   struct dentry *newdentry, bool hardlink)
>  {
>  	ovl_dir_modified(dentry->d_parent, false);
> -	ovl_copyattr(d_inode(newdentry), inode);
>  	ovl_dentry_set_upper_alias(dentry);
>  	if (!hardlink) {
> -		ovl_inode_update(inode, newdentry);
> +		int err;
> +
> +		ovl_inode_init(inode, newdentry, NULL);
> +		err = ovl_insert_inode_locked(inode, d_inode(newdentry));
> +		if (err)
> +			return err;
> +
> +		d_instantiate_new(dentry, inode);
>  	} else {
>  		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
>  		dput(newdentry);
>  		inc_nlink(inode);
> -	}
> -	d_instantiate(dentry, inode);
> -	/* Force lookup of new upper hardlink to find its lower */
> -	if (hardlink)
> +		d_instantiate(dentry, inode);
> +		/* Force lookup of new upper hardlink to find its lower */
>  		d_drop(dentry);
> +	}
> +
> +	return 0;
>  }
>  
>  static bool ovl_type_merge(struct dentry *dentry)
> @@ -238,7 +245,7 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>  		ovl_set_opaque(dentry, newdentry);
>  	}
>  
> -	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
> +	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
>  	newdentry = NULL;
>  out_dput:
>  	dput(newdentry);
> @@ -439,7 +446,7 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>  		if (err)
>  			goto out_cleanup;
>  	}
> -	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
> +	err = ovl_instantiate(dentry, inode, newdentry, !!hardlink);
>  	newdentry = NULL;
>  out_dput2:
>  	dput(upper);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 7abcf96e94fc..70c966b1bb5a 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -741,6 +741,12 @@ static bool ovl_verify_inode(struct inode *inode, struct dentry *lowerdentry,
>  	return true;
>  }
>  
> +int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode)
> +{
> +	return insert_inode_locked4(inode, (unsigned long) realinode,
> +				    ovl_inode_test, realinode);
> +}
> +
>  struct inode *ovl_lookup_inode(struct super_block *sb, struct dentry *real,
>  			       bool is_upper)
>  {
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index caaa47cea2aa..dd320be86600 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -343,6 +343,7 @@ int ovl_update_time(struct inode *inode, struct timespec *ts, int flags);
>  bool ovl_is_private_xattr(const char *name);
>  
>  struct inode *ovl_new_inode(struct super_block *sb, umode_t mode, dev_t rdev);
> +int ovl_insert_inode_locked(struct inode *inode, struct inode *realinode);
>  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,
> -- 
> 2.7.4
> 
> --
> 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 v2 1/3] ovl: use d_instantiate_new() to instantiate a new dentry

[Amir Goldstein]

On Mon, May 14, 2018 at 11:30 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Sat, May 12, 2018 at 12:17:09PM +0300, Amir Goldstein wrote:
>> Currently, there is a small window where ovl_obtain_alias() can
>> race with ovl_instantiate() and create two aliases for an overlay
>> directory inode, see Al's explanation in this post:
>> https://marc.info/?l=linux-fsdevel&m=152599914515224&w=2
>>
>> This patch fixes the race, by using the d_instantiate_new() helper.
>>
>> Another logic change by this patch is that if there is an
>> inconcsistency and a new created upper inode apears to already
>> exist in icache (hashed by the same real upper inode), we will
>> export this error to user instead of silently not hashing the new
>> inode.
>>
>> Backporting only makes sense for v4.16 where NFS export was introduced.
>
> Hi Amir,
>
> So for ovlerlay, this race does not exist for directories as you fall
> back to lookup path and don't call ovl_obtain_alias(). Does that mean it
> exists for non-dir only and that's what you are trying to fix.

Hi Vivek,

Thanks for being alert.
You are right about the fact that overlay doesn't call ovl_obtain_alias()
for dir and no, this race is not relevant for non-dir, because non-dir
may have more than one hashed alias.

However, I'm afraid a race does exists with ovl_get_dentry() vs.
ovl_instantiate(). The race can end up with two different inodes
for the same directory. This is what ovl_insert_inode_locked() fixes.

I am not sure if there is also a race with inserting dentry to cache
I'll need to look closer.

>
> I would be nice to add few more lines describing exact race. It makes
> it easier and one does not have to find another email thread and
> dig out a specific email to figure out what was the race exactly and
> how does apply to overlayfs.
>

Well, I did link to the exact post with the description, although
the race Al explains about is completely different than the inode insert
race, so I'll need to revise the commit message.

Thanks,
Amir.