VFS: change vfs_mkdir() to unlock on failure.

vfs_mkdir() already drops the reference to the dentry on failure but it
leaves the parent locked.
This complicates end_creating() which needs to unlock the parent even
though the dentry is no longer available.

If we change vfs_mkdir() to unlock on failure as well as releasing the
dentry, we can remove the "parent" arg from end_creating() and simplify
the rules for calling it.

Note that cachefiles_get_directory() can choose to substitute an error
instead of actually calling vfs_mkdir(), for fault injection.  In that
case it needs to call end_creating(), just as vfs_mkdir() now does on
error.

ovl_create_real() will now unlock on error.  So the conditional
end_creating() after the call is removed, and end_creating() is called
internally on error.

Reviewed-by: Amir Goldstein <amir73il@gmail.com>
Reviewed-by: Jeff Layton <jlayton@kernel.org>
Tested-by: syzbot@syzkaller.appspotmail.com
Signed-off-by: NeilBrown <neil@brown.name>
Link: https://patch.msgid.link/20251113002050.676694-15-neilb@ownmail.net
Signed-off-by: Christian Brauner <brauner@kernel.org>

diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index 7233b04668fcce75f1ed170329a2cd18110a7d89..76ff738a00f3b434a976bf4cbcd383db0b28f461 100644 (file)
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1309,3 +1309,16 @@ a different length, use
        vfs_parse_fs_qstr(fc, key, &QSTR_LEN(value, len))
 
 instead.
+
+---
+
+**mandatory**
+
+vfs_mkdir() now returns a dentry - the one returned by ->mkdir().  If
+that dentry is different from the dentry passed in, including if it is
+an IS_ERR() dentry pointer, the original dentry is dput().
+
+When vfs_mkdir() returns an error, and so both dputs() the original
+dentry and doesn't provide a replacement, it also unlocks the parent.
+Consequently the return value from vfs_mkdir() can be passed to
+end_creating() and the parent will be unlocked precisely when necessary.

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 4fbfdd8faf6a72edd8627a0cb717d433fad5f7a2..90ef777eae25cc12cfeb70b5a3f9c5682c574af8 100644 (file)
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -935,7 +935,7 @@ static noinline int btrfs_mksubvol(struct dentry *parent,
 out_up_read:
        up_read(&fs_info->subvol_sem);
 out_dput:
-       end_creating(dentry, parent);
+       end_creating(dentry);
        return ret;
 }
 

diff --git a/fs/cachefiles/namei.c b/fs/cachefiles/namei.c
index 0104ac00485d6e4f25458ea85c6b94170dd251c6..59327618ac429e657f4c6eb78ce7383272305ce2 100644 (file)
--- a/fs/cachefiles/namei.c
+++ b/fs/cachefiles/namei.c
@@ -128,10 +128,12 @@ retry:
                if (ret < 0)
                        goto mkdir_error;
                ret = cachefiles_inject_write_error();
-               if (ret == 0)
+               if (ret == 0) {
                        subdir = vfs_mkdir(&nop_mnt_idmap, d_inode(dir), subdir, 0700);
-               else
+               } else {
+                       end_creating(subdir);
                        subdir = ERR_PTR(ret);
+               }
                if (IS_ERR(subdir)) {
                        trace_cachefiles_vfs_error(NULL, d_inode(dir), ret,
                                                   cachefiles_trace_mkdir_error);
@@ -140,7 +142,7 @@ retry:
                trace_cachefiles_mkdir(dir, subdir);
 
                if (unlikely(d_unhashed(subdir) || d_is_negative(subdir))) {
-                       end_creating(subdir, dir);
+                       end_creating(subdir);
                        goto retry;
                }
                ASSERT(d_backing_inode(subdir));
@@ -154,7 +156,7 @@ retry:
        /* Tell rmdir() it's not allowed to delete the subdir */
        inode_lock(d_inode(subdir));
        dget(subdir);
-       end_creating(subdir, dir);
+       end_creating(subdir);
 
        if (!__cachefiles_mark_inode_in_use(NULL, d_inode(subdir))) {
                pr_notice("cachefiles: Inode already in use: %pd (B=%lx)\n",
@@ -196,7 +198,7 @@ mark_error:
        return ERR_PTR(-EBUSY);
 
 mkdir_error:
-       end_creating(subdir, dir);
+       end_creating(subdir);
        pr_err("mkdir %s failed with error %d\n", dirname, ret);
        return ERR_PTR(ret);
 
@@ -699,7 +701,7 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
                if (ret < 0)
                        goto out_end;
 
-               end_creating(dentry, fan);
+               end_creating(dentry);
 
                ret = cachefiles_inject_read_error();
                if (ret == 0)
@@ -733,7 +735,7 @@ bool cachefiles_commit_tmpfile(struct cachefiles_cache *cache,
        }
 
 out_end:
-       end_creating(dentry, fan);
+       end_creating(dentry);
 out:
        _leave(" = %u", success);
        return success;

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 6a5bca89e752ed6eeb1cb64c7bd97b4bc0d93323..2ad1db2cd2ece9147a98d2663b24cbbce1af6a3d 100644 (file)
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -211,7 +211,7 @@ ecryptfs_do_create(struct inode *directory_inode,
        fsstack_copy_attr_times(directory_inode, lower_dir);
        fsstack_copy_inode_size(directory_inode, lower_dir);
 out_lock:
-       end_creating(lower_dentry, NULL);
+       end_creating(lower_dentry);
        return inode;
 }
 
@@ -456,7 +456,7 @@ static int ecryptfs_link(struct dentry *old_dentry, struct inode *dir,
                  ecryptfs_inode_to_lower(d_inode(old_dentry))->i_nlink);
        i_size_write(d_inode(new_dentry), file_size_save);
 out_lock:
-       end_creating(lower_new_dentry, NULL);
+       end_creating(lower_new_dentry);
        return rc;
 }
 
@@ -500,7 +500,7 @@ static int ecryptfs_symlink(struct mnt_idmap *idmap,
        fsstack_copy_attr_times(dir, lower_dir);
        fsstack_copy_inode_size(dir, lower_dir);
 out_lock:
-       end_creating(lower_dentry, NULL);
+       end_creating(lower_dentry);
        if (d_really_is_negative(dentry))
                d_drop(dentry);
        return rc;
@@ -534,7 +534,7 @@ static struct dentry *ecryptfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
        fsstack_copy_inode_size(dir, lower_dir);
        set_nlink(dir, lower_dir->i_nlink);
 out:
-       end_creating(lower_dentry, lower_dir_dentry);
+       end_creating(lower_dentry);
        if (d_really_is_negative(dentry))
                d_drop(dentry);
        return ERR_PTR(rc);

diff --git a/fs/namei.c b/fs/namei.c
index 8b7807cd1343d30250adf8bb953177749b1da6fa..d284ebae41bf116cd62c769f508e9519bbcc1908 100644 (file)
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4832,7 +4832,7 @@ EXPORT_SYMBOL(start_creating_path);
  */
 void end_creating_path(const struct path *path, struct dentry *dentry)
 {
-       end_creating(dentry, path->dentry);
+       end_creating(dentry);
        mnt_drop_write(path->mnt);
        path_put(path);
 }
@@ -5034,7 +5034,7 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
        return dentry;
 
 err:
-       dput(dentry);
+       end_creating(dentry);
        return ERR_PTR(error);
 }
 EXPORT_SYMBOL(vfs_mkdir);

diff --git a/fs/nfsd/nfs3proc.c b/fs/nfsd/nfs3proc.c
index e2aac0def2cb253c10d0dd914e3b68e3f742fa05..6b39e4aff95903e8b454d0bdaaad2a878a4f9eac 100644 (file)
--- a/fs/nfsd/nfs3proc.c
+++ b/fs/nfsd/nfs3proc.c
@@ -364,7 +364,7 @@ set_attr:
        status = nfsd_create_setattr(rqstp, fhp, resfhp, &attrs);
 
 out:
-       end_creating(child, parent);
+       end_creating(child);
 out_write:
        fh_drop_write(fhp);
        return status;

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index b2c95e8e7c683d6886f505e499b6ef461e34bc7e..524cb07a477c252f1558e0275d6c03856f405e63 100644 (file)
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -376,7 +376,7 @@ set_attr:
        if (attrs.na_aclerr)
                open->op_bmval[0] &= ~FATTR4_WORD0_ACL;
 out:
-       end_creating(child, parent);
+       end_creating(child);
        nfsd_attrs_free(&attrs);
 out_write:
        fh_drop_write(fhp);

diff --git a/fs/nfsd/nfs4recover.c b/fs/nfsd/nfs4recover.c
index 3eefaa2202e38fe3026cc259de852896bfedaabd..18c08395b273308f79e3b83deeb021a2b1b90244 100644 (file)
--- a/fs/nfsd/nfs4recover.c
+++ b/fs/nfsd/nfs4recover.c
@@ -215,7 +215,7 @@ nfsd4_create_clid_dir(struct nfs4_client *clp)
        if (IS_ERR(dentry))
                status = PTR_ERR(dentry);
 out_end:
-       end_creating(dentry, dir);
+       end_creating(dentry);
 out:
        if (status == 0) {
                if (nn->in_grace)

diff --git a/fs/nfsd/nfsproc.c b/fs/nfsd/nfsproc.c
index ee1b16e921fdcdd194210894b838e9cecfa4ca0c..28f03a6a3cc38249f45a397ba4b19cc36db53eef 100644 (file)
--- a/fs/nfsd/nfsproc.c
+++ b/fs/nfsd/nfsproc.c
@@ -421,7 +421,7 @@ nfsd_proc_create(struct svc_rqst *rqstp)
        }
 
 out_unlock:
-       end_creating(dchild, dirfhp->fh_dentry);
+       end_creating(dchild);
 out_write:
        fh_drop_write(dirfhp);
 done:

diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 62109885d4dbcd4050e04f34387b0b642ad4e72e..6e9a57863904cbcaf684af3c5ab3cf32cfcba35b 100644 (file)
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -1589,7 +1589,7 @@ nfsd_create_locked(struct svc_rqst *rqstp, struct svc_fh *fhp,
 out:
        if (!err)
                fh_fill_post_attrs(fhp);
-       end_creating(dchild, dentry);
+       end_creating(dchild);
        return err;
 
 out_nfserr:
@@ -1646,7 +1646,7 @@ nfsd_create(struct svc_rqst *rqstp, struct svc_fh *fhp,
        return err;
 
 out_unlock:
-       end_creating(dchild, dentry);
+       end_creating(dchild);
        return err;
 }
 
@@ -1747,7 +1747,7 @@ nfsd_symlink(struct svc_rqst *rqstp, struct svc_fh *fhp,
                nfsd_create_setattr(rqstp, fhp, resfhp, attrs);
        fh_fill_post_attrs(fhp);
 out_unlock:
-       end_creating(dnew, dentry);
+       end_creating(dnew);
        if (!err)
                err = nfserrno(commit_metadata(fhp));
        if (!err)
@@ -1824,7 +1824,7 @@ nfsd_link(struct svc_rqst *rqstp, struct svc_fh *ffhp,
        host_err = vfs_link(dold, &nop_mnt_idmap, dirp, dnew, NULL);
        fh_fill_post_attrs(ffhp);
 out_unlock:
-       end_creating(dnew, ddir);
+       end_creating(dnew);
        if (!host_err) {
                host_err = commit_metadata(ffhp);
                if (!host_err)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 27014ada11c73180107bf7f42f1d687dfab5d967..36949856ddeac793c27ca83654cfdc359984054a 100644 (file)
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -624,7 +624,7 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
                        ovl_dentry_set_upper_alias(c->dentry);
                        ovl_dentry_update_reval(c->dentry, upper);
                }
-               end_creating(upper, upperdir);
+               end_creating(upper);
        }
        if (err)
                goto out;
@@ -891,7 +891,7 @@ static int ovl_copy_up_tmpfile(struct ovl_copy_up_ctx *c)
        err = PTR_ERR(upper);
        if (!IS_ERR(upper)) {
                err = ovl_do_link(ofs, temp, udir, upper);
-               end_creating(upper, c->destdir);
+               end_creating(upper);
        }
 
        if (err)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index b7f443932d933434e0f927f8ab21222572b6d9a1..e097ef4e79d2c0b3e79e66ca8a933e4a77269031 100644 (file)
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -91,7 +91,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
                err = ovl_do_whiteout(ofs, wdir, whiteout);
                if (!err)
                        ofs->whiteout = dget(whiteout);
-               end_creating(whiteout, workdir);
+               end_creating(whiteout);
                if (err)
                        return ERR_PTR(err);
        }
@@ -103,7 +103,7 @@ static struct dentry *ovl_whiteout(struct ovl_fs *ofs)
                err = ovl_do_link(ofs, ofs->whiteout, wdir, link);
                if (!err)
                        whiteout = dget(link);
-               end_creating(link, workdir);
+               end_creating(link);
                if (!err)
                        return whiteout;;
 
@@ -187,7 +187,7 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
                        if (!err && ofs->casefold != ovl_dentry_casefolded(newdentry)) {
                                pr_warn_ratelimited("wrong inherited casefold (%pd2)\n",
                                                    newdentry);
-                               dput(newdentry);
+                               end_creating(newdentry);
                                err = -EINVAL;
                        }
                        break;
@@ -237,8 +237,7 @@ struct dentry *ovl_create_real(struct ovl_fs *ofs, struct dentry *parent,
        }
 out:
        if (err) {
-               if (!IS_ERR(newdentry))
-                       dput(newdentry);
+               end_creating(newdentry);
                return ERR_PTR(err);
        }
        return newdentry;
@@ -254,7 +253,7 @@ struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
        ret = ovl_create_real(ofs, workdir, ret, attr);
        if (!IS_ERR(ret))
                dget(ret);
-       end_creating(ret, workdir);
+       end_creating(ret);
        return ret;
 }
 
@@ -362,12 +361,11 @@ static int ovl_create_upper(struct dentry *dentry, struct inode *inode,
        if (IS_ERR(newdentry))
                return PTR_ERR(newdentry);
        newdentry = ovl_create_real(ofs, upperdir, newdentry, attr);
-       if (IS_ERR(newdentry)) {
-               end_creating(newdentry, upperdir);
+       if (IS_ERR(newdentry))
                return PTR_ERR(newdentry);
-       }
+
        dget(newdentry);
-       end_creating(newdentry, upperdir);
+       end_creating(newdentry);
 
        if (ovl_type_merge(dentry->d_parent) && d_is_dir(newdentry) &&
            !ovl_allow_offline_changes(ofs)) {

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index a721ef2b90e8c056b24ba518ce0d514de4d6d3cc..3acda985c8a31f462cf06fcacf31dd78e61281a9 100644 (file)
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -320,7 +320,7 @@ retry:
 
                if (work->d_inode) {
                        dget(work);
-                       end_creating(work, ofs->workbasedir);
+                       end_creating(work);
                        if (persist)
                                return work;
                        err = -EEXIST;
@@ -338,7 +338,7 @@ retry:
                work = ovl_do_mkdir(ofs, dir, work, attr.ia_mode);
                if (!IS_ERR(work))
                        dget(work);
-               end_creating(work, ofs->workbasedir);
+               end_creating(work);
                err = PTR_ERR(work);
                if (IS_ERR(work))
                        goto out_err;
@@ -632,7 +632,7 @@ static struct dentry *ovl_lookup_or_create(struct ovl_fs *ofs,
                                                OVL_CATTR(mode));
                if (!IS_ERR(child))
                        dget(child);
-               end_creating(child, parent);
+               end_creating(child);
        }
        dput(parent);
 

diff --git a/fs/xfs/scrub/orphanage.c b/fs/xfs/scrub/orphanage.c
index e732605924a15a0346122ba7f7b367d14a62bf27..b77c2b6b6d44dc11b9c38f7e431e9a0e93f27f75 100644 (file)
--- a/fs/xfs/scrub/orphanage.c
+++ b/fs/xfs/scrub/orphanage.c
@@ -199,7 +199,7 @@ xrep_orphanage_create(
        sc->orphanage_ilock_flags = 0;
 
 out_dput_orphanage:
-       end_creating(orphanage_dentry, root_dentry);
+       end_creating(orphanage_dentry);
 out_dput_root:
        dput(root_dentry);
 out:

diff --git a/include/linux/namei.h b/include/linux/namei.h
index 208aed1d67283331fcf973eccba83607ed3a7237..0ef73d739a310d41183d634d43d68e9b9bf3d8db 100644 (file)
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -105,34 +105,24 @@ struct dentry *start_creating_dentry(struct dentry *parent,
 struct dentry *start_removing_dentry(struct dentry *parent,
                                     struct dentry *child);
 
-/**
- * end_creating - finish action started with start_creating
- * @child:  dentry returned by start_creating() or vfs_mkdir()
- * @parent: dentry given to start_creating(),
- *
- * Unlock and release the child.
+/* end_creating - finish action started with start_creating
+ * @child: dentry returned by start_creating() or vfs_mkdir()
  *
- * Unlike end_dirop() this can only be called if start_creating() succeeded.
- * It handles @child being and error as vfs_mkdir() might have converted the
- * dentry to an error - in that case the parent still needs to be unlocked.
+ * Unlock and release the child. This can be called after
+ * start_creating() whether that function succeeded or not,
+ * but it is not needed on failure.
  *
  * If vfs_mkdir() was called then the value returned from that function
  * should be given for @child rather than the original dentry, as vfs_mkdir()
- * may have provided a new dentry.  Even if vfs_mkdir() returns an error
- * it must be given to end_creating().
+ * may have provided a new dentry.
+ *
  *
  * If vfs_mkdir() was not called, then @child will be a valid dentry and
  * @parent will be ignored.
  */
-static inline void end_creating(struct dentry *child, struct dentry *parent)
+static inline void end_creating(struct dentry *child)
 {
-       if (IS_ERR(child))
-               /* The parent is still locked despite the error from
-                * vfs_mkdir() - must unlock it.
-                */
-               inode_unlock(parent->d_inode);
-       else
-               end_dirop(child);
+       end_dirop(child);
 }
 
 /**

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 6d76103100030071fd26c08b7eaeda0290174eb5..83d9466710d6931e116b7475b0b6fb54ba618d79 100644 (file)
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -932,7 +932,7 @@ out_putfd:
                put_unused_fd(fd);
                fd = error;
        }
-       end_creating(path.dentry, root);
+       end_creating(path.dentry);
        if (!ro)
                mnt_drop_write(mnt);
 out_putname: