linux-kernel - Re: [PATCH 08/19] VFS: introduce lookup_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-id: <173889172414.22054.10997679254257474673@noble.neil.brown.name>
Date: Fri, 07 Feb 2025 12:28:44 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Christian Brauner" <brauner@...nel.org>
Cc: "Alexander Viro" <viro@...iv.linux.org.uk>, "Jan Kara" <jack@...e.cz>,
 "Linus Torvalds" <torvalds@...ux-foundation.org>,
 "Jeff Layton" <jlayton@...nel.org>, "Dave Chinner" <david@...morbit.com>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/19] VFS: introduce lookup_and_lock() and friends

On Fri, 07 Feb 2025, Christian Brauner wrote:
> 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@...e.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.

Thanks.

> 
> > +	}
> > +	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()?

No.  The thing that we are done with is "lookup_and_lock()".
This matches "done_path_create()" which doesn't create anything.

On the other hand we have d_lookup_done() which puts _done at the end.
Or end_name_hash().  ->write_end(), finish_automount()

I guess I could accept done_lookup_and_unlock() if you prefer that.

> 
> > +{
> > +	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.

OK.

Thanks,
NeilBrown