Re: [PATCH 08/19] VFS: introduce lookup_and_lock() and friends

[Christian Brauner <brauner@kernel.org>]

On Thu, Feb 06, 2025 at 04:42:45PM +1100, NeilBrown wrote:
> lookup_and_lock() combines locking the directory and performing a lookup
> prior to a change to the directory.
> Abstracting this prepares for changing the locking requirements.
> 
> done_lookup_and_lock() provides the inverse of putting the dentry and
> unlocking.
> 
> For "silly_rename" we will need to lookup_and_lock() in a directory that
> is already locked.  For this purpose we add LOOKUP_PARENT_LOCKED.
> 
> Like lookup_len_qstr(), lookup_and_lock() returns -ENOENT if
> LOOKUP_CREATE was NOT given and the name cannot be found,, and returns
> -EEXIST if LOOKUP_EXCL WAS given and the name CAN be found.
> 
> These functions replace all uses of lookup_one_qstr() in namei.c
> except for those used for rename.
> 
> The name might seem backwards as the lock happens before the lookup.
> A future patch will change this so that only a shared lock is taken
> before the lookup, and an exclusive lock on the dentry is taken after a
> successful lookup.  So the order "lookup" then "lock" will make sense.
> 
> This functionality is exported as lookup_and_lock_one() which takes a
> name and len rather than a qstr.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/namei.c            | 102 ++++++++++++++++++++++++++++--------------
>  include/linux/namei.h |  15 ++++++-
>  2 files changed, 83 insertions(+), 34 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 69610047f6c6..3c0feca081a2 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1715,6 +1715,41 @@ struct dentry *lookup_one_qstr(const struct qstr *name,
>  }
>  EXPORT_SYMBOL(lookup_one_qstr);
>  
> +static struct dentry *lookup_and_lock_nested(const struct qstr *last,
> +					     struct dentry *base,
> +					     unsigned int lookup_flags,
> +					     unsigned int subclass)
> +{
> +	struct dentry *dentry;
> +
> +	if (!(lookup_flags & LOOKUP_PARENT_LOCKED))
> +		inode_lock_nested(base->d_inode, subclass);
> +
> +	dentry = lookup_one_qstr(last, base, lookup_flags);
> +	if (IS_ERR(dentry) && !(lookup_flags & LOOKUP_PARENT_LOCKED)) {
> +			inode_unlock(base->d_inode);

Nit: The indentation here is wrong and the {} aren't common practice.

> +	}
> +	return dentry;
> +}
> +
> +static struct dentry *lookup_and_lock(const struct qstr *last,
> +				      struct dentry *base,
> +				      unsigned int lookup_flags)
> +{
> +	return lookup_and_lock_nested(last, base, lookup_flags,
> +				      I_MUTEX_PARENT);
> +}
> +
> +void done_lookup_and_lock(struct dentry *base, struct dentry *dentry,
> +			  unsigned int lookup_flags)

Did you mean done_lookup_and_unlock()?

> +{
> +	d_lookup_done(dentry);
> +	dput(dentry);
> +	if (!(lookup_flags & LOOKUP_PARENT_LOCKED))
> +		inode_unlock(base->d_inode);
> +}
> +EXPORT_SYMBOL(done_lookup_and_lock);
> +
>  /**
>   * lookup_fast - do fast lockless (but racy) lookup of a dentry
>   * @nd: current nameidata
> @@ -2754,12 +2789,9 @@ static struct dentry *__kern_path_locked(int dfd, struct filename *name, struct
>  		path_put(path);
>  		return ERR_PTR(-EINVAL);
>  	}
> -	inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
> -	d = lookup_one_qstr(&last, path->dentry, 0);
> -	if (IS_ERR(d)) {
> -		inode_unlock(path->dentry->d_inode);
> +	d = lookup_and_lock(&last, path->dentry, 0);
> +	if (IS_ERR(d))
>  		path_put(path);
> -	}
>  	return d;
>  }
>  
> @@ -3053,6 +3085,22 @@ struct dentry *lookup_positive_unlocked(const char *name,
>  }
>  EXPORT_SYMBOL(lookup_positive_unlocked);
>  
> +struct dentry *lookup_and_lock_one(struct mnt_idmap *idmap,
> +				   const char *name, int len, struct dentry *base,
> +				   unsigned int lookup_flags)
> +{
> +	struct qstr this;
> +	int err;
> +
> +	if (!idmap)
> +		idmap = &nop_mnt_idmap;

The callers should pass nop_mnt_idmap. That's how every function that
takes this argument works. This is a lot more explicit than magically
fixing this up in the function.

> +	err = lookup_one_common(idmap, name, base, len, &this);
> +	if (err)
> +		return ERR_PTR(err);
> +	return lookup_and_lock(&this, base, lookup_flags);
> +}
> +EXPORT_SYMBOL(lookup_and_lock_one);
> +
>  #ifdef CONFIG_UNIX98_PTYS
>  int path_pts(struct path *path)
>  {
> @@ -4071,7 +4119,6 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>  	unsigned int reval_flag = lookup_flags & LOOKUP_REVAL;
>  	unsigned int create_flags = LOOKUP_CREATE | LOOKUP_EXCL;
>  	int type;
> -	int err2;
>  	int error;
>  
>  	error = filename_parentat(dfd, name, reval_flag, path, &last, &type);
> @@ -4083,36 +4130,30 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>  	 * (foo/., foo/.., /////)
>  	 */
>  	if (unlikely(type != LAST_NORM))
> -		goto out;
> +		goto put;
>  
>  	/* don't fail immediately if it's r/o, at least try to report other errors */
> -	err2 = mnt_want_write(path->mnt);
> +	error = mnt_want_write(path->mnt);
>  	/*
>  	 * Do the final lookup.  Suppress 'create' if there is a trailing
>  	 * '/', and a directory wasn't requested.
>  	 */
>  	if (last.name[last.len] && !want_dir)
>  		create_flags &= ~LOOKUP_CREATE;
> -	inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
> -	dentry = lookup_one_qstr(&last, path->dentry,
> -				 reval_flag | create_flags);
> +	dentry = lookup_and_lock(&last, path->dentry, reval_flag | create_flags);
>  	if (IS_ERR(dentry))
> -		goto unlock;
> +		goto drop;
>  
> -	if (unlikely(err2)) {
> -		error = err2;
> +	if (unlikely(error))
>  		goto fail;
> -	}
>  	return dentry;
>  fail:
> -	d_lookup_done(dentry);
> -	dput(dentry);
> +	done_lookup_and_lock(path->dentry, dentry, reval_flag | create_flags);
>  	dentry = ERR_PTR(error);
> -unlock:
> -	inode_unlock(path->dentry->d_inode);
> -	if (!err2)
> +drop:
> +	if (!error)
>  		mnt_drop_write(path->mnt);
> -out:
> +put:
>  	path_put(path);
>  	return dentry;
>  }
> @@ -4130,14 +4171,13 @@ EXPORT_SYMBOL(kern_path_create);
>  
>  void done_path_create(struct path *path, struct dentry *dentry)
>  {
> -	dput(dentry);
> -	inode_unlock(path->dentry->d_inode);
> +	done_lookup_and_lock(path->dentry, dentry, LOOKUP_CREATE);
>  	mnt_drop_write(path->mnt);
>  	path_put(path);
>  }
>  EXPORT_SYMBOL(done_path_create);
>  
> -inline struct dentry *user_path_create(int dfd, const char __user *pathname,
> +struct dentry *user_path_create(int dfd, const char __user *pathname,
>  				struct path *path, unsigned int lookup_flags)
>  {
>  	struct filename *filename = getname(pathname);
> @@ -4510,19 +4550,18 @@ int do_rmdir(int dfd, struct filename *name)
>  	if (error)
>  		goto exit2;
>  
> -	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
> -	dentry = lookup_one_qstr(&last, path.dentry, lookup_flags);
> +	dentry = lookup_and_lock(&last, path.dentry, lookup_flags);
>  	error = PTR_ERR(dentry);
>  	if (IS_ERR(dentry))
>  		goto exit3;
> +
>  	error = security_path_rmdir(&path, dentry);
>  	if (error)
>  		goto exit4;
>  	error = vfs_rmdir(mnt_idmap(path.mnt), path.dentry->d_inode, dentry);
>  exit4:
> -	dput(dentry);
> +	done_lookup_and_lock(path.dentry, dentry, lookup_flags);
>  exit3:
> -	inode_unlock(path.dentry->d_inode);
>  	mnt_drop_write(path.mnt);
>  exit2:
>  	path_put(&path);
> @@ -4639,11 +4678,9 @@ int do_unlinkat(int dfd, struct filename *name)
>  	if (error)
>  		goto exit2;
>  retry_deleg:
> -	inode_lock_nested(path.dentry->d_inode, I_MUTEX_PARENT);
> -	dentry = lookup_one_qstr(&last, path.dentry, lookup_flags);
> +	dentry = lookup_and_lock(&last, path.dentry, lookup_flags);
>  	error = PTR_ERR(dentry);
>  	if (!IS_ERR(dentry)) {
> -
>  		/* Why not before? Because we want correct error value */
>  		if (last.name[last.len])
>  			goto slashes;
> @@ -4655,9 +4692,8 @@ int do_unlinkat(int dfd, struct filename *name)
>  		error = vfs_unlink(mnt_idmap(path.mnt), path.dentry->d_inode,
>  				   dentry, &delegated_inode);
>  exit3:
> -		dput(dentry);
> +		done_lookup_and_lock(path.dentry, dentry, lookup_flags);
>  	}
> -	inode_unlock(path.dentry->d_inode);
>  	if (inode)
>  		iput(inode);	/* truncate the inode here */
>  	inode = NULL;
> diff --git a/include/linux/namei.h b/include/linux/namei.h
> index 0d81e571a159..76c587a5ec3a 100644
> --- a/include/linux/namei.h
> +++ b/include/linux/namei.h
> @@ -29,7 +29,11 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT};
>  #define LOOKUP_RCU		BIT(8)	/* RCU pathwalk mode; semi-internal */
>  #define LOOKUP_CACHED		BIT(9) /* Only do cached lookup */
>  #define LOOKUP_PARENT		BIT(10)	/* Looking up final parent in path */
> -/* 5 spare bits for pathwalk */
> +#define LOOKUP_PARENT_LOCKED	BIT(11)	/* filesystem sets this for nested
> +					 * "lookup_and_lock_one" when it knows
> +					 * parent is sufficiently locked.
> +					 */
> +/* 4 spare bits for pathwalk */
>  
>  /* These tell filesystem methods that we are dealing with the final component... */
>  #define LOOKUP_OPEN		BIT(16)	/* ... in open */
> @@ -82,6 +86,15 @@ struct dentry *lookup_one_unlocked(struct mnt_idmap *idmap,
>  struct dentry *lookup_one_positive_unlocked(struct mnt_idmap *idmap,
>  					    const char *name,
>  					    struct dentry *base, int len);
> +struct dentry *lookup_and_lock_one(struct mnt_idmap *idmap,
> +				   const char *name, int len, struct dentry *base,
> +				   unsigned int lookup_flags);
> +struct dentry *__lookup_and_lock_one(struct mnt_idmap *idmap,
> +				     const char *name, int len, struct dentry *base,
> +				     unsigned int lookup_flags);
> +void done_lookup_and_lock(struct dentry *base, struct dentry *dentry,
> +			  unsigned int lookup_flags);
> +void __done_lookup_and_lock(struct dentry *dentry);
>  
>  extern int follow_down_one(struct path *);
>  extern int follow_down(struct path *path, unsigned int flags);
> -- 
> 2.47.1
>