Re: [PATCH 2/2] VFS: add common error checks to lookup_one_qstr_excl()

[NeilBrown]

On Wed, 12 Feb 2025, Al Viro wrote:
> On Wed, Feb 12, 2025 at 03:16:08AM +0000, Al Viro wrote:
> > On Fri, Feb 07, 2025 at 02:36:48PM +1100, NeilBrown wrote:
> > > @@ -1690,6 +1692,15 @@ struct dentry *lookup_one_qstr_excl(const struct qstr *name,
> > >  		dput(dentry);
> > >  		dentry = old;
> > >  	}
> > > +found:
> > 
> > ... and if ->lookup() returns an error, this will blow up (as bot has just
> > reported).

Yes, I need an early exit if (IS_ERR(dentry)).  Thanks.

> > 
> > > +	if (d_is_negative(dentry) && !(flags & LOOKUP_CREATE)) {
> > > +		dput(dentry);
> > > +		return ERR_PTR(-ENOENT);
> > > +	}
> > > +	if (d_is_positive(dentry) && (flags & LOOKUP_EXCL)) {
> > > +		dput(dentry);
> > > +		return ERR_PTR(-EEXIST);
> > > +	}
> > 
> > 
> > > @@ -4077,27 +4084,13 @@ static struct dentry *filename_create(int dfd, struct filename *name,
> > >  	 * '/', and a directory wasn't requested.
> > >  	 */
> > >  	if (last.name[last.len] && !want_dir)
> > > -		create_flags = 0;
> > > +		create_flags &= ~LOOKUP_CREATE;
> > 
> > See the patch I've posted in earlier thread; the entire "strip LOOKUP_CREATE"
> > thing is wrong.
> 
> On top of mainline that's
> 
> filename_create(): don't force handling trailing slashes into the common path
> 
> Only mkdir accepts pathnames that end with / - anything like mknod() (symlink(),
> etc.) always fails on those.  Don't try to force that the common codepath -
> all we are doing is a lookup and check for existence to determine which
> error should it be.  Do that before bothering with mnt_want_write(), etc.;
> as far as underlying filesystem is concerned it's just a lookup.  Simplifies
> the normal codepath and kills the lookup intent dependency on more than
> the call site.
> 
> Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> ---
> diff --git a/fs/namei.c b/fs/namei.c
> index 3ab9440c5b93..6189e54f767a 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4054,13 +4054,13 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>  	struct dentry *dentry = ERR_PTR(-EEXIST);
>  	struct qstr last;
>  	bool want_dir = lookup_flags & LOOKUP_DIRECTORY;
> -	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);
> +	lookup_flags &= LOOKUP_REVAL;
> +
> +	error = filename_parentat(dfd, name, lookup_flags, path, &last, &type);
>  	if (error)
>  		return ERR_PTR(error);
>  
> @@ -4070,18 +4070,28 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>  	 */
>  	if (unlikely(type != LAST_NORM))
>  		goto out;
> +	/*
> +	 * mkdir foo/bar/ is OK, but for anything else a slash in the end
> +	 * is always an error; the only question is which one.
> +	 */
> +	if (unlikely(last.name[last.len] && !want_dir)) {
> +		dentry = lookup_dcache(&last, path->dentry, lookup_flags);
> +		if (!dentry)
> +			dentry = lookup_slow(&last, path->dentry, lookup_flags);

I do see some value in the simplicity of this approach, though maybe not
as much value as you see.  But the above uses inode_lock_share(), rather
than the nested version, so lockdep will complain.
If you open-code a nested lock, or write a new helper, you get very
close to the sequence for calling lookup_one_qstr_excl() below.  So
it isn't clear to me that the benefit is worth the cost.

This current code in filename_create isn't actually wrong is it?

Thanks,
NeilBrown



> +		if (!IS_ERR(dentry)) {
> +			error = d_is_positive(dentry) ? -EEXIST : -ENOENT;
> +			dput(dentry);
> +			dentry = ERR_PTR(error);
> +		}
> +		goto out;
> +	}
>  
>  	/* don't fail immediately if it's r/o, at least try to report other errors */
>  	err2 = 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 = 0;
> +	/* do the final lookup */
>  	inode_lock_nested(path->dentry->d_inode, I_MUTEX_PARENT);
>  	dentry = lookup_one_qstr_excl(&last, path->dentry,
> -				      reval_flag | create_flags);
> +				lookup_flags | LOOKUP_CREATE | LOOKUP_EXCL);
>  	if (IS_ERR(dentry))
>  		goto unlock;
>  
> @@ -4089,16 +4099,6 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>  	if (d_is_positive(dentry))
>  		goto fail;
>  
> -	/*
> -	 * Special case - lookup gave negative, but... we had foo/bar/
> -	 * From the vfs_mknod() POV we just have a negative dentry -
> -	 * all is fine. Let's be bastards - you had / on the end, you've
> -	 * been asking for (non-existent) directory. -ENOENT for you.
> -	 */
> -	if (unlikely(!create_flags)) {
> -		error = -ENOENT;
> -		goto fail;
> -	}
>  	if (unlikely(err2)) {
>  		error = err2;
>  		goto fail;
>