Re: [PATCH 05/11] VFS: introduce start_creating_noperm() and start_removing_noperm()

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2025-09-26 at 12:49 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> xfs, fuse, ipc/mqueue need variants of start_creating or start_removing
> which do not check permissions.
> This patch adds _noperm versions of these functions.
> 
> Note that do_mq_open() was only calling mntget() so it could call
> path_put() - it didn't really need an extra reference on the mnt.
> Now it doesn't call mntget() and uses end_creating() which does
> the dput() half of path_put().
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  fs/fuse/dir.c            | 19 +++++++---------
>  fs/namei.c               | 48 ++++++++++++++++++++++++++++++++++++++++
>  fs/xfs/scrub/orphanage.c | 11 ++++-----
>  include/linux/namei.h    |  2 ++
>  ipc/mqueue.c             | 31 +++++++++-----------------
>  5 files changed, 73 insertions(+), 38 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 5c569c3cb53f..88bc512639e2 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1404,27 +1404,25 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
>  	if (!parent)
>  		return -ENOENT;
>  
> -	inode_lock_nested(parent, I_MUTEX_PARENT);
>  	if (!S_ISDIR(parent->i_mode))
> -		goto unlock;
> +		goto put_parent;
>  
>  	err = -ENOENT;
>  	dir = d_find_alias(parent);
>  	if (!dir)
> -		goto unlock;
> +		goto put_parent;
>  
> -	name->hash = full_name_hash(dir, name->name, name->len);
> -	entry = d_lookup(dir, name);
> +	entry = start_removing_noperm(dir, name);
>  	dput(dir);
> -	if (!entry)
> -		goto unlock;
> +	if (IS_ERR(entry))
> +		goto put_parent;
>  
>  	fuse_dir_changed(parent);
>  	if (!(flags & FUSE_EXPIRE_ONLY))
>  		d_invalidate(entry);
>  	fuse_invalidate_entry_cache(entry);
>  
> -	if (child_nodeid != 0 && d_really_is_positive(entry)) {
> +	if (child_nodeid != 0) {
>  		inode_lock(d_inode(entry));
>  		if (get_node_id(d_inode(entry)) != child_nodeid) {
>  			err = -ENOENT;
> @@ -1452,10 +1450,9 @@ int fuse_reverse_inval_entry(struct fuse_conn *fc, u64 parent_nodeid,
>  	} else {
>  		err = 0;
>  	}
> -	dput(entry);
>  
> - unlock:
> -	inode_unlock(parent);
> +	end_removing(entry);
> + put_parent:
>  	iput(parent);
>  	return err;
>  }
> diff --git a/fs/namei.c b/fs/namei.c
> index 0d9e98961758..bd5c45801756 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -3296,6 +3296,54 @@ struct dentry *start_removing(struct mnt_idmap *idmap, struct dentry *parent,
>  }
>  EXPORT_SYMBOL(start_removing);
>  
> +/**
> + * start_creating_noperm - prepare to create a given name without permission checking
> + * @parent - directory in which to prepare to create the name
> + * @name - the name to be created
> + *
> + * Locks are taken and a lookup in performed prior to creating
> + * an object in a directory.
> + *
> + * If the name already exists, a positive dentry is returned.
> + *
> + * Returns: a negative or positive dentry, or an error.
> + */
> +struct dentry *start_creating_noperm(struct dentry *parent,
> +				     struct qstr *name)
> +{
> +	int err = lookup_noperm_common(name, parent);
> +
> +	if (err)
> +		return ERR_PTR(err);
> +	return start_dirop(parent, name, LOOKUP_CREATE);
> +}
> +EXPORT_SYMBOL(start_creating_noperm);
> +
> +/**
> + * start_removing_noperm - prepare to remove a given name without permission checking
> + * @parent - directory in which to find the name
> + * @name - the name to be removed
> + *
> + * Locks are taken and a lookup in performed prior to removing
> + * an object from a directory.
> + *
> + * If the name doesn't exist, an error is returned.
> + *
> + * end_removing() should be called when removal is complete, or aborted.
> + *
> + * Returns: a positive dentry, or an error.
> + */
> +struct dentry *start_removing_noperm(struct dentry *parent,
> +				     struct qstr *name)
> +{
> +	int err = lookup_noperm_common(name, parent);
> +
> +	if (err)
> +		return ERR_PTR(err);
> +	return start_dirop(parent, name, 0);
> +}
> +EXPORT_SYMBOL(start_removing_noperm);
> +
>  #ifdef CONFIG_UNIX98_PTYS
>  int path_pts(struct path *path)
>  {
> diff --git a/fs/xfs/scrub/orphanage.c b/fs/xfs/scrub/orphanage.c
> index 9c12cb844231..e732605924a1 100644
> --- a/fs/xfs/scrub/orphanage.c
> +++ b/fs/xfs/scrub/orphanage.c
> @@ -152,11 +152,10 @@ xrep_orphanage_create(
>  	}
>  
>  	/* Try to find the orphanage directory. */
> -	inode_lock_nested(root_inode, I_MUTEX_PARENT);
> -	orphanage_dentry = lookup_noperm(&QSTR(ORPHANAGE), root_dentry);
> +	orphanage_dentry = start_creating_noperm(root_dentry, &QSTR(ORPHANAGE));
>  	if (IS_ERR(orphanage_dentry)) {
>  		error = PTR_ERR(orphanage_dentry);
> -		goto out_unlock_root;
> +		goto out_dput_root;
>  	}
>  
>  	/*
> @@ -170,7 +169,7 @@ xrep_orphanage_create(
>  					     orphanage_dentry, 0750);
>  		error = PTR_ERR(orphanage_dentry);
>  		if (IS_ERR(orphanage_dentry))
> -			goto out_unlock_root;
> +			goto out_dput_orphanage;
>  	}
>  
>  	/* Not a directory? Bail out. */
> @@ -200,9 +199,7 @@ xrep_orphanage_create(
>  	sc->orphanage_ilock_flags = 0;
>  
>  out_dput_orphanage:
> -	dput(orphanage_dentry);
> -out_unlock_root:
> -	inode_unlock(VFS_I(sc->mp->m_rootip));
> +	end_creating(orphanage_dentry, root_dentry);
>  out_dput_root:
>  	dput(root_dentry);
>  out:
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 63941fdbc23d..20a88a46fe92 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -92,6 +92,8 @@ struct dentry *start_creating(struct mnt_idmap *idmap, struct dentry *parent,
>  			      struct qstr *name);
>  struct dentry *start_removing(struct mnt_idmap *idmap, struct dentry *parent,
>  			      struct qstr *name);
> +struct dentry *start_creating_noperm(struct dentry *parent, struct qstr *name);
> +struct dentry *start_removing_noperm(struct dentry *parent, struct qstr *name);
>  
>  /* end_creating - finish action started with start_creating
>   * @child - dentry returned by start_creating()
> diff --git a/ipc/mqueue.c b/ipc/mqueue.c
> index 093551fe66a7..060e8e9c4f59 100644
> --- a/ipc/mqueue.c
> +++ b/ipc/mqueue.c
> @@ -913,13 +913,11 @@ static int do_mq_open(const char __user *u_name, int oflag, umode_t mode,
>  		goto out_putname;
>  
>  	ro = mnt_want_write(mnt);	/* we'll drop it in any case */
> -	inode_lock(d_inode(root));
> -	path.dentry = lookup_noperm(&QSTR(name->name), root);
> +	path.dentry = start_creating_noperm(root, &QSTR(name->name));
>  	if (IS_ERR(path.dentry)) {
>  		error = PTR_ERR(path.dentry);
>  		goto out_putfd;
>  	}
> -	path.mnt = mntget(mnt);
>  	error = prepare_open(path.dentry, oflag, ro, mode, name, attr);
>  	if (!error) {
>  		struct file *file = dentry_open(&path, oflag, current_cred());
> @@ -928,13 +926,12 @@ static int do_mq_open(const char __user *u_name, int oflag, umode_t mode,
>  		else
>  			error = PTR_ERR(file);
>  	}
> -	path_put(&path);
>  out_putfd:
>  	if (error) {
>  		put_unused_fd(fd);
>  		fd = error;
>  	}
> -	inode_unlock(d_inode(root));
> +	end_creating(path.dentry, root);
>  	if (!ro)
>  		mnt_drop_write(mnt);
>  out_putname:
> @@ -957,7 +954,7 @@ SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)
>  	int err;
>  	struct filename *name;
>  	struct dentry *dentry;
> -	struct inode *inode = NULL;
> +	struct inode *inode;
>  	struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;
>  	struct vfsmount *mnt = ipc_ns->mq_mnt;
>  
> @@ -969,26 +966,20 @@ SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)
>  	err = mnt_want_write(mnt);
>  	if (err)
>  		goto out_name;
> -	inode_lock_nested(d_inode(mnt->mnt_root), I_MUTEX_PARENT);
> -	dentry = lookup_noperm(&QSTR(name->name), mnt->mnt_root);
> +	dentry = start_removing_noperm(mnt->mnt_root, &QSTR(name->name));
>  	if (IS_ERR(dentry)) {
>  		err = PTR_ERR(dentry);
> -		goto out_unlock;
> +		goto out_drop_write;
>  	}
>  
>  	inode = d_inode(dentry);
> -	if (!inode) {
> -		err = -ENOENT;
> -	} else {
> -		ihold(inode);
> -		err = vfs_unlink(&nop_mnt_idmap, d_inode(dentry->d_parent),
> -				 dentry, NULL);
> -	}
> -	dput(dentry);
> -
> -out_unlock:
> -	inode_unlock(d_inode(mnt->mnt_root));
> +	ihold(inode);
> +	err = vfs_unlink(&nop_mnt_idmap, d_inode(dentry->d_parent),
> +			 dentry, NULL);
> +	end_removing(dentry);
>  	iput(inode);
> +
> +out_drop_write:
>  	mnt_drop_write(mnt);
>  out_name:
>  	putname(name);

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