[RFC][PATCH] ovl_create_index(): vfs_mkdir() might succeed leaving dentry negative unhashed

[RFC][PATCH] ovl_create_index(): vfs_mkdir() might succeed leaving dentry negative unhashed

[Al Viro]

[same story as with the previous two patches]

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8bede0742619..cdd8f8816d2a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -373,6 +373,22 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
 	if (err)
 		goto out;
 
+	if (unlikely(d_unhashed(temp))) {
+		struct dentry *d = lookup_one_len(temp->d_name.name,
+						  temp->d_parent,
+						  temp->d_name.len);
+		if (IS_ERR(d)) {
+			err = PTR_ERR(d);
+			goto out;
+		}
+		dput(temp);
+		temp = d;
+		if (d_is_negative(temp)) {
+			err = -EIO;
+			goto out;
+		}
+	}
+
 	err = ovl_set_upper_fh(upper, temp);
 	if (err)
 		goto out_cleanup;

[PATCH][RFC] ovl_create_real(): make it cope with vfs_mkdir() safely

[Al Viro]

[this one really, really needs a review by overlayfs folks;
all I can promise is that it compiles.]

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.

Make ovl_create_real() return the dentry to be used or ERR_PTR(-E...)
in case of error; usually that'll be the dentry passed to it, but
it might be a different one - result of lookup in case of vfs_mkdir()
leaving looking the inode, etc. up to the next lookup to come.

In that case (as well as in case of an error), original dentry is
dropped.  That simplifies the callers.  Moreover, passing ERR_PTR()
as dentry leads to immediate return of the same ERR_PTR().  That
simplifies the callers even more.

Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index cdd8f8816d2a..e0939d69470f 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -517,22 +517,14 @@ 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_real(d_inode(c->workdir),
+					ovl_lookup_temp(c->workdir),
+					&cattr, NULL, true);
+	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 839709c7803a..bc07bdac013f 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -114,13 +114,18 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
 	goto out;
 }
 
-int ovl_create_real(struct inode *dir, struct dentry *newdentry,
+struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		    struct cattr *attr, struct dentry *hardlink, bool debug)
 {
 	int err;
 
-	if (newdentry->d_inode)
-		return -ESTALE;
+	if (IS_ERR(newdentry))
+		return newdentry;
+
+	if (newdentry->d_inode) {
+		dput(newdentry);
+		return ERR_PTR(-ESTALE);
+	}
 
 	if (hardlink) {
 		err = ovl_do_link(hardlink, dir, newdentry, debug);
@@ -132,6 +137,16 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 
 		case S_IFDIR:
 			err = ovl_do_mkdir(dir, newdentry, attr->mode, debug);
+			if (!err && unlikely(d_unhashed(newdentry))) {
+				struct dentry *d;
+				d = lookup_one_len(newdentry->d_name.name,
+						   newdentry->d_parent,
+						   newdentry->d_name.len);
+				dput(newdentry);
+				if (IS_ERR(d))
+					return d;
+				newdentry = d;
+			}
 			break;
 
 		case S_IFCHR:
@@ -157,7 +172,11 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
 		 */
 		err = -ENOENT;
 	}
-	return err;
+	if (err) {
+		dput(newdentry);
+		return ERR_PTR(err);
+	}
+	return newdentry;
 }
 
 static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
@@ -218,20 +237,21 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
 	struct inode *udir = upperdir->d_inode;
 	struct dentry *newdentry;
-	int err;
+	int err = 0;
 
 	if (!hardlink && !IS_POSIXACL(udir))
 		attr->mode &= ~current_umask();
 
 	inode_lock_nested(udir, I_MUTEX_PARENT);
-	newdentry = lookup_one_len(dentry->d_name.name, upperdir,
-				   dentry->d_name.len);
-	err = PTR_ERR(newdentry);
-	if (IS_ERR(newdentry))
+	newdentry = ovl_create_real(udir,
+				    lookup_one_len(dentry->d_name.name,
+						   upperdir,
+						   dentry->d_name.len),
+				    attr, hardlink, false);
+	if (IS_ERR(newdentry)) {
+		err = PTR_ERR(newdentry);
 		goto out_unlock;
-	err = ovl_create_real(udir, newdentry, attr, hardlink, false);
-	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 +259,6 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
 	}
 
 	ovl_instantiate(dentry, inode, newdentry, !!hardlink);
-	newdentry = NULL;
-out_dput:
-	dput(newdentry);
 out_unlock:
 	inode_unlock(udir);
 	return err;
@@ -280,15 +297,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);
-	err = PTR_ERR(opaquedir);
-	if (IS_ERR(opaquedir))
-		goto out_unlock;
-
-	err = ovl_create_real(wdir, opaquedir,
+	opaquedir = ovl_create_real(wdir, ovl_lookup_temp(workdir),
 			      &(struct cattr){.mode = stat.mode}, NULL, true);
-	if (err)
-		goto out_dput;
+	if (IS_ERR(opaquedir)) {
+		err = PTR_ERR(opaquedir);
+		goto out_unlock;
+	}
 
 	err = ovl_copy_xattr(upper, opaquedir);
 	if (err)
@@ -319,7 +333,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);
@@ -380,20 +393,18 @@ 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_real(wdir, ovl_lookup_temp(workdir),
+				    cattr, hardlink, true);
+
+	err = PTR_ERR(newdentry);
+	if (IS_ERR(newdentry))
+		goto out_dput;
 
 	/*
 	 * mode could have been mutilated due to umask (e.g. sgid directory)
@@ -442,9 +453,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
 	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 e0b7de799f6b..a618f530c74e 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -360,7 +360,7 @@ struct cattr {
 	umode_t mode;
 	const char *link;
 };
-int ovl_create_real(struct inode *dir, struct dentry *newdentry,
+struct dentry *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);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index e8551c97de51..c5aee822fd6a 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -611,11 +611,13 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
 			goto retry;
 		}
 
-		err = ovl_create_real(dir, work,
+		work = ovl_create_real(dir, work,
 				      &(struct cattr){.mode = S_IFDIR | 0},
 				      NULL, true);
-		if (err)
-			goto out_dput;
+		if (IS_ERR(work)) {
+			err = PTR_ERR(work);
+			goto out_err;
+		}
 
 		/*
 		 * Try to remove POSIX ACL xattrs from workdir.  We are good if:

Re: [RFC][PATCH] ovl_create_index(): vfs_mkdir() might succeed leaving dentry negative unhashed

[Amir Goldstein]

On Sat, May 12, 2018 at 4:25 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> [same story as with the previous two patches]
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8bede0742619..cdd8f8816d2a 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -373,6 +373,22 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
>         if (err)
>                 goto out;
>
> +       if (unlikely(d_unhashed(temp))) {
> +               struct dentry *d = lookup_one_len(temp->d_name.name,
> +                                                 temp->d_parent,
> +                                                 temp->d_name.len);
> +               if (IS_ERR(d)) {
> +                       err = PTR_ERR(d);
> +                       goto out;
> +               }
> +               dput(temp);
> +               temp = d;
> +               if (d_is_negative(temp)) {
> +                       err = -EIO;
> +                       goto out;
> +               }
> +       }
> +

:-/ this pattern is too nasty to be open codes twice in ovl and
once in knfsd.
For ovl, we can hoist this into the ovl_do_mkdir() helper, but
what filesystems really need is vfs_mkdir_hashed(), then knfsd
can use it as well.

Thanks,
Amir.

Re: [PATCH][RFC] ovl_create_real(): make it cope with vfs_mkdir() safely

[Amir Goldstein]

On Sat, May 12, 2018 at 4:29 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> [this one really, really needs a review by overlayfs folks;
> all I can promise is that it compiles.]
>
> 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.
>
> Make ovl_create_real() return the dentry to be used or ERR_PTR(-E...)
> in case of error; usually that'll be the dentry passed to it, but
> it might be a different one - result of lookup in case of vfs_mkdir()
> leaving looking the inode, etc. up to the next lookup to come.
>
> In that case (as well as in case of an error), original dentry is
> dropped.  That simplifies the callers.  Moreover, passing ERR_PTR()
> as dentry leads to immediate return of the same ERR_PTR().  That
> simplifies the callers even more.

So in the beginning I had mixed feelings about the internal interface
change.. it feels shady, but it does simplify the callers... but then
I realized the correct way to simplify the callers would be a helper
ovl_create_real_temp(), because in most of the call sites
ovl_lookup_temp() is the argument to ovl_create_real(), so
your patch shouldn't bother with it.

With that in mind, I think your mkdir fix should go into ovl_do_mkdir(),
hopefully, calling a vfs helper vfs_mkdir_hashed(), where all
of the above, including ovl_create_real() pass in a struct dentry **

I can prep and test the ovl patches if you like.

Thanks,
Amir.

>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index cdd8f8816d2a..e0939d69470f 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -517,22 +517,14 @@ 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_real(d_inode(c->workdir),
> +                                       ovl_lookup_temp(c->workdir),
> +                                       &cattr, NULL, true);
> +       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 839709c7803a..bc07bdac013f 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -114,13 +114,18 @@ int ovl_cleanup_and_whiteout(struct dentry *workdir, struct inode *dir,
>         goto out;
>  }
>
> -int ovl_create_real(struct inode *dir, struct dentry *newdentry,
> +struct dentry *ovl_create_real(struct inode *dir, struct dentry *newdentry,
>                     struct cattr *attr, struct dentry *hardlink, bool debug)
>  {
>         int err;
>
> -       if (newdentry->d_inode)
> -               return -ESTALE;
> +       if (IS_ERR(newdentry))
> +               return newdentry;
> +
> +       if (newdentry->d_inode) {
> +               dput(newdentry);
> +               return ERR_PTR(-ESTALE);
> +       }
>
>         if (hardlink) {
>                 err = ovl_do_link(hardlink, dir, newdentry, debug);
> @@ -132,6 +137,16 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>
>                 case S_IFDIR:
>                         err = ovl_do_mkdir(dir, newdentry, attr->mode, debug);
> +                       if (!err && unlikely(d_unhashed(newdentry))) {
> +                               struct dentry *d;
> +                               d = lookup_one_len(newdentry->d_name.name,
> +                                                  newdentry->d_parent,
> +                                                  newdentry->d_name.len);
> +                               dput(newdentry);
> +                               if (IS_ERR(d))
> +                                       return d;
> +                               newdentry = d;
> +                       }
>                         break;
>
>                 case S_IFCHR:
> @@ -157,7 +172,11 @@ int ovl_create_real(struct inode *dir, struct dentry *newdentry,
>                  */
>                 err = -ENOENT;
>         }
> -       return err;
> +       if (err) {
> +               dput(newdentry);
> +               return ERR_PTR(err);
> +       }
> +       return newdentry;
>  }
>
>  static int ovl_set_opaque_xerr(struct dentry *dentry, struct dentry *upper,
> @@ -218,20 +237,21 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>         struct dentry *upperdir = ovl_dentry_upper(dentry->d_parent);
>         struct inode *udir = upperdir->d_inode;
>         struct dentry *newdentry;
> -       int err;
> +       int err = 0;
>
>         if (!hardlink && !IS_POSIXACL(udir))
>                 attr->mode &= ~current_umask();
>
>         inode_lock_nested(udir, I_MUTEX_PARENT);
> -       newdentry = lookup_one_len(dentry->d_name.name, upperdir,
> -                                  dentry->d_name.len);
> -       err = PTR_ERR(newdentry);
> -       if (IS_ERR(newdentry))
> +       newdentry = ovl_create_real(udir,
> +                                   lookup_one_len(dentry->d_name.name,
> +                                                  upperdir,
> +                                                  dentry->d_name.len),
> +                                   attr, hardlink, false);
> +       if (IS_ERR(newdentry)) {
> +               err = PTR_ERR(newdentry);
>                 goto out_unlock;
> -       err = ovl_create_real(udir, newdentry, attr, hardlink, false);
> -       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 +259,6 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
>         }
>
>         ovl_instantiate(dentry, inode, newdentry, !!hardlink);
> -       newdentry = NULL;
> -out_dput:
> -       dput(newdentry);
>  out_unlock:
>         inode_unlock(udir);
>         return err;
> @@ -280,15 +297,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);
> -       err = PTR_ERR(opaquedir);
> -       if (IS_ERR(opaquedir))
> -               goto out_unlock;
> -
> -       err = ovl_create_real(wdir, opaquedir,
> +       opaquedir = ovl_create_real(wdir, ovl_lookup_temp(workdir),
>                               &(struct cattr){.mode = stat.mode}, NULL, true);
> -       if (err)
> -               goto out_dput;
> +       if (IS_ERR(opaquedir)) {
> +               err = PTR_ERR(opaquedir);
> +               goto out_unlock;
> +       }
>
>         err = ovl_copy_xattr(upper, opaquedir);
>         if (err)
> @@ -319,7 +333,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);
> @@ -380,20 +393,18 @@ 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_real(wdir, ovl_lookup_temp(workdir),
> +                                   cattr, hardlink, true);
> +
> +       err = PTR_ERR(newdentry);
> +       if (IS_ERR(newdentry))
> +               goto out_dput;
>
>         /*
>          * mode could have been mutilated due to umask (e.g. sgid directory)
> @@ -442,9 +453,9 @@ static int ovl_create_over_whiteout(struct dentry *dentry, struct inode *inode,
>         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 e0b7de799f6b..a618f530c74e 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -360,7 +360,7 @@ struct cattr {
>         umode_t mode;
>         const char *link;
>  };
> -int ovl_create_real(struct inode *dir, struct dentry *newdentry,
> +struct dentry *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);
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index e8551c97de51..c5aee822fd6a 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -611,11 +611,13 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>                         goto retry;
>                 }
>
> -               err = ovl_create_real(dir, work,
> +               work = ovl_create_real(dir, work,
>                                       &(struct cattr){.mode = S_IFDIR | 0},
>                                       NULL, true);
> -               if (err)
> -                       goto out_dput;
> +               if (IS_ERR(work)) {
> +                       err = PTR_ERR(work);
> +                       goto out_err;
> +               }
>
>                 /*
>                  * Try to remove POSIX ACL xattrs from workdir.  We are good if:

Re: [PATCH][RFC] ovl_create_real(): make it cope with vfs_mkdir() safely

[Amir Goldstein]

On Sat, May 12, 2018 at 8:13 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Sat, May 12, 2018 at 4:29 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>> [this one really, really needs a review by overlayfs folks;
>> all I can promise is that it compiles.]
>>
>> 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.
>>
>> Make ovl_create_real() return the dentry to be used or ERR_PTR(-E...)
>> in case of error; usually that'll be the dentry passed to it, but
>> it might be a different one - result of lookup in case of vfs_mkdir()
>> leaving looking the inode, etc. up to the next lookup to come.
>>
>> In that case (as well as in case of an error), original dentry is
>> dropped.  That simplifies the callers.  Moreover, passing ERR_PTR()
>> as dentry leads to immediate return of the same ERR_PTR().  That
>> simplifies the callers even more.
>
> So in the beginning I had mixed feelings about the internal interface
> change.. it feels shady, but it does simplify the callers... but then
> I realized the correct way to simplify the callers would be a helper
> ovl_create_real_temp(), because in most of the call sites
> ovl_lookup_temp() is the argument to ovl_create_real(), so
> your patch shouldn't bother with it.
>
> With that in mind, I think your mkdir fix should go into ovl_do_mkdir(),
> hopefully, calling a vfs helper vfs_mkdir_hashed(), where all
> of the above, including ovl_create_real() pass in a struct dentry **
>
> I can prep and test the ovl patches if you like.
>

My tested version is at:
https://github.com/amir73il/linux/commits/ovl-fixes

The branch is based on a merge of your vfs/fixes and Miklos'
vfs/overlayfs-rorw branches. It includes also a patch for overlayfs
to use d_instantiate_new().

Will post the patches to overlayfs list shortly.

Thanks,
Amir.

Re: [RFC][PATCH] ovl_create_index(): vfs_mkdir() might succeed leaving dentry negative unhashed

[Al Viro]

On Sat, May 12, 2018 at 07:49:21AM +0300, Amir Goldstein wrote:

> :-/ this pattern is too nasty to be open codes twice in ovl and
> once in knfsd.
> For ovl, we can hoist this into the ovl_do_mkdir() helper, but
> what filesystems really need is vfs_mkdir_hashed(), then knfsd
> can use it as well.

Not really - handling of that case is really different for different
callers, especially if you look into the "it's managed to disappear
in between ->mkdir and ->lookup" case.  Passing by reference seriously
limits what the callers can do; if it works for overlayfs, more power
to you, but it's not a good general-purpose helper.

Re: [RFC][PATCH] ovl_create_index(): vfs_mkdir() might succeed leaving dentry negative unhashed

[Miklos Szeredi]

On Sat, May 12, 2018 at 3:25 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> [same story as with the previous two patches]
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> ---
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8bede0742619..cdd8f8816d2a 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -373,6 +373,22 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
>         if (err)
>                 goto out;
>
> +       if (unlikely(d_unhashed(temp))) {
> +               struct dentry *d = lookup_one_len(temp->d_name.name,
> +                                                 temp->d_parent,
> +                                                 temp->d_name.len);
> +               if (IS_ERR(d)) {
> +                       err = PTR_ERR(d);
> +                       goto out;

This violates the "If -1 is returned, no directory shall be created" rule.

lookup_one_len() does various permission checks.  The normal DAC check
is not a worry, because of the lock on the parent. But is it
guaranteed that MAC allows lookup if it allowed mkdir?

Then there's still the theoretical possibility of the lookup failing
with ENOMEM,  probably not worth worrying about too much (maybe emit a
warning).

Thanks,
Miklos

> +               }
> +               dput(temp);
> +               temp = d;
> +               if (d_is_negative(temp)) {
> +                       err = -EIO;
> +                       goto out;
> +               }
> +       }
> +
>         err = ovl_set_upper_fh(upper, temp);
>         if (err)
>                 goto out_cleanup;

Re: [RFC][PATCH] ovl_create_index(): vfs_mkdir() might succeed leaving dentry negative unhashed

[Al Viro]

On Mon, May 14, 2018 at 05:04:44PM +0200, Miklos Szeredi wrote:
> On Sat, May 12, 2018 at 3:25 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > [same story as with the previous two patches]
> >
> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> > ---
> > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > index 8bede0742619..cdd8f8816d2a 100644
> > --- a/fs/overlayfs/copy_up.c
> > +++ b/fs/overlayfs/copy_up.c
> > @@ -373,6 +373,22 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
> >         if (err)
> >                 goto out;
> >
> > +       if (unlikely(d_unhashed(temp))) {
> > +               struct dentry *d = lookup_one_len(temp->d_name.name,
> > +                                                 temp->d_parent,
> > +                                                 temp->d_name.len);
> > +               if (IS_ERR(d)) {
> > +                       err = PTR_ERR(d);
> > +                       goto out;
> 
> This violates the "If -1 is returned, no directory shall be created" rule.

So does NFS, for that matter...  If you use weird filesystems as layers,
you get corner cases.  Note that on NFS we'll probably have to go for
that possibility (== argument unhashed negative after success) on mkdir,
in case when it's reexported and somebody is playing silly buggers with
races.

> lookup_one_len() does various permission checks.  The normal DAC check
> is not a worry, because of the lock on the parent. But is it
> guaranteed that MAC allows lookup if it allowed mkdir?

*shrug*

Or it can be set so that rmdir won't be allowed afterwards, or...
There are all kinds of ways to set idiotic LSM that would screw overlayfs.
Doctor, it hurts when I do it...  But then you know what I think of
Linux S&M community in general...

Right now you code would fail with -EIO in all those cases, AFAICS.  So you
might get a stray -EPERM...