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

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

On Fri, Feb 07, 2025 at 08:22:35PM +0000, Al Viro 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.
> 
> Ewww...  I do realize that such things might appear in intermediate
> stages of locking massage, but they'd better be _GONE_ by the end of it.
> Conditional locking of that sort is really asking for trouble.
> 
> If nothing else, better split the function in two variants and document
> the differences; that kind of stuff really does not belong in arguments.
> If you need it to exist through the series, that is - if not, you should
> just leave lookup_one_qstr() for the "locked" case from the very beginning.

The same, BTW, applies to more than LOOKUP_PARENT_LOCKED part.

One general observation: if the locking behaviour of a function depends
upon the flags passed to it, it's going to cause massive headache afterwards.

If you need to bother with data flow analysis to tell what given call will
do, expect trouble.

If anything, I would rather have separate lookup_for_removal(), etc., each
with its locking effects explicitly spelled out.  Incidentally, looking
through that I would say that your "VFS: filename_create(): fix incorrect
intent" is not the right solution.  If we hit that condition (no LOOKUP_DIRECTORY
and last component ending with slash), we are going to fail anyway, the only
question is which error to return.  Rules:
	* if the last component lookup fails, return the error from lookup
	* if it yields positive, return -EEXIST
	* if it yields negative, return -ENOENT
Correct?  So how about this:

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;