Re: [PATCH v5 13/14] VFS: change vfs_mkdir() to unlock on failure.

[Jeff Layton <jlayton@kernel.org>]

On Thu, 2025-11-06 at 11:50 +1100, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> 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.
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: NeilBrown <neil@brown.name>
> 
> --
> changes since v2:
>  - extra {} in if() branch in cachefiles_get_directory() to
>    match the new extra {} in the else branch.
>  - filesystems/porting.rst updated.
> ---
>  Documentation/filesystems/porting.rst | 13 +++++++++++++
>  fs/btrfs/ioctl.c                      |  2 +-
>  fs/cachefiles/namei.c                 | 16 ++++++++-------
>  fs/ecryptfs/inode.c                   |  8 ++++----
>  fs/namei.c                            |  4 ++--
>  fs/nfsd/nfs3proc.c                    |  2 +-
>  fs/nfsd/nfs4proc.c                    |  2 +-
>  fs/nfsd/nfs4recover.c                 |  2 +-
>  fs/nfsd/nfsproc.c                     |  2 +-
>  fs/nfsd/vfs.c                         |  8 ++++----
>  fs/overlayfs/copy_up.c                |  4 ++--
>  fs/overlayfs/dir.c                    | 13 ++++++-------
>  fs/overlayfs/super.c                  |  6 +++---
>  fs/xfs/scrub/orphanage.c              |  2 +-
>  include/linux/namei.h                 | 28 +++++++++------------------
>  ipc/mqueue.c                          |  2 +-
>  16 files changed, 59 insertions(+), 55 deletions(-)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index 35f027981b21..d33429294252 100644
> --- 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 d0c3bb0423bb..b138120feba3 100644
> --- 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 0104ac00485d..59327618ac42 100644
> --- a/fs/cachefiles/namei.c
> +++ b/fs/cachefiles/namei.c
> @@ -128,10 +128,12 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
>  		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 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
>  		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 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
>  	/* 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 @@ struct dentry *cachefiles_get_directory(struct cachefiles_cache *cache,
>  	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 37d6293600c7..c951e723f24d 100644
> --- 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 2444c7ddb926..e834486acff1 100644
> --- 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 e2aac0def2cb..6b39e4aff959 100644
> --- a/fs/nfsd/nfs3proc.c
> +++ b/fs/nfsd/nfs3proc.c
> @@ -364,7 +364,7 @@ nfsd3_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	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 b2c95e8e7c68..524cb07a477c 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -376,7 +376,7 @@ nfsd4_create_file(struct svc_rqst *rqstp, struct svc_fh *fhp,
>  	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 3eefaa2202e3..18c08395b273 100644
> --- 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 ee1b16e921fd..28f03a6a3cc3 100644
> --- 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 a993f1e54182..145f1c8d124d 100644
> --- 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 9911a346b477..23216ed01325 100644
> --- 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 61e9484e4ab8..a4a0dc261310 100644
> --- 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;;
>  
> @@ -254,7 +254,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 +362,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 a721ef2b90e8..3acda985c8a3 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -320,7 +320,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>  
>  		if (work->d_inode) {
>  			dget(work);
> -			end_creating(work, ofs->workbasedir);
> +			end_creating(work);
>  			if (persist)
>  				return work;
>  			err = -EEXIST;
> @@ -338,7 +338,7 @@ static struct dentry *ovl_workdir_create(struct ovl_fs *ofs,
>  		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 e732605924a1..b77c2b6b6d44 100644
> --- 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 0e6b1b9afc26..b4d95b79b5a8 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -106,34 +106,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 6d7610310003..83d9466710d6 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -932,7 +932,7 @@ static int do_mq_open(const char __user *u_name, int oflag, umode_t mode,
>  		put_unused_fd(fd);
>  		fd = error;
>  	}
> -	end_creating(path.dentry, root);
> +	end_creating(path.dentry);
>  	if (!ro)
>  		mnt_drop_write(mnt);
>  out_putname:

Reviewed-by: Jeff Layton <jlayton@kernel.org>