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

[Al Viro <viro@zeniv.linux.org.uk>]

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).
> 
> > +	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@zeniv.linux.org.uk>
---
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);
+		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;