ovl_create_index(): vfs_mkdir() might succeed leaving dentry negative unhashed

[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 <[email protected]>

fs/overlayfs/copy_up.c +7 -15

index cdd8f8816d2a..e0939d69470f 100644
@@ -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:

fs/overlayfs/dir.c +46 -35

index 839709c7803a..bc07bdac013f 100644
@@ -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:

fs/overlayfs/overlayfs.h +1 -1

index e0b7de799f6b..a618f530c74e 100644
@@ -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);

fs/overlayfs/super.c +5 -3

index e8551c97de51..c5aee822fd6a 100644
@@ -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:

Quoted 12 lines · 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.

Quoted 4 lines

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.

[same story as with the previous two patches]

Signed-off-by Al Viro <[email protected]>

---

fs/overlayfs/copy_up.c +6 -0

index 8bede0742619..cdd8f8816d2a 100644
@@ -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);

:-/ 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.

[same story as with the previous two patches]

Signed-off-by Al Viro <[email protected]>

---

fs/overlayfs/copy_up.c +0 -0

index 8bede0742619..cdd8f8816d2a 100644

fs/overlayfs/copy_up.c +9 -0

+               }
+               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;

Quoted 12 lines · Al Viro

[same story as with the previous two patches]

Signed-off-by Al Viro <[email protected]>

---

fs/overlayfs/copy_up.c +0 -0

index 8bede0742619..cdd8f8816d2a 100644
@@ -373,6 +373,22 @@ static int ovl_create_index(struct dentry *dentry, struct dentry *origin,
        if (err)
                goto out;
 

fs/overlayfs/copy_up.c static int ovl_create_index(struct dentry *dentry, struct dentry *origin, · line 382

Quoted 7 lines

fs/overlayfs/copy_up.c +7 -0

+       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?