[PATCH v3 00/14] vfs: add O_CREAT|O_DIRECTORY to open*(2)

[PATCH v3 00/14] vfs: add O_CREAT|O_DIRECTORY to open*(2)

[Jori Koolstra]

This series implements new semantics for the O_CREAT|O_DIRECTORY flag
combination for open*(2): perform a mkdir and open the resulting
directory; return a pinning fd (which mkdir does not).

Most of the work happens in "vfs: add O_CREAT|O_DIRECTORY to open*(2),"
as may be expected. Before that there is some clean-up work and
preparation. Also, the inconsistency in calling audit_inode_child() in
lookup_open() versus vfs_create() path is addressed. The
"vfs: short-circuit MAY_WRITE access for O_DIRECTORY opens" patch is
also worth paying extra attention to as it short-circuits doomed opening
of directories as writable. This check for regular files is now done
very late in do_open(), but for O_CREAT|O_DIRECTORY this is unacceptable
as this would create the directory and then still fail.

Jori Koolstra (14):
  fs/namei.c: use trailing_slashes()
  vfs: move create error && negative dentry case in lookup_open() up
  vfs: prepare vfs_creat|mkdir_no_perm for reuse in lookup_open()
  vfs: call audit_inode_child() in lookup_open() on failure too
  fs/namei.c: update docstring of atomic_open()
  vfs: lookup_open(): move setting FMODE_CREATED down
  vfs: move ->create check in lookup_open() to before try_break_deleg()
  vfs: lookup_open(): use vfs_create_no_perm()
  vfs: add O_CREAT|O_DIRECTORY to open*(2)
  vfs: move O_IS_MKDIR check from lookup_open() into individual
    filesystems
  vfs: refuse O_CREAT for directories through a dangling symlink
  vfs: short-circuit MAY_WRITE access for O_DIRECTORY opens
  selftest: fix headers in fclog.c
  selftest: add tests for open*(O_CREAT|O_DIRECTORY)

 fs/9p/vfs_inode.c                             |   3 +
 fs/9p/vfs_inode_dotl.c                        |   3 +
 fs/ceph/file.c                                |   3 +
 fs/fuse/dir.c                                 |   3 +
 fs/gfs2/inode.c                               |   3 +
 fs/namei.c                                    | 313 ++++++++++++------
 fs/nfs/dir.c                                  |   6 +
 fs/open.c                                     |  32 +-
 fs/smb/client/dir.c                           |   3 +
 fs/vboxsf/dir.c                               |   3 +
 include/linux/fcntl.h                         |   6 +
 .../testing/selftests/filesystems/.gitignore  |   1 +
 tools/testing/selftests/filesystems/Makefile  |   2 +-
 tools/testing/selftests/filesystems/fclog.c   |   4 +-
 .../filesystems/open_o_creat_o_dir.c          | 197 +++++++++++
 15 files changed, 463 insertions(+), 119 deletions(-)
 create mode 100644 tools/testing/selftests/filesystems/open_o_creat_o_dir.c

-- 
2.55.0

[PATCH v3 01/14] fs/namei.c: use trailing_slashes()

[Jori Koolstra]

There are several places in fs/namei.c that can use the
trailing_slashes() function to improve intent.

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 19ce43c9a6e6..a8890bc0ae21 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2781,9 +2781,14 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
 	return s;
 }
 
+static inline bool trailing_slashes(const struct qstr last)
+{
+	return (bool)last.name[last.len];
+}
+
 static inline const char *lookup_last(struct nameidata *nd)
 {
-	if (nd->last_type == LAST_NORM && nd->last.name[nd->last.len])
+	if (nd->last_type == LAST_NORM && trailing_slashes(nd->last))
 		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
 
 	return walk_component(nd, WALK_TRAILING);
@@ -4521,17 +4526,12 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	return ERR_PTR(error);
 }
 
-static inline bool trailing_slashes(struct nameidata *nd)
-{
-	return (bool)nd->last.name[nd->last.len];
-}
-
 static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
 {
 	struct dentry *dentry;
 
 	if (open_flag & O_CREAT) {
-		if (trailing_slashes(nd))
+		if (trailing_slashes(nd->last))
 			return ERR_PTR(-EISDIR);
 
 		/* Don't bother on an O_EXCL create */
@@ -4539,7 +4539,7 @@ static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
 			return NULL;
 	}
 
-	if (trailing_slashes(nd))
+	if (trailing_slashes(nd->last))
 		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
 
 	dentry = lookup_fast(nd);
@@ -4950,7 +4950,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
 	 * Do the final lookup.  Suppress 'create' if there is a trailing
 	 * '/', and a directory wasn't requested.
 	 */
-	if (last.name[last.len] && !want_dir)
+	if (trailing_slashes(last) && !want_dir)
 		create_flags &= ~LOOKUP_CREATE;
 	dentry = start_dirop(path->dentry, &last, reval_flag | create_flags);
 	if (IS_ERR(dentry))
@@ -5569,7 +5569,7 @@ int filename_unlinkat(int dfd, struct filename *name)
 		goto exit_drop_write;
 
 	/* Why not before? Because we want correct error value */
-	if (unlikely(last.name[last.len])) {
+	if (unlikely(trailing_slashes(last))) {
 		if (d_is_dir(dentry))
 			error = -EISDIR;
 		else
@@ -6171,16 +6171,16 @@ int filename_renameat2(int olddfd, struct filename *from,
 	if (flags & RENAME_EXCHANGE) {
 		if (!d_is_dir(rd.new_dentry)) {
 			error = -ENOTDIR;
-			if (new_last.name[new_last.len])
+			if (trailing_slashes(new_last))
 				goto exit_unlock;
 		}
 	}
 	/* unless the source is a directory trailing slashes give -ENOTDIR */
 	if (!d_is_dir(rd.old_dentry)) {
 		error = -ENOTDIR;
-		if (old_last.name[old_last.len])
+		if (trailing_slashes(old_last))
 			goto exit_unlock;
-		if (!(flags & RENAME_EXCHANGE) && new_last.name[new_last.len])
+		if (!(flags & RENAME_EXCHANGE) && trailing_slashes(new_last))
 			goto exit_unlock;
 	}
 
-- 
2.55.0

[PATCH v3 02/14] vfs: move create error && negative dentry case in lookup_open() up

[Jori Koolstra]

O_CREAT is stripped when create_error is set in lookup_open(), so when
lookup does not return an inode, the case

	if (!dentry->d_inode && (open_flag & O_CREAT))

is always skipped. We can get rid of this cognitive step by handling the
error case first.

Reviewed-by: NeilBrown <neil@brown.name>
Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index a8890bc0ae21..40f83a942e12 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4496,6 +4496,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		}
 	}
 
+	if (unlikely(create_error) && !dentry->d_inode) {
+		error = create_error;
+		goto out_dput;
+	}
+
 	/* Negative dentry, just create the file */
 	if (!dentry->d_inode && (open_flag & O_CREAT)) {
 		/* but break the directory lease first! */
@@ -4515,10 +4520,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		if (error)
 			goto out_dput;
 	}
-	if (unlikely(create_error) && !dentry->d_inode) {
-		error = create_error;
-		goto out_dput;
-	}
+
 	return dentry;
 
 out_dput:
-- 
2.55.0

[PATCH v3 03/14] vfs: prepare vfs_creat|mkdir_no_perm for reuse in lookup_open()

[Jori Koolstra]

To implement O_CREAT|O_DIRECTORY we will have to repeat some of the
logic that is now in vfs_mkdir() (e.g. do error checks in the same
order). Separate this out in vfs_mkdir_no_perm(), which does all the
non-permission related work of vfs_mkdir(). Permission checking for the
lookup_open() path is timed differently because we may just be doing an
open and no create. Similar considerations give rise to
vfs_create_no_perm().

Moving the fsnotify_* calls also allows us to deal with this in one
place for each type of operation. This does mean that we also need
to move the fsnotify_* calls into atomic_open() for the atomic open
case, but this actually reduces duplicate code in open_last_lookups()
and dentry_create().

Reviewed-by: NeilBrown <neil@brown.name>
Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 105 ++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 68 insertions(+), 37 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 40f83a942e12..ee47d3f5159f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4169,6 +4169,24 @@ static inline umode_t vfs_prepare_mode(struct mnt_idmap *idmap,
 	return mode;
 }
 
+static inline
+int vfs_create_no_perm(struct mnt_idmap *idmap, struct dentry *dentry,
+		       umode_t mode, struct delegated_inode *di)
+{
+	struct inode *dir = d_inode(dentry->d_parent);
+	int error;
+
+	error = try_break_deleg(dir, LEASE_BREAK_DIR_CREATE, di);
+	if (error)
+		return error;
+
+	error = dir->i_op->create(idmap, dir, dentry, mode, true);
+	if (!error)
+		fsnotify_create(dir, dentry);
+
+	return error;
+}
+
 /**
  * vfs_create - create new file
  * @idmap:	idmap of the mount the inode was found from
@@ -4201,13 +4219,8 @@ int vfs_create(struct mnt_idmap *idmap, struct dentry *dentry, umode_t mode,
 	error = security_inode_create(dir, dentry, mode);
 	if (error)
 		return error;
-	error = try_break_deleg(dir, LEASE_BREAK_DIR_CREATE, di);
-	if (error)
-		return error;
-	error = dir->i_op->create(idmap, dir, dentry, mode, true);
-	if (!error)
-		fsnotify_create(dir, dentry);
-	return error;
+
+	return vfs_create_no_perm(idmap, dentry, mode, di);
 }
 EXPORT_SYMBOL(vfs_create);
 
@@ -4384,10 +4397,17 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 				error = -ENOENT;
 		}
 	}
+
 	if (error) {
 		dput(dentry);
 		dentry = ERR_PTR(error);
+	} else {
+		if (file->f_mode & FMODE_CREATED)
+			fsnotify_create(dir, dentry);
+		if (file->f_mode & FMODE_OPENED)
+			fsnotify_open(file);
 	}
+
 	return dentry;
 }
 
@@ -4519,6 +4539,8 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 						mode, open_flag & O_EXCL);
 		if (error)
 			goto out_dput;
+
+		fsnotify_create(dir_inode, dentry);
 	}
 
 	return dentry;
@@ -4607,13 +4629,9 @@ static const char *open_last_lookups(struct nameidata *nd,
 		inode_lock(dir->d_inode);
 	else
 		inode_lock_shared(dir->d_inode);
+
 	dentry = lookup_open(nd, file, op, got_write, &delegated_inode);
-	if (!IS_ERR(dentry)) {
-		if (file->f_mode & FMODE_CREATED)
-			fsnotify_create(dir->d_inode, dentry);
-		if (file->f_mode & FMODE_OPENED)
-			fsnotify_open(file);
-	}
+
 	if (open_flag & O_CREAT)
 		inode_unlock(dir->d_inode);
 	else
@@ -5066,13 +5084,6 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
 		if (unlikely(create_error) && error == -ENOENT)
 			error = create_error;
 
-		if (!error) {
-			if (file->f_mode & FMODE_CREATED)
-				fsnotify_create(dir->d_inode, dentry);
-			if (file->f_mode & FMODE_OPENED)
-				fsnotify_open(file);
-		}
-
 		path->dentry = dentry;
 
 	} else {
@@ -5224,6 +5235,39 @@ SYSCALL_DEFINE3(mknod, const char __user *, filename, umode_t, mode, unsigned, d
 	return filename_mknodat(AT_FDCWD, name, mode, dev);
 }
 
+static inline
+struct dentry *vfs_mkdir_no_perm(struct mnt_idmap *idmap, struct inode *dir,
+				 struct dentry *dentry, umode_t mode,
+				 struct delegated_inode *di)
+{
+	int error;
+	struct dentry *de;
+	unsigned max_links = dir->i_sb->s_max_links;
+
+	error = -EMLINK;
+	if (max_links && dir->i_nlink >= max_links)
+		goto err;
+
+	error = try_break_deleg(dir, LEASE_BREAK_DIR_CREATE, di);
+	if (error)
+		goto err;
+
+	de = dir->i_op->mkdir(idmap, dir, dentry, mode);
+	if (IS_ERR(de)) {
+		error = PTR_ERR(de);
+		goto err;
+	}
+	if (de) {
+		dput(dentry);
+		dentry = de;
+	}
+	fsnotify_mkdir(dir, dentry);
+	return dentry;
+
+err:
+	return ERR_PTR(error);
+}
+
 /**
  * vfs_mkdir - create directory returning correct dentry if possible
  * @idmap:		idmap of the mount the inode was found from
@@ -5251,7 +5295,6 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 			 struct delegated_inode *delegated_inode)
 {
 	int error;
-	unsigned max_links = dir->i_sb->s_max_links;
 	struct dentry *de;
 
 	error = may_create_dentry(idmap, dir, dentry);
@@ -5267,24 +5310,12 @@ struct dentry *vfs_mkdir(struct mnt_idmap *idmap, struct inode *dir,
 	if (error)
 		goto err;
 
-	error = -EMLINK;
-	if (max_links && dir->i_nlink >= max_links)
-		goto err;
-
-	error = try_break_deleg(dir, LEASE_BREAK_DIR_CREATE, delegated_inode);
-	if (error)
-		goto err;
-
-	de = dir->i_op->mkdir(idmap, dir, dentry, mode);
-	error = PTR_ERR(de);
-	if (IS_ERR(de))
+	de = vfs_mkdir_no_perm(idmap, dir, dentry, mode, delegated_inode);
+	if (IS_ERR(de)) {
+		error = PTR_ERR(de);
 		goto err;
-	if (de) {
-		dput(dentry);
-		dentry = de;
 	}
-	fsnotify_mkdir(dir, dentry);
-	return dentry;
+	return de;
 
 err:
 	end_creating(dentry);
-- 
2.55.0

[PATCH v3 04/14] vfs: call audit_inode_child() in lookup_open() on failure too

[Jori Koolstra]

audit_inode_child() is called in may_create_dentry() so that failed
filesystem operations still register an audit entry. On success, the
entry is overwritten when, for instance, fsnotify_create() is called.
This is the calling convention in vfs_create() and vfs_mkdir().
In lookup_open(), however, when atomic_open() should have created a
file but didn't, no call to audit_inode_child() is made. The same is
true for the regular ->create() path.

Fix the calling of audit_inode_child() in lookup_open() to match the
vfs_create() path. For the ->atomic_open() filesystems this logic has
been pushed into atomic_open(). This function is also reordered a bit
to make the case distinction of the possible returns from
->atomic_open() more explicit (i.e. finish_open() or finish_no_open()).

When retrying delegation breaking, audit_inode_child() could be called
more than once, but this is OK because those entries are reused.

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 75 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index ee47d3f5159f..956adcd14c4a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4369,10 +4369,11 @@ static int may_o_create(struct mnt_idmap *idmap,
  */
 static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
 				  struct file *file,
-				  int open_flag, umode_t mode)
+				  int open_flag, umode_t mode, bool create_error)
 {
 	struct dentry *const DENTRY_NOT_SET = (void *) -1UL;
 	struct inode *dir =  path->dentry->d_inode;
+	bool failed_create = false;
 	int error;
 
 	file->__f_path.dentry = DENTRY_NOT_SET;
@@ -4382,23 +4383,33 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 	d_lookup_done(dentry);
 	if (!error) {
 		if (file->f_mode & FMODE_OPENED) {
-			if (unlikely(dentry != file->f_path.dentry)) {
+			// finish_open() called
+			struct dentry *opened = file->f_path.dentry;
+			if (unlikely(opened != dentry)) {
 				dput(dentry);
-				dentry = dget(file->f_path.dentry);
+				dentry = dget(opened);
 			}
-		} else if (WARN_ON(file->f_path.dentry == DENTRY_NOT_SET)) {
-			error = -EIO;
-		} else {
-			if (file->f_path.dentry) {
+		} else if (likely(file->f_path.dentry != DENTRY_NOT_SET)) {
+			// finish_no_open() called
+			struct dentry *replaced = file->f_path.dentry;
+			if (replaced) {
 				dput(dentry);
-				dentry = file->f_path.dentry;
+				dentry = replaced;
 			}
 			if (unlikely(d_is_negative(dentry)))
 				error = -ENOENT;
+			if (error && create_error) // should have created, but errored before
+				failed_create = true;
+		} else {
+			const char *fsname = dentry->d_sb->s_type->name;
+			WARN(1, "%s: ->atomic_open() left file->f_path.dentry unset!\n", fsname);
+			error = -EIO;
 		}
 	}
 
 	if (error) {
+		if (failed_create)
+			audit_inode_child(dir, dentry, AUDIT_TYPE_CHILD_CREATE);
 		dput(dentry);
 		dentry = ERR_PTR(error);
 	} else {
@@ -4462,7 +4473,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		dentry = NULL;
 	}
 	if (dentry->d_inode) {
-		/* Cached positive dentry: will open in f_op->open */
+		/* Cached positive dentry: will open in do_open(). */
 		return dentry;
 	}
 
@@ -4496,7 +4507,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (dir_inode->i_op->atomic_open) {
 		if (nd->flags & LOOKUP_DIRECTORY)
 			open_flag |= O_DIRECTORY;
-		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
+		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode, create_error);
 		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
 			dentry = ERR_PTR(create_error);
 		return dentry;
@@ -4515,33 +4526,37 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 			dentry = res;
 		}
 	}
+	if (dentry->d_inode || !(op->open_flag & O_CREAT)) {
+		/* No need to create a file. If lookup returned a positive
+		 * dentry, the file will be opened in do_open(). */
+		return dentry;
+	}
+
+	/* Negative dentry with O_CREAT flag set */
+	audit_inode_child(dir->d_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
 
-	if (unlikely(create_error) && !dentry->d_inode) {
+	if (unlikely(create_error)) {
+		/* Should have done a create, but we already errored */
 		error = create_error;
 		goto out_dput;
 	}
 
-	/* Negative dentry, just create the file */
-	if (!dentry->d_inode && (open_flag & O_CREAT)) {
-		/* but break the directory lease first! */
-		error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
-		if (error)
-			goto out_dput;
+	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
+	if (error)
+		goto out_dput;
 
-		file->f_mode |= FMODE_CREATED;
-		audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
-		if (!dir_inode->i_op->create) {
-			error = -EACCES;
-			goto out_dput;
-		}
+	file->f_mode |= FMODE_CREATED;
+	if (!dir_inode->i_op->create) {
+		error = -EACCES;
+		goto out_dput;
+	}
 
-		error = dir_inode->i_op->create(idmap, dir_inode, dentry,
-						mode, open_flag & O_EXCL);
-		if (error)
-			goto out_dput;
+	error = dir_inode->i_op->create(idmap, dir_inode, dentry,
+					mode, open_flag & O_EXCL);
+	if (error)
+		goto out_dput;
 
-		fsnotify_create(dir_inode, dentry);
-	}
+	fsnotify_create(dir_inode, dentry);
 
 	return dentry;
 
@@ -5071,7 +5086,7 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
 
 		/* atomic_open will dput(dentry) on error */
 		dget(orig_dentry);
-		dentry = atomic_open(path, dentry, file, flags, mode);
+		dentry = atomic_open(path, dentry, file, flags, mode, create_error);
 		error = PTR_ERR_OR_ZERO(dentry);
 
 		if (IS_ERR(dentry))
-- 
2.55.0

[PATCH v3 05/14] fs/namei.c: update docstring of atomic_open()

[Jori Koolstra]

The docstring of atomic_open() contains several errors:
  - It does not return 0 if successful.
  - path is not updated

Fix those and be more explicit about when FMODE_OPENED and FMODE_CREATED
are set.

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 956adcd14c4a..88d56a470832 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4355,17 +4355,24 @@ static int may_o_create(struct mnt_idmap *idmap,
 }
 
 /*
- * Attempt to atomically look up, create and open a file from a negative
- * dentry.
- *
- * Returns 0 if successful.  The file will have been created and attached to
- * @file by the filesystem calling finish_open().
- *
- * If the file was looked up only or didn't need creating, FMODE_OPENED won't
- * be set.  The caller will need to perform the open themselves.  @path will
- * have been updated to point to the new dentry.  This may be negative.
- *
- * Returns an error code otherwise.
+ * atomic_open() - attempt to atomically look up, create and open a file
+ * from a negative dentry.
+ * @path:          parent directory path
+ * @dentry:        child to ->atomic_open()
+ * @open_flag:     open flags
+ * @mode:          create mode
+ * @create_error:  return value from may_o_create()
+ *
+ * If FMODE_OPENED is set, the file will have been attached to @file by the
+ * filesystem calling finish_open(). If FMODE_OPENED isn't set, the
+ * filesystem instead called finish_no_open() and the caller will need to
+ * perform the open themselves.
+ *
+ * FMODE_CREATED is set when the call to ->atomic_open() actually created
+ * the file.
+ *
+ * Returns the opened/looked-up dentry on success or ERR_PTR(-E) on failure.
+ * On error, atomic_open() consumes @dentry.
  */
 static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
 				  struct file *file,
-- 
2.55.0

[PATCH v3 06/14] vfs: lookup_open(): move setting FMODE_CREATED down

[Jori Koolstra]

In preparation for using vfs_create_no_perm() in lookup_open() we need
to move setting FMODE_CREATED on the file mode to either before or after
that call, as currently it is in the middle. If try_break_deleg() fails
it is currently not set, but vfs_create_no_perm() includes a
try_break_deleg().

Going up the call chain of lookup_open() we see that it is only used in
open_last_lookups() if no error is returned from lookup_open(), so we
can safely move it to after the filesystem create() call. This also
makes more sense when reading the code as you don't have to wonder what
the implications are of setting FMODE_CREATED before the create() call.

Reviewed-by: NeilBrown <neil@brown.name>
Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/namei.c b/fs/namei.c
index 88d56a470832..daf31845e3bb 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4552,7 +4552,6 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (error)
 		goto out_dput;
 
-	file->f_mode |= FMODE_CREATED;
 	if (!dir_inode->i_op->create) {
 		error = -EACCES;
 		goto out_dput;
@@ -4564,6 +4563,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		goto out_dput;
 
 	fsnotify_create(dir_inode, dentry);
+	file->f_mode |= FMODE_CREATED;
 
 	return dentry;
 
-- 
2.55.0

[PATCH v3 07/14] vfs: move ->create check in lookup_open() to before try_break_deleg()

[Jori Koolstra]

The i_op->create check in lookup_open() takes place after the
try_break_deleg() call. This does not match the order when doing a
regular file create via mknod(2). There the call order is:

	filename_mknodat()
	vfs_create()
	i_op->create check
	try_break_deleg()

Move the i_op->create check to before try_break_deleg() in
lookup_open().

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index daf31845e3bb..6601460d2772 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4548,15 +4548,15 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		goto out_dput;
 	}
 
-	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
-	if (error)
-		goto out_dput;
-
 	if (!dir_inode->i_op->create) {
 		error = -EACCES;
 		goto out_dput;
 	}
 
+	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
+	if (error)
+		goto out_dput;
+
 	error = dir_inode->i_op->create(idmap, dir_inode, dentry,
 					mode, open_flag & O_EXCL);
 	if (error)
-- 
2.55.0

[PATCH v3 08/14] vfs: lookup_open(): use vfs_create_no_perm()

[Jori Koolstra]

We can replace the code in the no create_error/negative dentry found
from lookup case in lookup_open() with the vfs_create_no_perm() helper.

It is safe here to always pass excl=true to ->create via
vfs_create_no_perm(). Local filesystems always ignore the excl flag,
and cluster filesystems implement ->atomic_open, so that the ->create
call is never reached in lookup_open().

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 6601460d2772..eaa0ade4a1b0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4553,16 +4553,10 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		goto out_dput;
 	}
 
-	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
+	error = vfs_create_no_perm(idmap, dentry, mode, delegated_inode);
 	if (error)
 		goto out_dput;
 
-	error = dir_inode->i_op->create(idmap, dir_inode, dentry,
-					mode, open_flag & O_EXCL);
-	if (error)
-		goto out_dput;
-
-	fsnotify_create(dir_inode, dentry);
 	file->f_mode |= FMODE_CREATED;
 
 	return dentry;
-- 
2.55.0

[PATCH v3 09/14] vfs: add O_CREAT|O_DIRECTORY to open*(2)

[Jori Koolstra]

Currently there is no way to race-freely create and open a directory.
For regular files we have open(O_CREAT) for creating a new file inode,
and returning a pinning fd to it. The lack of such functionality for
directories means that when populating a directory tree there's always
a race involved: the inodes first need to be created, and then opened
to adjust their permissions/ownership/labels/timestamps/acls/xattrs/...,
but in the time window between the creation and the opening they might
be replaced by something else.

Addressing this race without proper APIs is possible (by immediately
fstat()ing what was opened, to verify that it has the right inode type),
but difficult to get right. Hence, adding support for a new flag combo
O_CREAT|O_DIRECTORY to open*(2) that creates a directory (if it does not
exist already) and returns an O_DIRECTORY fd is very useful.

Historically, the O_CREAT|O_DIRECTORY behaviour was to return ENOTDIR if
a regular file exists at the open path; EISDIR if a directory exists at
the path; and to create a regular file if no file exists at the path.
This behaviour changed accidentally with 973d4b73fbaf ("do_last(): rejoin
the common path even earlier in FMODE_{OPENED,CREATED} case") causing
ENOTDIR to return in the last case while still creating the file. As
this change was not detected for a long time, Brauner proposed to adopt
the more consistent NetBSD behaviour, i.e. to return EINVAL on the the
O_CREAT|O_DIRECTORY combination. This change was applied in 43b450632676
("open: return EINVAL for O_DIRECTORY | O_CREAT") in March, 2023. As
the EINVAL behaviour has been in the kernel for about 3 year now, no
rollback is expected as a result of userspace reliance on old
behaviour, leaving us free to reassign the O_CREAT|O_DIRECTORY semantics.

O_CREAT|O_DIRECTORY is made to reduce to a lookup on ->atomic_open()
filesystems. This is implemented by stripping the O_CREAT bit. The other
option of simply returning -EINVAL leads to inconsistent behaviour:
before ->atomic_open() is called in lookup_open(), the dcache is
queried. So returning -EINVAL there would make open(O_CREAT|O_DIRECTORY)
dependent on the cache state of the dentry. By stripping the O_CREAT
bit, lookup is consistently performed on ->atomic_open() filesystems.

There is no separate sysctl for directory creation implemented currently.
Therefore, for the S_ISDIR case, disabling sysctl_protected_regular is
not enough to allow creating a directory in a sticky folder, because that
may surprise users not expecting that O_CREAT|O_DIRECTORY is possible on
newer kernels.

This feature idea (and some of its description) is taken from the
UAPI group:
https://github.com/uapi-group/kernel-features?tab=readme-ov-file#race-free-creation-and-opening-of-non-file-inodes

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c            | 86 +++++++++++++++++++++++++++++++++++--------
 fs/open.c             | 25 +++++++------
 include/linux/fcntl.h |  6 +++
 3 files changed, 90 insertions(+), 27 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index eaa0ade4a1b0..5113ac986f50 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1382,13 +1382,12 @@ int may_linkat(struct mnt_idmap *idmap, const struct path *link)
 
 /**
  * may_create_in_sticky - Check whether an O_CREAT open in a sticky directory
- *			  should be allowed, or not, on files that already
- *			  exist.
+ *			  should be allowed, or not
  * @idmap: idmap of the mount the inode was found from
  * @nd: nameidata pathwalk data
  * @inode: the inode of the file to open
  *
- * Block an O_CREAT open of a FIFO (or a regular file) when:
+ * Block an O_CREAT open of a FIFO (or a regular file/directory) when:
  *   - sysctl_protected_fifos (or sysctl_protected_regular) is enabled
  *   - the file already exists
  *   - we are in a sticky directory
@@ -1416,6 +1415,14 @@ static int may_create_in_sticky(struct mnt_idmap *idmap, struct nameidata *nd,
 	if (likely(!(dir_mode & S_ISVTX)))
 		return 0;
 
+	/*
+	 * There is no separate sysctl for directory creation in sticky
+	 * folders. Therefore, for the S_ISDIR case, disabling
+	 * sysctl_protected_regular is not enough to allow creating a
+	 * directory in a sticky folder, because that may surprise users
+	 * not expecting that O_CREAT|O_DIRECTORY is possible on newer
+	 * kernels.
+	 */
 	if (S_ISREG(inode->i_mode) && !sysctl_protected_regular)
 		return 0;
 
@@ -1447,6 +1454,12 @@ static int may_create_in_sticky(struct mnt_idmap *idmap, struct nameidata *nd,
 					      "sticky_create_regular");
 			return -EACCES;
 		}
+
+		if (S_ISDIR(inode->i_mode)) {
+			audit_log_path_denied(AUDIT_ANOM_CREAT,
+					      "sticky_create_dir");
+			return -EACCES;
+		}
 	}
 
 	return 0;
@@ -4337,21 +4350,40 @@ static inline int open_to_namei_flags(int flag)
 
 static int may_o_create(struct mnt_idmap *idmap,
 			const struct path *dir, struct dentry *dentry,
-			umode_t mode)
+			umode_t mode, bool create_dir)
 {
-	int error = security_path_mknod(dir, dentry, mode, 0);
+	struct inode *dir_inode = dir->dentry->d_inode;
+	int error;
+
+	if (create_dir)
+		error = security_path_mkdir(dir, dentry, mode);
+	else
+		error = security_path_mknod(dir, dentry, mode, 0);
 	if (error)
 		return error;
 
 	if (!fsuidgid_has_mapping(dir->dentry->d_sb, idmap))
 		return -EOVERFLOW;
 
-	error = inode_permission(idmap, dir->dentry->d_inode,
-				 MAY_WRITE | MAY_EXEC);
+	error = inode_permission(idmap, dir_inode, MAY_WRITE | MAY_EXEC);
 	if (error)
 		return error;
 
-	return security_inode_create(dir->dentry->d_inode, dentry, mode);
+	if (create_dir)
+		error = security_inode_mkdir(dir_inode, dentry, mode);
+	else
+		error = security_inode_create(dir_inode, dentry, mode);
+
+	return error;
+}
+
+static inline umode_t o_create_mode(struct mnt_idmap *idmap,
+		const struct inode *dir, umode_t mode, bool create_dir)
+{
+	if (create_dir)
+		return vfs_prepare_mode(idmap, dir, mode, S_IRWXUGO | S_ISVTX, 0);
+	else
+		return vfs_prepare_mode(idmap, dir, mode, S_IALLUGO, S_IFREG);
 }
 
 /*
@@ -4385,6 +4417,7 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 
 	file->__f_path.dentry = DENTRY_NOT_SET;
 	file->__f_path.mnt = path->mnt;
+
 	error = dir->i_op->atomic_open(dir, dentry, file,
 				       open_to_namei_flags(open_flag), mode);
 	d_lookup_done(dentry);
@@ -4429,6 +4462,10 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 	return dentry;
 }
 
+static inline
+struct dentry *vfs_mkdir_no_perm(struct mnt_idmap *, struct inode *,
+				 struct dentry *, umode_t,
+				 struct delegated_inode *);
 /*
  * Look up and maybe create and open the last component.
  *
@@ -4455,10 +4492,14 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	struct dentry *dentry;
 	int error, create_error = 0;
 	umode_t mode = op->mode;
+	bool create_dir = O_IS_MKDIR(open_flag);
 
 	if (unlikely(IS_DEADDIR(dir_inode)))
 		return ERR_PTR(-ENOENT);
 
+	if (O_IS_MKDIR(open_flag) && dir_inode->i_op->atomic_open)
+		open_flag &= ~O_CREAT;
+
 	file->f_mode &= ~FMODE_CREATED;
 	dentry = d_lookup(dir, &nd->last);
 	for (;;) {
@@ -4502,10 +4543,10 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (open_flag & O_CREAT) {
 		if (open_flag & O_EXCL)
 			open_flag &= ~O_TRUNC;
-		mode = vfs_prepare_mode(idmap, dir->d_inode, mode, mode, mode);
+		mode = o_create_mode(idmap, dir_inode, mode, create_dir);
 		if (likely(got_write))
 			create_error = may_o_create(idmap, &nd->path,
-						    dentry, mode);
+						    dentry, mode, create_dir);
 		else
 			create_error = -EROFS;
 	}
@@ -4548,12 +4589,24 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		goto out_dput;
 	}
 
-	if (!dir_inode->i_op->create) {
+	if ((create_dir && !dir_inode->i_op->mkdir)
+	    || (!create_dir && !dir_inode->i_op->create)) {
 		error = -EACCES;
 		goto out_dput;
 	}
 
-	error = vfs_create_no_perm(idmap, dentry, mode, delegated_inode);
+	if (create_dir) {
+		struct dentry *res = vfs_mkdir_no_perm(idmap, dir_inode, dentry,
+						       mode, delegated_inode);
+		if (IS_ERR(res)) {
+			error = PTR_ERR(res);
+		} else {
+			error = 0;
+			dentry = res;
+		}
+	} else {
+		error = vfs_create_no_perm(idmap, dentry, mode, delegated_inode);
+	}
 	if (error)
 		goto out_dput;
 
@@ -4571,7 +4624,7 @@ static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
 	struct dentry *dentry;
 
 	if (open_flag & O_CREAT) {
-		if (trailing_slashes(nd->last))
+		if (trailing_slashes(nd->last) && !(open_flag & O_DIRECTORY))
 			return ERR_PTR(-EISDIR);
 
 		/* Don't bother on an O_EXCL create */
@@ -4642,7 +4695,7 @@ static const char *open_last_lookups(struct nameidata *nd,
 		 */
 	}
 	if (open_flag & O_CREAT)
-		inode_lock(dir->d_inode);
+		inode_lock_nested(dir->d_inode, I_MUTEX_PARENT);
 	else
 		inode_lock_shared(dir->d_inode);
 
@@ -4705,8 +4758,9 @@ static int do_open(struct nameidata *nd,
 	if (open_flag & O_CREAT) {
 		if ((open_flag & O_EXCL) && !(file->f_mode & FMODE_CREATED))
 			return -EEXIST;
-		if (d_is_dir(nd->path.dentry))
+		if (!(open_flag & O_DIRECTORY) && d_is_dir(nd->path.dentry))
 			return -EISDIR;
+
 		error = may_create_in_sticky(idmap, nd,
 					     d_backing_inode(nd->path.dentry));
 		if (unlikely(error))
@@ -5081,7 +5135,7 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
 		path->dentry = dir;
 		mode = vfs_prepare_mode(idmap, dir_inode, mode, S_IALLUGO, S_IFREG);
 
-		create_error = may_o_create(idmap, path, dentry, mode);
+		create_error = may_o_create(idmap, path, dentry, mode, /*create_dir=*/ false);
 		if (create_error)
 			flags &= ~O_CREAT;
 
diff --git a/fs/open.c b/fs/open.c
index 408925d7bd0b..9121aece78bc 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1190,29 +1190,30 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
 	if (WILL_CREATE(flags)) {
 		if (how->mode & ~S_IALLUGO)
 			return -EINVAL;
-		op->mode = how->mode | S_IFREG;
+		if (O_IS_MKDIR(flags))
+			op->mode = how->mode | S_IFDIR;
+		else
+			op->mode = how->mode | S_IFREG;
 	} else {
 		if (how->mode != 0)
 			return -EINVAL;
 		op->mode = 0;
 	}
 
-	/*
-	 * Block bugs where O_DIRECTORY | O_CREAT created regular files.
-	 * Note, that blocking O_DIRECTORY | O_CREAT here also protects
-	 * O_TMPFILE below which requires O_DIRECTORY being raised.
-	 */
-	if ((flags & (O_DIRECTORY | O_CREAT)) == (O_DIRECTORY | O_CREAT))
-		return -EINVAL;
-
 	/* Now handle the creative implementation of O_TMPFILE. */
 	if (flags & __O_TMPFILE) {
 		/*
 		 * In order to ensure programs get explicit errors when trying
 		 * to use O_TMPFILE on old kernels we enforce that O_DIRECTORY
-		 * is raised alongside __O_TMPFILE.
+		 * is raised alongside __O_TMPFILE, but without O_CREAT. The
+		 * reason for disallowing O_CREAT|O_TMPFILE is that
+		 * O_DIRECTORY|O_CREAT used to work and created a regular file
+		 * if nothing existed at the open path. Hence, allowing the
+		 * combination would have caused O_CREAT|O_TMPFILE to create a
+		 * regular (non-temporary) file on old kernels, while the caller
+		 * would believe they created an actual O_TMPFILE.
 		 */
-		if (!(flags & O_DIRECTORY))
+		if (!(flags & O_DIRECTORY) || (flags & O_CREAT))
 			return -EINVAL;
 		if (!(acc_mode & MAY_WRITE))
 			return -EINVAL;
@@ -1270,6 +1271,8 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
 	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
 
 	if (flags & O_CREAT) {
+		if ((flags & O_DIRECTORY) && (acc_mode & MAY_WRITE))
+			return -EISDIR;
 		op->intent |= LOOKUP_CREATE;
 		if (flags & O_EXCL) {
 			op->intent |= LOOKUP_EXCL;
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 6ad6b9e7a226..6b75277be559 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -30,6 +30,12 @@
  */
 #define __O_REGULAR		(1 << 30)
 
+#define O_MKDIR_MASK	(O_CREAT | O_DIRECTORY)
+static inline bool O_IS_MKDIR(unsigned flags)
+{
+	return (flags & O_MKDIR_MASK) == O_MKDIR_MASK;
+}
+
 /* List of all valid flags for the how->resolve argument: */
 #define VALID_RESOLVE_FLAGS \
 	(RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \
-- 
2.55.0

[PATCH v3 10/14] vfs: move O_IS_MKDIR check from lookup_open() into individual filesystems

[Jori Koolstra]

Individual filesystems that implement ->atomic_open() need to get the
chance to implement O_CREAT|O_DIRECTORY or not, rather than decide
this at the VFS level in lookup_open().

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/9p/vfs_inode.c      | 3 +++
 fs/9p/vfs_inode_dotl.c | 3 +++
 fs/ceph/file.c         | 3 +++
 fs/fuse/dir.c          | 3 +++
 fs/gfs2/inode.c        | 3 +++
 fs/namei.c             | 3 ---
 fs/nfs/dir.c           | 6 ++++++
 fs/smb/client/dir.c    | 3 +++
 fs/vboxsf/dir.c        | 3 +++
 9 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
index 5783d0336f96..bb555e5ba261 100644
--- a/fs/9p/vfs_inode.c
+++ b/fs/9p/vfs_inode.c
@@ -777,6 +777,9 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
 	struct inode *inode;
 	int p9_omode;
 
+	if (O_IS_MKDIR(flags))
+		flags &= ~O_CREAT;
+
 	if (d_in_lookup(dentry)) {
 		struct dentry *res = v9fs_vfs_lookup(dir, dentry, 0);
 		if (res || d_really_is_positive(dentry))
diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
index f7396d20cb6c..ff3facb420c2 100644
--- a/fs/9p/vfs_inode_dotl.c
+++ b/fs/9p/vfs_inode_dotl.c
@@ -239,6 +239,9 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
 	struct v9fs_session_info *v9ses;
 	struct posix_acl *pacl = NULL, *dacl = NULL;
 
+	if (O_IS_MKDIR(flags))
+		flags &= ~O_CREAT;
+
 	if (d_in_lookup(dentry)) {
 		struct dentry *res = v9fs_vfs_lookup(dir, dentry, 0);
 		if (res || d_really_is_positive(dentry))
diff --git a/fs/ceph/file.c b/fs/ceph/file.c
index 71161f2b2151..e79abdbd9f61 100644
--- a/fs/ceph/file.c
+++ b/fs/ceph/file.c
@@ -811,6 +811,9 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
 	      dir, ceph_vinop(dir), dentry, dentry,
 	      d_unhashed(dentry) ? "unhashed" : "hashed", flags, mode);
 
+	if (O_IS_MKDIR(flags))
+		flags &= ~O_CREAT;
+
 	if (dentry->d_name.len > NAME_MAX)
 		return -ENAMETOOLONG;
 
diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 0e2a1039fa43..e01869565d5e 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -935,6 +935,9 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 	struct mnt_idmap *idmap = file_mnt_idmap(file);
 	struct fuse_conn *fc = get_fuse_conn(dir);
 
+	if (O_IS_MKDIR(flags))
+		flags &= ~O_CREAT;
+
 	if (fuse_is_bad(dir))
 		return -EIO;
 
diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 8a77794bbd4a..9859ae0341a9 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -1387,6 +1387,9 @@ static int gfs2_atomic_open(struct inode *dir, struct dentry *dentry,
 {
 	bool excl = !!(flags & O_EXCL);
 
+	if (O_IS_MKDIR(flags))
+		flags &= ~O_CREAT;
+
 	if (d_in_lookup(dentry)) {
 		struct dentry *d = __gfs2_lookup(dir, dentry, file);
 		if (file->f_mode & FMODE_OPENED) {
diff --git a/fs/namei.c b/fs/namei.c
index 5113ac986f50..76916caa264d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4497,9 +4497,6 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 	if (unlikely(IS_DEADDIR(dir_inode)))
 		return ERR_PTR(-ENOENT);
 
-	if (O_IS_MKDIR(open_flag) && dir_inode->i_op->atomic_open)
-		open_flag &= ~O_CREAT;
-
 	file->f_mode &= ~FMODE_CREATED;
 	dentry = d_lookup(dir, &nd->last);
 	for (;;) {
diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
index c7b723c18620..c815d9aa9b69 100644
--- a/fs/nfs/dir.c
+++ b/fs/nfs/dir.c
@@ -2121,6 +2121,9 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
 	dfprintk(VFS, "NFS: atomic_open(%s/%llu), %pd\n",
 			dir->i_sb->s_id, dir->i_ino, dentry);
 
+	if (O_IS_MKDIR(open_flags))
+		open_flags &= ~O_CREAT;
+
 	err = nfs_check_flags(open_flags);
 	if (err)
 		return err;
@@ -2313,6 +2316,9 @@ int nfs_atomic_open_v23(struct inode *dir, struct dentry *dentry,
 	 */
 	int error = 0;
 
+	if (O_IS_MKDIR(open_flags))
+		open_flags &= ~O_CREAT;
+
 	if (dentry->d_name.len > NFS_SERVER(dir)->namelen)
 		return -ENAMETOOLONG;
 
diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
index 88a4a1787ff0..75bf86cc0612 100644
--- a/fs/smb/client/dir.c
+++ b/fs/smb/client/dir.c
@@ -538,6 +538,9 @@ int cifs_atomic_open(struct inode *dir, struct dentry *direntry,
 	if (unlikely(cifs_forced_shutdown(cifs_sb)))
 		return smb_EIO(smb_eio_trace_forced_shutdown);
 
+	if (O_IS_MKDIR(oflags))
+		oflags &= ~O_CREAT;
+
 	/*
 	 * Posix open is only called (at lookup time) for file create now. For
 	 * opens (rather than creates), because we do not know if it is a file
diff --git a/fs/vboxsf/dir.c b/fs/vboxsf/dir.c
index c5bd3271aa96..77ae2f696fd4 100644
--- a/fs/vboxsf/dir.c
+++ b/fs/vboxsf/dir.c
@@ -318,6 +318,9 @@ static int vboxsf_dir_atomic_open(struct inode *parent, struct dentry *dentry,
 	u64 handle;
 	int err;
 
+	if (O_IS_MKDIR(flags))
+		flags &= ~O_CREAT;
+
 	if (d_in_lookup(dentry)) {
 		struct dentry *res = vboxsf_dir_lookup(parent, dentry, 0);
 		if (res || d_really_is_positive(dentry))
-- 
2.55.0

[PATCH v3 11/14] vfs: refuse O_CREAT for directories through a dangling symlink

[Jori Koolstra]

open(O_CREAT) without O_EXCL follows a trailing symlink and, when the
symlink target does not exist, creates it. Refuse to create through a
dangling symlink for directories.

In lookup_open() a negative target reached with nd->depth > 0 was
arrived at by following a trailing symlink; since the dentry is negative
the symlink is dangling. Set create_error to -EEXIST in that case
(matching the errorno returned by mkdir(2).) Reusing the existing
create_error path strips O_CREAT for both the generic and
->atomic_open create paths and only reports the error when the target is
actually negative. Thus opening an existing target through a symlink,
interior symlinks, and O_EXCL (which never follows the trailing link)
are all unaffected.

Suggested-by: Christian Brauner (Amutable) <brauner@kernel.org>
Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/namei.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 76916caa264d..030cac4fbe55 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4546,6 +4546,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 						    dentry, mode, create_dir);
 		else
 			create_error = -EROFS;
+		/* Refuse to create a directory through a dangling (trailing)
+		 * symlink. For regular files this has been allowed historically
+		 * on O_CREAT without O_EXCL. */
+		if (unlikely(nd->depth) && create_dir && !create_error)
+			create_error = -EEXIST;
 	}
 	if (create_error)
 		open_flag &= ~O_CREAT;
-- 
2.55.0

[PATCH v3 12/14] vfs: short-circuit MAY_WRITE access for O_DIRECTORY opens

[Jori Koolstra]

Requesting write access on a directory can never succeed. Rather
than performing a path-walk to determine whether the target is
actually a directory (-EISDIR) or not (-ENOTDIR), or does not exist
(-ENOENT), etc., we short-circuit to -ENOTDIR.

Currently O_WRONLY for directories is only blocked in may_open(),
which happens after we have the inode for the target, so after any
create via O_CREAT|O_DIRECTORY.

The advantage of short-circuiting is that we don't have to add even
more logic to lookup_open() to differentiate -EISDIR/-ENOTDIR. Also,
for filesystems that define atomic_open(), handling this cannot even be
done at the VFS level, as we can't know ahead of calling
->atomic_open() what the result of the lookup is.

Suggested-by: Christian Brauner (Amutable) <brauner@kernel.org>
Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 fs/open.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/open.c b/fs/open.c
index 9121aece78bc..87e8864d9480 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1270,9 +1270,16 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
 
 	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
 
+	/*
+	 * Requesting write access on a directory can never succeed. Rather
+	 * than performing a path-walk to determine whether the target is
+	 * actually a directory (-EISDIR) or not (-ENOTDIR), we short-circuit
+	 * to -ENOTDIR.
+	 */
+	if ((flags & O_DIRECTORY) && !(flags & __O_TMPFILE) && (acc_mode & MAY_WRITE))
+		return -ENOTDIR;
+
 	if (flags & O_CREAT) {
-		if ((flags & O_DIRECTORY) && (acc_mode & MAY_WRITE))
-			return -EISDIR;
 		op->intent |= LOOKUP_CREATE;
 		if (flags & O_EXCL) {
 			op->intent |= LOOKUP_EXCL;
-- 
2.55.0

[PATCH v3 13/14] selftest: fix headers in fclog.c

[Jori Koolstra]

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 tools/testing/selftests/filesystems/fclog.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/testing/selftests/filesystems/fclog.c b/tools/testing/selftests/filesystems/fclog.c
index 551c4a0f395a..593a5136e991 100644
--- a/tools/testing/selftests/filesystems/fclog.c
+++ b/tools/testing/selftests/filesystems/fclog.c
@@ -6,10 +6,8 @@
 
 #include <assert.h>
 #include <errno.h>
+#include <fcntl.h>
 #include <sched.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
 #include <unistd.h>
 #include <sys/mount.h>
 
-- 
2.55.0

[PATCH v3 14/14] selftest: add tests for open*(O_CREAT|O_DIRECTORY)

[Jori Koolstra]

Add some tests for the new valid O_CREAT|O_DIRECTORY flag combination for
open*(2) to test compliance and to showcase its behaviour.

Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
---
 .../testing/selftests/filesystems/.gitignore  |   1 +
 tools/testing/selftests/filesystems/Makefile  |   2 +-
 .../filesystems/open_o_creat_o_dir.c          | 197 ++++++++++++++++++
 3 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/filesystems/open_o_creat_o_dir.c

diff --git a/tools/testing/selftests/filesystems/.gitignore b/tools/testing/selftests/filesystems/.gitignore
index a78f894157de..d779a7945cf8 100644
--- a/tools/testing/selftests/filesystems/.gitignore
+++ b/tools/testing/selftests/filesystems/.gitignore
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0-only
+open_o_creat_o_dir
 dnotify_test
 devpts_pts
 fclog
diff --git a/tools/testing/selftests/filesystems/Makefile b/tools/testing/selftests/filesystems/Makefile
index a7ec2ba2dd83..b60950f8b15c 100644
--- a/tools/testing/selftests/filesystems/Makefile
+++ b/tools/testing/selftests/filesystems/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 CFLAGS += $(KHDR_INCLUDES)
-TEST_GEN_PROGS := devpts_pts file_stressor anon_inode_test kernfs_test fclog
+TEST_GEN_PROGS := open_o_creat_o_dir devpts_pts file_stressor anon_inode_test kernfs_test fclog
 TEST_GEN_PROGS += idmapped_tmpfile
 TEST_GEN_PROGS_EXTENDED := dnotify_test
 
diff --git a/tools/testing/selftests/filesystems/open_o_creat_o_dir.c b/tools/testing/selftests/filesystems/open_o_creat_o_dir.c
new file mode 100644
index 000000000000..df3cdbae2c85
--- /dev/null
+++ b/tools/testing/selftests/filesystems/open_o_creat_o_dir.c
@@ -0,0 +1,197 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <errno.h>
+#include <limits.h>
+#include <fcntl.h>
+
+#include "kselftest_harness.h"
+
+static inline int open_o_creat_o_dir(int dfd, const char *pathname,
+				     mode_t mode, unsigned int flags)
+{
+	return syscall(__NR_openat, dfd, pathname,
+		       flags | O_DIRECTORY | O_CREAT, mode);
+}
+
+#define open_o_creat_o_dir_checked_flags(dfd, pathname, flags) ({	\
+	struct stat __st;						\
+	int __fd = open_o_creat_o_dir(dfd, pathname, S_IRWXU, flags);	\
+	ASSERT_GE(__fd, 0);						\
+	ASSERT_EQ(fstat(__fd, &__st), 0);				\
+	EXPECT_TRUE(S_ISDIR(__st.st_mode));				\
+	__fd;								\
+})
+
+#define open_o_creat_o_dir_checked(dfd, pathname) \
+	open_o_creat_o_dir_checked_flags(dfd, pathname, 0)
+
+FIXTURE(open_o_creat_o_dir) {
+	char dirpath[PATH_MAX];
+	int dfd;
+};
+
+FIXTURE_SETUP(open_o_creat_o_dir)
+{
+	strcpy(self->dirpath, "/tmp/open_o_creat_o_dir_test.XXXXXX");
+	ASSERT_NE(mkdtemp(self->dirpath), NULL);
+	self->dfd = open(self->dirpath, O_DIRECTORY);
+	ASSERT_GE(self->dfd, 0);
+}
+
+FIXTURE_TEARDOWN(open_o_creat_o_dir)
+{
+	close(self->dfd);
+	rmdir(self->dirpath);
+}
+
+/* Does open_o_creat_o_dir return a fd at all? */
+TEST_F(open_o_creat_o_dir, returns_fd)
+{
+	int fd = open_o_creat_o_dir_checked(self->dfd, "newdir");
+	EXPECT_EQ(close(fd), 0);
+	EXPECT_EQ(unlinkat(self->dfd, "newdir", AT_REMOVEDIR), 0);
+}
+
+/* The fd must refer to the directory that was just created. */
+TEST_F(open_o_creat_o_dir, fd_is_created_dir)
+{
+	int fd;
+	struct stat st_via_fd, st_via_path;
+	char path[PATH_MAX];
+
+	fd = open_o_creat_o_dir_checked(self->dfd, "checkdir");
+
+	ASSERT_EQ(fstat(fd, &st_via_fd), 0);
+
+	snprintf(path, sizeof(path), "%s/checkdir", self->dirpath);
+	ASSERT_EQ(stat(path, &st_via_path), 0);
+
+	EXPECT_EQ(st_via_fd.st_ino, st_via_path.st_ino);
+	EXPECT_EQ(st_via_fd.st_dev, st_via_path.st_dev);
+
+	EXPECT_EQ(close(fd), 0);
+	EXPECT_EQ(rmdir(path), 0);
+}
+
+/* Missing parent component must fail with ENOENT. */
+TEST_F(open_o_creat_o_dir, enoent_missing_parent)
+{
+	EXPECT_EQ(open_o_creat_o_dir(self->dfd, "nonexistent/child", S_IRWXU, 0), -1);
+	EXPECT_EQ(errno, ENOENT);
+}
+
+/* An invalid dfd must fail with EBADF. */
+TEST_F(open_o_creat_o_dir, ebadf)
+{
+	EXPECT_EQ(open_o_creat_o_dir(-42, "badfdir", S_IRWXU, 0), -1);
+	EXPECT_EQ(errno, EBADF);
+}
+
+/* A dfd that points to a file (not a directory) must fail with ENOTDIR. */
+TEST_F(open_o_creat_o_dir, enotdir_dfd)
+{
+	int file_fd;
+
+	file_fd = openat(self->dfd, "file",
+			 O_CREAT | O_WRONLY, S_IRWXU);
+	ASSERT_GE(file_fd, 0);
+
+	EXPECT_EQ(open_o_creat_o_dir(file_fd, "subdir", S_IRWXU, 0), -1);
+	EXPECT_EQ(errno, ENOTDIR);
+
+	EXPECT_EQ(close(file_fd), 0);
+	EXPECT_EQ(unlinkat(self->dfd, "file", 0), 0);
+}
+
+/*
+ * O_EXCL together with O_CREAT|O_DIRECTORY must fail with EEXIST when
+ * the target directory already exists.
+ */
+TEST_F(open_o_creat_o_dir, o_excl_eexist)
+{
+	int fd;
+
+	fd = open_o_creat_o_dir_checked_flags(self->dfd, "excldir", O_EXCL);
+	EXPECT_EQ(close(fd), 0);
+
+	EXPECT_EQ(open_o_creat_o_dir(self->dfd, "excldir", S_IRWXU, O_EXCL), -1);
+	EXPECT_EQ(errno, EEXIST);
+
+	EXPECT_EQ(unlinkat(self->dfd, "excldir", AT_REMOVEDIR), 0);
+}
+
+/*
+ * O_CREAT|O_DIRECTORY on a path that already exists as a regular file
+ * must fail with ENOTDIR.
+ */
+TEST_F(open_o_creat_o_dir, existing_file_enotdir)
+{
+	int file_fd;
+
+	file_fd = openat(self->dfd, "regfile",
+			 O_CREAT | O_WRONLY, S_IRWXU);
+	ASSERT_GE(file_fd, 0);
+	EXPECT_EQ(close(file_fd), 0);
+
+	EXPECT_EQ(open_o_creat_o_dir(self->dfd, "regfile", S_IRWXU, 0), -1);
+	EXPECT_EQ(errno, ENOTDIR);
+
+	EXPECT_EQ(unlinkat(self->dfd, "regfile", 0), 0);
+}
+
+/*
+ * O_CREAT|O_DIRECTORY combined with a writable access mode must be
+ * rejected: a directory cannot be opened for writing.
+ */
+TEST_F(open_o_creat_o_dir, rejects_writable_acc_mode)
+{
+	EXPECT_EQ(open_o_creat_o_dir(self->dfd, "rdwrdir", S_IRWXU, O_RDWR), -1);
+	EXPECT_EQ(errno, ENOTDIR);
+	/* Clean up if the kernel created the directory anyway. */
+	unlinkat(self->dfd, "rdwrdir", AT_REMOVEDIR);
+}
+
+/*
+ * openat(O_CREAT) with a trailing slash but without O_DIRECTORY
+ * must fail with EISDIR and must not create anything at the path.
+ */
+TEST_F(open_o_creat_o_dir, trailing_slash_no_o_dir)
+{
+	int fd;
+	struct stat st;
+
+	fd = openat(self->dfd, "trailing/", O_CREAT | O_WRONLY, S_IRWXU);
+	EXPECT_EQ(fd, -1);
+	EXPECT_EQ(errno, EISDIR);
+
+	EXPECT_EQ(fstatat(self->dfd, "trailing", &st, 0), -1);
+	EXPECT_EQ(errno, ENOENT);
+
+	/* Best-effort cleanup in case the kernel left a file behind. */
+	if (fd >= 0)
+		close(fd);
+	unlinkat(self->dfd, "trailing", 0);
+}
+
+/*
+ * The returned fd must be usable as a dfd for further *at() calls.
+ */
+TEST_F(open_o_creat_o_dir, fd_usable_as_dfd)
+{
+	int parent_fd, child_fd;
+	char path[PATH_MAX];
+
+	parent_fd = open_o_creat_o_dir_checked(self->dfd, "parent");
+	child_fd = open_o_creat_o_dir_checked(parent_fd, "child");
+
+	EXPECT_EQ(close(child_fd), 0);
+	EXPECT_EQ(close(parent_fd), 0);
+
+	snprintf(path, sizeof(path), "%s/parent/child", self->dirpath);
+	EXPECT_EQ(rmdir(path), 0);
+	snprintf(path, sizeof(path), "%s/parent", self->dirpath);
+	EXPECT_EQ(rmdir(path), 0);
+}
+
+TEST_HARNESS_MAIN
-- 
2.55.0

Re: [PATCH v3 01/14] fs/namei.c: use trailing_slashes()

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> There are several places in fs/namei.c that can use the
> trailing_slashes() function to improve intent.

I think it would help to state here that trailing_slashes() is changed
to take a qstr instead of a nameidata as that is useful in more places.

Reviewed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown


> 
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> ---
>  fs/namei.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 19ce43c9a6e6..a8890bc0ae21 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2781,9 +2781,14 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
>  	return s;
>  }
>  
> +static inline bool trailing_slashes(const struct qstr last)
> +{
> +	return (bool)last.name[last.len];
> +}
> +
>  static inline const char *lookup_last(struct nameidata *nd)
>  {
> -	if (nd->last_type == LAST_NORM && nd->last.name[nd->last.len])
> +	if (nd->last_type == LAST_NORM && trailing_slashes(nd->last))
>  		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
>  
>  	return walk_component(nd, WALK_TRAILING);
> @@ -4521,17 +4526,12 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  	return ERR_PTR(error);
>  }
>  
> -static inline bool trailing_slashes(struct nameidata *nd)
> -{
> -	return (bool)nd->last.name[nd->last.len];
> -}
> -
>  static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
>  {
>  	struct dentry *dentry;
>  
>  	if (open_flag & O_CREAT) {
> -		if (trailing_slashes(nd))
> +		if (trailing_slashes(nd->last))
>  			return ERR_PTR(-EISDIR);
>  
>  		/* Don't bother on an O_EXCL create */
> @@ -4539,7 +4539,7 @@ static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
>  			return NULL;
>  	}
>  
> -	if (trailing_slashes(nd))
> +	if (trailing_slashes(nd->last))
>  		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
>  
>  	dentry = lookup_fast(nd);
> @@ -4950,7 +4950,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
>  	 * Do the final lookup.  Suppress 'create' if there is a trailing
>  	 * '/', and a directory wasn't requested.
>  	 */
> -	if (last.name[last.len] && !want_dir)
> +	if (trailing_slashes(last) && !want_dir)
>  		create_flags &= ~LOOKUP_CREATE;
>  	dentry = start_dirop(path->dentry, &last, reval_flag | create_flags);
>  	if (IS_ERR(dentry))
> @@ -5569,7 +5569,7 @@ int filename_unlinkat(int dfd, struct filename *name)
>  		goto exit_drop_write;
>  
>  	/* Why not before? Because we want correct error value */
> -	if (unlikely(last.name[last.len])) {
> +	if (unlikely(trailing_slashes(last))) {
>  		if (d_is_dir(dentry))
>  			error = -EISDIR;
>  		else
> @@ -6171,16 +6171,16 @@ int filename_renameat2(int olddfd, struct filename *from,
>  	if (flags & RENAME_EXCHANGE) {
>  		if (!d_is_dir(rd.new_dentry)) {
>  			error = -ENOTDIR;
> -			if (new_last.name[new_last.len])
> +			if (trailing_slashes(new_last))
>  				goto exit_unlock;
>  		}
>  	}
>  	/* unless the source is a directory trailing slashes give -ENOTDIR */
>  	if (!d_is_dir(rd.old_dentry)) {
>  		error = -ENOTDIR;
> -		if (old_last.name[old_last.len])
> +		if (trailing_slashes(old_last))
>  			goto exit_unlock;
> -		if (!(flags & RENAME_EXCHANGE) && new_last.name[new_last.len])
> +		if (!(flags & RENAME_EXCHANGE) && trailing_slashes(new_last))
>  			goto exit_unlock;
>  	}
>  
> -- 
> 2.55.0
> 
> 

Re: [PATCH v3 04/14] vfs: call audit_inode_child() in lookup_open() on failure too

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> audit_inode_child() is called in may_create_dentry() so that failed
> filesystem operations still register an audit entry. On success, the
> entry is overwritten when, for instance, fsnotify_create() is called.
> This is the calling convention in vfs_create() and vfs_mkdir().
> In lookup_open(), however, when atomic_open() should have created a
> file but didn't, no call to audit_inode_child() is made. The same is
> true for the regular ->create() path.
> 
> Fix the calling of audit_inode_child() in lookup_open() to match the
> vfs_create() path. For the ->atomic_open() filesystems this logic has
> been pushed into atomic_open(). This function is also reordered a bit
> to make the case distinction of the possible returns from
> ->atomic_open() more explicit (i.e. finish_open() or finish_no_open()).
> 
> When retrying delegation breaking, audit_inode_child() could be called
> more than once, but this is OK because those entries are reused.
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> ---
>  fs/namei.c | 75 ++++++++++++++++++++++++++++++++----------------------
>  1 file changed, 45 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index ee47d3f5159f..956adcd14c4a 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4369,10 +4369,11 @@ static int may_o_create(struct mnt_idmap *idmap,
>   */
>  static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
>  				  struct file *file,
> -				  int open_flag, umode_t mode)
> +				  int open_flag, umode_t mode, bool create_error)
>  {
>  	struct dentry *const DENTRY_NOT_SET = (void *) -1UL;
>  	struct inode *dir =  path->dentry->d_inode;
> +	bool failed_create = false;
>  	int error;
>  
>  	file->__f_path.dentry = DENTRY_NOT_SET;
> @@ -4382,23 +4383,33 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
>  	d_lookup_done(dentry);
>  	if (!error) {
>  		if (file->f_mode & FMODE_OPENED) {
> -			if (unlikely(dentry != file->f_path.dentry)) {
> +			// finish_open() called
> +			struct dentry *opened = file->f_path.dentry;
> +			if (unlikely(opened != dentry)) {
>  				dput(dentry);
> -				dentry = dget(file->f_path.dentry);
> +				dentry = dget(opened);
>  			}
> -		} else if (WARN_ON(file->f_path.dentry == DENTRY_NOT_SET)) {
> -			error = -EIO;
> -		} else {
> -			if (file->f_path.dentry) {
> +		} else if (likely(file->f_path.dentry != DENTRY_NOT_SET)) {
> +			// finish_no_open() called
> +			struct dentry *replaced = file->f_path.dentry;
> +			if (replaced) {
>  				dput(dentry);
> -				dentry = file->f_path.dentry;
> +				dentry = replaced;
>  			}
>  			if (unlikely(d_is_negative(dentry)))
>  				error = -ENOENT;

I think this can now be
   if (unlikely(d_is_negative(dentry))
         error = create_error ?: -ENOENT;
or something similar if people don't like "?:".

> +			if (error && create_error) // should have created, but errored before
> +				failed_create = true;

Why have 'failed_create' ? Why not just put the audit_inode_child() like
here?

There rest of the patch look pretty good.

Thanks,
NeilBrown


> +		} else {
> +			const char *fsname = dentry->d_sb->s_type->name;
> +			WARN(1, "%s: ->atomic_open() left file->f_path.dentry unset!\n", fsname);
> +			error = -EIO;
>  		}
>  	}
>  
>  	if (error) {
> +		if (failed_create)
> +			audit_inode_child(dir, dentry, AUDIT_TYPE_CHILD_CREATE);
>  		dput(dentry);
>  		dentry = ERR_PTR(error);
>  	} else {
> @@ -4462,7 +4473,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		dentry = NULL;
>  	}
>  	if (dentry->d_inode) {
> -		/* Cached positive dentry: will open in f_op->open */
> +		/* Cached positive dentry: will open in do_open(). */
>  		return dentry;
>  	}
>  
> @@ -4496,7 +4507,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  	if (dir_inode->i_op->atomic_open) {
>  		if (nd->flags & LOOKUP_DIRECTORY)
>  			open_flag |= O_DIRECTORY;
> -		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
> +		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode, create_error);
>  		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
>  			dentry = ERR_PTR(create_error);
>  		return dentry;
> @@ -4515,33 +4526,37 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  			dentry = res;
>  		}
>  	}
> +	if (dentry->d_inode || !(op->open_flag & O_CREAT)) {
> +		/* No need to create a file. If lookup returned a positive
> +		 * dentry, the file will be opened in do_open(). */
> +		return dentry;
> +	}
> +
> +	/* Negative dentry with O_CREAT flag set */
> +	audit_inode_child(dir->d_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
>  
> -	if (unlikely(create_error) && !dentry->d_inode) {
> +	if (unlikely(create_error)) {
> +		/* Should have done a create, but we already errored */
>  		error = create_error;
>  		goto out_dput;
>  	}
>  
> -	/* Negative dentry, just create the file */
> -	if (!dentry->d_inode && (open_flag & O_CREAT)) {
> -		/* but break the directory lease first! */
> -		error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
> -		if (error)
> -			goto out_dput;
> +	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
> +	if (error)
> +		goto out_dput;
>  
> -		file->f_mode |= FMODE_CREATED;
> -		audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> -		if (!dir_inode->i_op->create) {
> -			error = -EACCES;
> -			goto out_dput;
> -		}
> +	file->f_mode |= FMODE_CREATED;
> +	if (!dir_inode->i_op->create) {
> +		error = -EACCES;
> +		goto out_dput;
> +	}
>  
> -		error = dir_inode->i_op->create(idmap, dir_inode, dentry,
> -						mode, open_flag & O_EXCL);
> -		if (error)
> -			goto out_dput;
> +	error = dir_inode->i_op->create(idmap, dir_inode, dentry,
> +					mode, open_flag & O_EXCL);
> +	if (error)
> +		goto out_dput;
>  
> -		fsnotify_create(dir_inode, dentry);
> -	}
> +	fsnotify_create(dir_inode, dentry);
>  
>  	return dentry;
>  
> @@ -5071,7 +5086,7 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
>  
>  		/* atomic_open will dput(dentry) on error */
>  		dget(orig_dentry);
> -		dentry = atomic_open(path, dentry, file, flags, mode);
> +		dentry = atomic_open(path, dentry, file, flags, mode, create_error);
>  		error = PTR_ERR_OR_ZERO(dentry);
>  
>  		if (IS_ERR(dentry))
> -- 
> 2.55.0
> 
> 

Re: [PATCH v3 05/14] fs/namei.c: update docstring of atomic_open()

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> The docstring of atomic_open() contains several errors:
>   - It does not return 0 if successful.
>   - path is not updated
> 
> Fix those and be more explicit about when FMODE_OPENED and FMODE_CREATED
> are set.
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> ---
>  fs/namei.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 956adcd14c4a..88d56a470832 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4355,17 +4355,24 @@ static int may_o_create(struct mnt_idmap *idmap,
>  }
>  
>  /*
> - * Attempt to atomically look up, create and open a file from a negative
> - * dentry.
> - *
> - * Returns 0 if successful.  The file will have been created and attached to
> - * @file by the filesystem calling finish_open().
> - *
> - * If the file was looked up only or didn't need creating, FMODE_OPENED won't
> - * be set.  The caller will need to perform the open themselves.  @path will
> - * have been updated to point to the new dentry.  This may be negative.
> - *
> - * Returns an error code otherwise.
> + * atomic_open() - attempt to atomically look up, create and open a file
> + * from a negative dentry.
> + * @path:          parent directory path
> + * @dentry:        child to ->atomic_open()
> + * @open_flag:     open flags
> + * @mode:          create mode
> + * @create_error:  return value from may_o_create()
> + *

 "If a non-error dentry is returned then : "
> + * If FMODE_OPENED is set, the file will have been attached to @file by the
> + * filesystem calling finish_open(). If FMODE_OPENED isn't set, the
> + * filesystem instead called finish_no_open() and the caller will need to
> + * perform the open themselves.
> + *
> + * FMODE_CREATED is set when the call to ->atomic_open() actually created
> + * the file.
> + *
> + * Returns the opened/looked-up dentry on success or ERR_PTR(-E) on failure.
> + * On error, atomic_open() consumes @dentry.

The rest looks good.  Thanks for doing this.

NeilBrown


>   */
>  static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
>  				  struct file *file,
> -- 
> 2.55.0
> 
> 

Re: [PATCH v3 07/14] vfs: move ->create check in lookup_open() to before try_break_deleg()

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> The i_op->create check in lookup_open() takes place after the
> try_break_deleg() call. This does not match the order when doing a
> regular file create via mknod(2). There the call order is:
> 
> 	filename_mknodat()
> 	vfs_create()
> 	i_op->create check
> 	try_break_deleg()
> 
> Move the i_op->create check to before try_break_deleg() in
> lookup_open().
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>

Reviwed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown

> ---
>  fs/namei.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index daf31845e3bb..6601460d2772 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4548,15 +4548,15 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		goto out_dput;
>  	}
>  
> -	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
> -	if (error)
> -		goto out_dput;
> -
>  	if (!dir_inode->i_op->create) {
>  		error = -EACCES;
>  		goto out_dput;
>  	}
>  
> +	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
> +	if (error)
> +		goto out_dput;
> +
>  	error = dir_inode->i_op->create(idmap, dir_inode, dentry,
>  					mode, open_flag & O_EXCL);
>  	if (error)
> -- 
> 2.55.0
> 
> 

Re: [PATCH v3 08/14] vfs: lookup_open(): use vfs_create_no_perm()

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> We can replace the code in the no create_error/negative dentry found
> from lookup case in lookup_open() with the vfs_create_no_perm() helper.
> 
> It is safe here to always pass excl=true to ->create via
> vfs_create_no_perm(). Local filesystems always ignore the excl flag,
> and cluster filesystems implement ->atomic_open, so that the ->create
> call is never reached in lookup_open().
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> ---
>  fs/namei.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 6601460d2772..eaa0ade4a1b0 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4553,16 +4553,10 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		goto out_dput;
>  	}
>  
> -	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
> +	error = vfs_create_no_perm(idmap, dentry, mode, delegated_inode);
>  	if (error)
>  		goto out_dput;
>  
> -	error = dir_inode->i_op->create(idmap, dir_inode, dentry,
> -					mode, open_flag & O_EXCL);
> -	if (error)
> -		goto out_dput;
> -
> -	fsnotify_create(dir_inode, dentry);
>  	file->f_mode |= FMODE_CREATED;
>  
>  	return dentry;
> -- 
> 2.55.0
> 
> 

Reviewed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown

Re: [PATCH v3 09/14] vfs: add O_CREAT|O_DIRECTORY to open*(2)

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> Currently there is no way to race-freely create and open a directory.
> For regular files we have open(O_CREAT) for creating a new file inode,
> and returning a pinning fd to it. The lack of such functionality for
> directories means that when populating a directory tree there's always
> a race involved: the inodes first need to be created, and then opened
> to adjust their permissions/ownership/labels/timestamps/acls/xattrs/...,
> but in the time window between the creation and the opening they might
> be replaced by something else.
> 
> Addressing this race without proper APIs is possible (by immediately
> fstat()ing what was opened, to verify that it has the right inode type),
> but difficult to get right. Hence, adding support for a new flag combo
> O_CREAT|O_DIRECTORY to open*(2) that creates a directory (if it does not
> exist already) and returns an O_DIRECTORY fd is very useful.
> 
> Historically, the O_CREAT|O_DIRECTORY behaviour was to return ENOTDIR if
> a regular file exists at the open path; EISDIR if a directory exists at
> the path; and to create a regular file if no file exists at the path.
> This behaviour changed accidentally with 973d4b73fbaf ("do_last(): rejoin
> the common path even earlier in FMODE_{OPENED,CREATED} case") causing
> ENOTDIR to return in the last case while still creating the file. As
> this change was not detected for a long time, Brauner proposed to adopt
> the more consistent NetBSD behaviour, i.e. to return EINVAL on the the
> O_CREAT|O_DIRECTORY combination. This change was applied in 43b450632676
> ("open: return EINVAL for O_DIRECTORY | O_CREAT") in March, 2023. As
> the EINVAL behaviour has been in the kernel for about 3 year now, no
> rollback is expected as a result of userspace reliance on old
> behaviour, leaving us free to reassign the O_CREAT|O_DIRECTORY semantics.
> 
> O_CREAT|O_DIRECTORY is made to reduce to a lookup on ->atomic_open()
> filesystems. This is implemented by stripping the O_CREAT bit. The other
> option of simply returning -EINVAL leads to inconsistent behaviour:
> before ->atomic_open() is called in lookup_open(), the dcache is
> queried. So returning -EINVAL there would make open(O_CREAT|O_DIRECTORY)
> dependent on the cache state of the dentry. By stripping the O_CREAT
> bit, lookup is consistently performed on ->atomic_open() filesystems.
> 
> There is no separate sysctl for directory creation implemented currently.
> Therefore, for the S_ISDIR case, disabling sysctl_protected_regular is
> not enough to allow creating a directory in a sticky folder, because that
> may surprise users not expecting that O_CREAT|O_DIRECTORY is possible on
> newer kernels.
> 
> This feature idea (and some of its description) is taken from the
> UAPI group:
> https://github.com/uapi-group/kernel-features?tab=readme-ov-file#race-free-creation-and-opening-of-non-file-inodes
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> ---
>  fs/namei.c            | 86 +++++++++++++++++++++++++++++++++++--------
>  fs/open.c             | 25 +++++++------
>  include/linux/fcntl.h |  6 +++
>  3 files changed, 90 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index eaa0ade4a1b0..5113ac986f50 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1382,13 +1382,12 @@ int may_linkat(struct mnt_idmap *idmap, const struct path *link)
>  
>  /**
>   * may_create_in_sticky - Check whether an O_CREAT open in a sticky directory
> - *			  should be allowed, or not, on files that already
> - *			  exist.
> + *			  should be allowed, or not

Why did you remove that clause?  I think it is important.
Maybe change "files" to "objects" or "files/directories"

>   * @idmap: idmap of the mount the inode was found from
>   * @nd: nameidata pathwalk data
>   * @inode: the inode of the file to open
>   *
> - * Block an O_CREAT open of a FIFO (or a regular file) when:
> + * Block an O_CREAT open of a FIFO (or a regular file/directory) when:
>   *   - sysctl_protected_fifos (or sysctl_protected_regular) is enabled
>   *   - the file already exists
>   *   - we are in a sticky directory
> @@ -1416,6 +1415,14 @@ static int may_create_in_sticky(struct mnt_idmap *idmap, struct nameidata *nd,
>  	if (likely(!(dir_mode & S_ISVTX)))
>  		return 0;
>  
> +	/*
> +	 * There is no separate sysctl for directory creation in sticky
> +	 * folders. Therefore, for the S_ISDIR case, disabling
> +	 * sysctl_protected_regular is not enough to allow creating a
> +	 * directory in a sticky folder, because that may surprise users
> +	 * not expecting that O_CREAT|O_DIRECTORY is possible on newer
> +	 * kernels.
> +	 */
>  	if (S_ISREG(inode->i_mode) && !sysctl_protected_regular)
>  		return 0;
>  
> @@ -1447,6 +1454,12 @@ static int may_create_in_sticky(struct mnt_idmap *idmap, struct nameidata *nd,
>  					      "sticky_create_regular");
>  			return -EACCES;
>  		}
> +
> +		if (S_ISDIR(inode->i_mode)) {
> +			audit_log_path_denied(AUDIT_ANOM_CREAT,
> +					      "sticky_create_dir");
> +			return -EACCES;
> +		}
>  	}
>  
>  	return 0;
> @@ -4337,21 +4350,40 @@ static inline int open_to_namei_flags(int flag)
>  
>  static int may_o_create(struct mnt_idmap *idmap,
>  			const struct path *dir, struct dentry *dentry,
> -			umode_t mode)
> +			umode_t mode, bool create_dir)
>  {
> -	int error = security_path_mknod(dir, dentry, mode, 0);
> +	struct inode *dir_inode = dir->dentry->d_inode;
> +	int error;
> +
> +	if (create_dir)
> +		error = security_path_mkdir(dir, dentry, mode);
> +	else
> +		error = security_path_mknod(dir, dentry, mode, 0);
>  	if (error)
>  		return error;
>  
>  	if (!fsuidgid_has_mapping(dir->dentry->d_sb, idmap))
>  		return -EOVERFLOW;
>  
> -	error = inode_permission(idmap, dir->dentry->d_inode,
> -				 MAY_WRITE | MAY_EXEC);
> +	error = inode_permission(idmap, dir_inode, MAY_WRITE | MAY_EXEC);
>  	if (error)
>  		return error;
>  
> -	return security_inode_create(dir->dentry->d_inode, dentry, mode);
> +	if (create_dir)
> +		error = security_inode_mkdir(dir_inode, dentry, mode);
> +	else
> +		error = security_inode_create(dir_inode, dentry, mode);
> +
> +	return error;
> +}
> +
> +static inline umode_t o_create_mode(struct mnt_idmap *idmap,
> +		const struct inode *dir, umode_t mode, bool create_dir)
> +{
> +	if (create_dir)
> +		return vfs_prepare_mode(idmap, dir, mode, S_IRWXUGO | S_ISVTX, 0);
> +	else
> +		return vfs_prepare_mode(idmap, dir, mode, S_IALLUGO, S_IFREG);
>  }

I"m still not keen on passing 'bool' args.  Maybe you could pass
open_flags and use O_IS_MKDIR()?  But I won't fight it.

>  
>  /*
> @@ -4385,6 +4417,7 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
>  
>  	file->__f_path.dentry = DENTRY_NOT_SET;
>  	file->__f_path.mnt = path->mnt;
> +
>  	error = dir->i_op->atomic_open(dir, dentry, file,
>  				       open_to_namei_flags(open_flag), mode);
>  	d_lookup_done(dentry);
> @@ -4429,6 +4462,10 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
>  	return dentry;
>  }
>  
> +static inline
> +struct dentry *vfs_mkdir_no_perm(struct mnt_idmap *, struct inode *,
> +				 struct dentry *, umode_t,
> +				 struct delegated_inode *);
>  /*
>   * Look up and maybe create and open the last component.
>   *
> @@ -4455,10 +4492,14 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  	struct dentry *dentry;
>  	int error, create_error = 0;
>  	umode_t mode = op->mode;
> +	bool create_dir = O_IS_MKDIR(open_flag);
>  
>  	if (unlikely(IS_DEADDIR(dir_inode)))
>  		return ERR_PTR(-ENOENT);
>  
> +	if (O_IS_MKDIR(open_flag) && dir_inode->i_op->atomic_open)

If you keep "create_dir", you should use it here.

> +		open_flag &= ~O_CREAT;
> +
>  	file->f_mode &= ~FMODE_CREATED;
>  	dentry = d_lookup(dir, &nd->last);
>  	for (;;) {
> @@ -4502,10 +4543,10 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  	if (open_flag & O_CREAT) {
>  		if (open_flag & O_EXCL)
>  			open_flag &= ~O_TRUNC;
> -		mode = vfs_prepare_mode(idmap, dir->d_inode, mode, mode, mode);
> +		mode = o_create_mode(idmap, dir_inode, mode, create_dir);
>  		if (likely(got_write))
>  			create_error = may_o_create(idmap, &nd->path,
> -						    dentry, mode);
> +						    dentry, mode, create_dir);
>  		else
>  			create_error = -EROFS;
>  	}
> @@ -4548,12 +4589,24 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		goto out_dput;
>  	}
>  
> -	if (!dir_inode->i_op->create) {
> +	if ((create_dir && !dir_inode->i_op->mkdir)
> +	    || (!create_dir && !dir_inode->i_op->create)) {
>  		error = -EACCES;
>  		goto out_dput;
>  	}
>  
> -	error = vfs_create_no_perm(idmap, dentry, mode, delegated_inode);
> +	if (create_dir) {
> +		struct dentry *res = vfs_mkdir_no_perm(idmap, dir_inode, dentry,
> +						       mode, delegated_inode);
> +		if (IS_ERR(res)) {
> +			error = PTR_ERR(res);
> +		} else {
> +			error = 0;
> +			dentry = res;
> +		}
> +	} else {
> +		error = vfs_create_no_perm(idmap, dentry, mode, delegated_inode);
> +	}
>  	if (error)
>  		goto out_dput;
>  
> @@ -4571,7 +4624,7 @@ static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
>  	struct dentry *dentry;
>  
>  	if (open_flag & O_CREAT) {
> -		if (trailing_slashes(nd->last))
> +		if (trailing_slashes(nd->last) && !(open_flag & O_DIRECTORY))
>  			return ERR_PTR(-EISDIR);
>  
>  		/* Don't bother on an O_EXCL create */
> @@ -4642,7 +4695,7 @@ static const char *open_last_lookups(struct nameidata *nd,
>  		 */
>  	}
>  	if (open_flag & O_CREAT)
> -		inode_lock(dir->d_inode);
> +		inode_lock_nested(dir->d_inode, I_MUTEX_PARENT);
>  	else
>  		inode_lock_shared(dir->d_inode);
>  
> @@ -4705,8 +4758,9 @@ static int do_open(struct nameidata *nd,
>  	if (open_flag & O_CREAT) {
>  		if ((open_flag & O_EXCL) && !(file->f_mode & FMODE_CREATED))
>  			return -EEXIST;
> -		if (d_is_dir(nd->path.dentry))
> +		if (!(open_flag & O_DIRECTORY) && d_is_dir(nd->path.dentry))
>  			return -EISDIR;
> +
>  		error = may_create_in_sticky(idmap, nd,
>  					     d_backing_inode(nd->path.dentry));
>  		if (unlikely(error))
> @@ -5081,7 +5135,7 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
>  		path->dentry = dir;
>  		mode = vfs_prepare_mode(idmap, dir_inode, mode, S_IALLUGO, S_IFREG);
>  
> -		create_error = may_o_create(idmap, path, dentry, mode);
> +		create_error = may_o_create(idmap, path, dentry, mode, /*create_dir=*/ false);
>  		if (create_error)
>  			flags &= ~O_CREAT;
>  
> diff --git a/fs/open.c b/fs/open.c
> index 408925d7bd0b..9121aece78bc 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1190,29 +1190,30 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
>  	if (WILL_CREATE(flags)) {
>  		if (how->mode & ~S_IALLUGO)
>  			return -EINVAL;
> -		op->mode = how->mode | S_IFREG;
> +		if (O_IS_MKDIR(flags))
> +			op->mode = how->mode | S_IFDIR;
> +		else
> +			op->mode = how->mode | S_IFREG;
>  	} else {
>  		if (how->mode != 0)
>  			return -EINVAL;
>  		op->mode = 0;
>  	}
>  
> -	/*
> -	 * Block bugs where O_DIRECTORY | O_CREAT created regular files.
> -	 * Note, that blocking O_DIRECTORY | O_CREAT here also protects
> -	 * O_TMPFILE below which requires O_DIRECTORY being raised.
> -	 */
> -	if ((flags & (O_DIRECTORY | O_CREAT)) == (O_DIRECTORY | O_CREAT))
> -		return -EINVAL;
> -
>  	/* Now handle the creative implementation of O_TMPFILE. */
>  	if (flags & __O_TMPFILE) {
>  		/*
>  		 * In order to ensure programs get explicit errors when trying
>  		 * to use O_TMPFILE on old kernels we enforce that O_DIRECTORY
> -		 * is raised alongside __O_TMPFILE.
> +		 * is raised alongside __O_TMPFILE, but without O_CREAT. The
> +		 * reason for disallowing O_CREAT|O_TMPFILE is that
> +		 * O_DIRECTORY|O_CREAT used to work and created a regular file
> +		 * if nothing existed at the open path. Hence, allowing the
> +		 * combination would have caused O_CREAT|O_TMPFILE to create a
> +		 * regular (non-temporary) file on old kernels, while the caller
> +		 * would believe they created an actual O_TMPFILE.
>  		 */
> -		if (!(flags & O_DIRECTORY))
> +		if (!(flags & O_DIRECTORY) || (flags & O_CREAT))
>  			return -EINVAL;
>  		if (!(acc_mode & MAY_WRITE))
>  			return -EINVAL;
> @@ -1270,6 +1271,8 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
>  	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
>  
>  	if (flags & O_CREAT) {
> +		if ((flags & O_DIRECTORY) && (acc_mode & MAY_WRITE))
> +			return -EISDIR;
>  		op->intent |= LOOKUP_CREATE;
>  		if (flags & O_EXCL) {
>  			op->intent |= LOOKUP_EXCL;
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index 6ad6b9e7a226..6b75277be559 100644
> --- a/include/linux/fcntl.h
> +++ b/include/linux/fcntl.h
> @@ -30,6 +30,12 @@
>   */
>  #define __O_REGULAR		(1 << 30)
>  
> +#define O_MKDIR_MASK	(O_CREAT | O_DIRECTORY)
> +static inline bool O_IS_MKDIR(unsigned flags)
> +{
> +	return (flags & O_MKDIR_MASK) == O_MKDIR_MASK;
> +}
> +
>  /* List of all valid flags for the how->resolve argument: */
>  #define VALID_RESOLVE_FLAGS \
>  	(RESOLVE_NO_XDEV | RESOLVE_NO_MAGICLINKS | RESOLVE_NO_SYMLINKS | \
> -- 
> 2.55.0
> 
> 

The rest looks good.

Thanks,
NeilBrown

Re: [PATCH v3 10/14] vfs: move O_IS_MKDIR check from lookup_open() into individual filesystems

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> Individual filesystems that implement ->atomic_open() need to get the
> chance to implement O_CREAT|O_DIRECTORY or not, rather than decide
> this at the VFS level in lookup_open().
> 
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>

This look straight forward.

Reviewed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown


> ---
>  fs/9p/vfs_inode.c      | 3 +++
>  fs/9p/vfs_inode_dotl.c | 3 +++
>  fs/ceph/file.c         | 3 +++
>  fs/fuse/dir.c          | 3 +++
>  fs/gfs2/inode.c        | 3 +++
>  fs/namei.c             | 3 ---
>  fs/nfs/dir.c           | 6 ++++++
>  fs/smb/client/dir.c    | 3 +++
>  fs/vboxsf/dir.c        | 3 +++
>  9 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 5783d0336f96..bb555e5ba261 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -777,6 +777,9 @@ v9fs_vfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  	struct inode *inode;
>  	int p9_omode;
>  
> +	if (O_IS_MKDIR(flags))
> +		flags &= ~O_CREAT;
> +
>  	if (d_in_lookup(dentry)) {
>  		struct dentry *res = v9fs_vfs_lookup(dir, dentry, 0);
>  		if (res || d_really_is_positive(dentry))
> diff --git a/fs/9p/vfs_inode_dotl.c b/fs/9p/vfs_inode_dotl.c
> index f7396d20cb6c..ff3facb420c2 100644
> --- a/fs/9p/vfs_inode_dotl.c
> +++ b/fs/9p/vfs_inode_dotl.c
> @@ -239,6 +239,9 @@ v9fs_vfs_atomic_open_dotl(struct inode *dir, struct dentry *dentry,
>  	struct v9fs_session_info *v9ses;
>  	struct posix_acl *pacl = NULL, *dacl = NULL;
>  
> +	if (O_IS_MKDIR(flags))
> +		flags &= ~O_CREAT;
> +
>  	if (d_in_lookup(dentry)) {
>  		struct dentry *res = v9fs_vfs_lookup(dir, dentry, 0);
>  		if (res || d_really_is_positive(dentry))
> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> index 71161f2b2151..e79abdbd9f61 100644
> --- a/fs/ceph/file.c
> +++ b/fs/ceph/file.c
> @@ -811,6 +811,9 @@ int ceph_atomic_open(struct inode *dir, struct dentry *dentry,
>  	      dir, ceph_vinop(dir), dentry, dentry,
>  	      d_unhashed(dentry) ? "unhashed" : "hashed", flags, mode);
>  
> +	if (O_IS_MKDIR(flags))
> +		flags &= ~O_CREAT;
> +
>  	if (dentry->d_name.len > NAME_MAX)
>  		return -ENAMETOOLONG;
>  
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 0e2a1039fa43..e01869565d5e 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -935,6 +935,9 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  	struct mnt_idmap *idmap = file_mnt_idmap(file);
>  	struct fuse_conn *fc = get_fuse_conn(dir);
>  
> +	if (O_IS_MKDIR(flags))
> +		flags &= ~O_CREAT;
> +
>  	if (fuse_is_bad(dir))
>  		return -EIO;
>  
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 8a77794bbd4a..9859ae0341a9 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -1387,6 +1387,9 @@ static int gfs2_atomic_open(struct inode *dir, struct dentry *dentry,
>  {
>  	bool excl = !!(flags & O_EXCL);
>  
> +	if (O_IS_MKDIR(flags))
> +		flags &= ~O_CREAT;
> +
>  	if (d_in_lookup(dentry)) {
>  		struct dentry *d = __gfs2_lookup(dir, dentry, file);
>  		if (file->f_mode & FMODE_OPENED) {
> diff --git a/fs/namei.c b/fs/namei.c
> index 5113ac986f50..76916caa264d 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4497,9 +4497,6 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  	if (unlikely(IS_DEADDIR(dir_inode)))
>  		return ERR_PTR(-ENOENT);
>  
> -	if (O_IS_MKDIR(open_flag) && dir_inode->i_op->atomic_open)
> -		open_flag &= ~O_CREAT;
> -
>  	file->f_mode &= ~FMODE_CREATED;
>  	dentry = d_lookup(dir, &nd->last);
>  	for (;;) {
> diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c
> index c7b723c18620..c815d9aa9b69 100644
> --- a/fs/nfs/dir.c
> +++ b/fs/nfs/dir.c
> @@ -2121,6 +2121,9 @@ int nfs_atomic_open(struct inode *dir, struct dentry *dentry,
>  	dfprintk(VFS, "NFS: atomic_open(%s/%llu), %pd\n",
>  			dir->i_sb->s_id, dir->i_ino, dentry);
>  
> +	if (O_IS_MKDIR(open_flags))
> +		open_flags &= ~O_CREAT;
> +
>  	err = nfs_check_flags(open_flags);
>  	if (err)
>  		return err;
> @@ -2313,6 +2316,9 @@ int nfs_atomic_open_v23(struct inode *dir, struct dentry *dentry,
>  	 */
>  	int error = 0;
>  
> +	if (O_IS_MKDIR(open_flags))
> +		open_flags &= ~O_CREAT;
> +
>  	if (dentry->d_name.len > NFS_SERVER(dir)->namelen)
>  		return -ENAMETOOLONG;
>  
> diff --git a/fs/smb/client/dir.c b/fs/smb/client/dir.c
> index 88a4a1787ff0..75bf86cc0612 100644
> --- a/fs/smb/client/dir.c
> +++ b/fs/smb/client/dir.c
> @@ -538,6 +538,9 @@ int cifs_atomic_open(struct inode *dir, struct dentry *direntry,
>  	if (unlikely(cifs_forced_shutdown(cifs_sb)))
>  		return smb_EIO(smb_eio_trace_forced_shutdown);
>  
> +	if (O_IS_MKDIR(oflags))
> +		oflags &= ~O_CREAT;
> +
>  	/*
>  	 * Posix open is only called (at lookup time) for file create now. For
>  	 * opens (rather than creates), because we do not know if it is a file
> diff --git a/fs/vboxsf/dir.c b/fs/vboxsf/dir.c
> index c5bd3271aa96..77ae2f696fd4 100644
> --- a/fs/vboxsf/dir.c
> +++ b/fs/vboxsf/dir.c
> @@ -318,6 +318,9 @@ static int vboxsf_dir_atomic_open(struct inode *parent, struct dentry *dentry,
>  	u64 handle;
>  	int err;
>  
> +	if (O_IS_MKDIR(flags))
> +		flags &= ~O_CREAT;
> +
>  	if (d_in_lookup(dentry)) {
>  		struct dentry *res = vboxsf_dir_lookup(parent, dentry, 0);
>  		if (res || d_really_is_positive(dentry))
> -- 
> 2.55.0
> 
> 

Re: [PATCH v3 11/14] vfs: refuse O_CREAT for directories through a dangling symlink

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> open(O_CREAT) without O_EXCL follows a trailing symlink and, when the
> symlink target does not exist, creates it. Refuse to create through a
> dangling symlink for directories.
> 
> In lookup_open() a negative target reached with nd->depth > 0 was
> arrived at by following a trailing symlink; since the dentry is negative
> the symlink is dangling. Set create_error to -EEXIST in that case
> (matching the errorno returned by mkdir(2).) Reusing the existing
> create_error path strips O_CREAT for both the generic and
> ->atomic_open create paths and only reports the error when the target is
> actually negative. Thus opening an existing target through a symlink,
> interior symlinks, and O_EXCL (which never follows the trailing link)
> are all unaffected.
> 
> Suggested-by: Christian Brauner (Amutable) <brauner@kernel.org>
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> ---
>  fs/namei.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 76916caa264d..030cac4fbe55 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4546,6 +4546,11 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  						    dentry, mode, create_dir);
>  		else
>  			create_error = -EROFS;
> +		/* Refuse to create a directory through a dangling (trailing)
> +		 * symlink. For regular files this has been allowed historically
> +		 * on O_CREAT without O_EXCL. */
> +		if (unlikely(nd->depth) && create_dir && !create_error)
> +			create_error = -EEXIST;
>  	}
>  	if (create_error)
>  		open_flag &= ~O_CREAT;
> -- 
> 2.55.0
> 
> 

Reviewed-by: NeilBrown <neilbrown.name>

NeilBrown

Re: [PATCH v3 12/14] vfs: short-circuit MAY_WRITE access for O_DIRECTORY opens

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> Requesting write access on a directory can never succeed. Rather
> than performing a path-walk to determine whether the target is
> actually a directory (-EISDIR) or not (-ENOTDIR), or does not exist
> (-ENOENT), etc., we short-circuit to -ENOTDIR.
> 
> Currently O_WRONLY for directories is only blocked in may_open(),
> which happens after we have the inode for the target, so after any
> create via O_CREAT|O_DIRECTORY.
> 
> The advantage of short-circuiting is that we don't have to add even
> more logic to lookup_open() to differentiate -EISDIR/-ENOTDIR. Also,
> for filesystems that define atomic_open(), handling this cannot even be
> done at the VFS level, as we can't know ahead of calling
> ->atomic_open() what the result of the lookup is.
> 
> Suggested-by: Christian Brauner (Amutable) <brauner@kernel.org>
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> ---
>  fs/open.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index 9121aece78bc..87e8864d9480 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -1270,9 +1270,16 @@ inline int build_open_flags(const struct open_how *how, struct open_flags *op)
>  
>  	op->intent = flags & O_PATH ? 0 : LOOKUP_OPEN;
>  
> +	/*
> +	 * Requesting write access on a directory can never succeed. Rather
> +	 * than performing a path-walk to determine whether the target is
> +	 * actually a directory (-EISDIR) or not (-ENOTDIR), we short-circuit
> +	 * to -ENOTDIR.
> +	 */
> +	if ((flags & O_DIRECTORY) && !(flags & __O_TMPFILE) && (acc_mode & MAY_WRITE))
> +		return -ENOTDIR;
> +

Nice simple approach.

Reviewed-by: NeilBrown <neil@brown.name>

NeilBrown


>  	if (flags & O_CREAT) {
> -		if ((flags & O_DIRECTORY) && (acc_mode & MAY_WRITE))
> -			return -EISDIR;
>  		op->intent |= LOOKUP_CREATE;
>  		if (flags & O_EXCL) {
>  			op->intent |= LOOKUP_EXCL;
> -- 
> 2.55.0
> 
> 

Re: [PATCH v3 13/14] selftest: fix headers in fclog.c

[NeilBrown]

On Sun, 05 Jul 2026, Jori Koolstra wrote:
> Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>

Maybe say a bit more about what needs fixing?

I guess the ones you removed are already included in 
  kselftest_harness.h

and fcntl.h is needed for O_RDONLY etc.  Is that all?
Lots of .c files in tools/testing/selftests include fcntl.h.
I wonder if it should go in kselftest_harness.h too?

NeilBrown

> ---
>  tools/testing/selftests/filesystems/fclog.c | 4 +---
>  1 file changed, 1 insertion`h+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/filesystems/fclog.c b/tools/testing/selftests/filesystems/fclog.c
> index 551c4a0f395a..593a5136e991 100644
> --- a/tools/testing/selftests/filesystems/fclog.c
> +++ b/tools/testing/selftests/filesystems/fclog.c
> @@ -6,10 +6,8 @@
>  
>  #include <assert.h>
>  #include <errno.h>
> +#include <fcntl.h>
>  #include <sched.h>
> -#include <stdio.h>
> -#include <stdlib.h>
> -#include <string.h>
>  #include <unistd.h>
>  #include <sys/mount.h>
>  
> -- 
> 2.55.0
> 
> 

Re: [PATCH v3 01/14] fs/namei.c: use trailing_slashes()

[David Laight]

On Mon, 06 Jul 2026 13:56:00 +1000
NeilBrown <neilb@ownmail.net> wrote:

> On Sun, 05 Jul 2026, Jori Koolstra wrote:
> > There are several places in fs/namei.c that can use the
> > trailing_slashes() function to improve intent.  
> 
> I think it would help to state here that trailing_slashes() is changed
> to take a qstr instead of a nameidata as that is useful in more places.

It is also taking the struct 'by value' so should really be always_inline
before something makes a compiler decide it should be a real function.

(Or make it take a pointer)

	David

> 
> Reviewed-by: NeilBrown <neil@brown.name>
> 
> Thanks,
> NeilBrown
> 
> 
> > 
> > Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> > ---
> >  fs/namei.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 19ce43c9a6e6..a8890bc0ae21 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -2781,9 +2781,14 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
> >  	return s;
> >  }
> >  
> > +static inline bool trailing_slashes(const struct qstr last)
> > +{
> > +	return (bool)last.name[last.len];
> > +}
> > +
> >  static inline const char *lookup_last(struct nameidata *nd)
> >  {
> > -	if (nd->last_type == LAST_NORM && nd->last.name[nd->last.len])
> > +	if (nd->last_type == LAST_NORM && trailing_slashes(nd->last))
> >  		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
> >  
> >  	return walk_component(nd, WALK_TRAILING);
> > @@ -4521,17 +4526,12 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> >  	return ERR_PTR(error);
> >  }
> >  
> > -static inline bool trailing_slashes(struct nameidata *nd)
> > -{
> > -	return (bool)nd->last.name[nd->last.len];
> > -}
> > -
> >  static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
> >  {
> >  	struct dentry *dentry;
> >  
> >  	if (open_flag & O_CREAT) {
> > -		if (trailing_slashes(nd))
> > +		if (trailing_slashes(nd->last))
> >  			return ERR_PTR(-EISDIR);
> >  
> >  		/* Don't bother on an O_EXCL create */
> > @@ -4539,7 +4539,7 @@ static struct dentry *lookup_fast_for_open(struct nameidata *nd, int open_flag)
> >  			return NULL;
> >  	}
> >  
> > -	if (trailing_slashes(nd))
> > +	if (trailing_slashes(nd->last))
> >  		nd->flags |= LOOKUP_FOLLOW | LOOKUP_DIRECTORY;
> >  
> >  	dentry = lookup_fast(nd);
> > @@ -4950,7 +4950,7 @@ static struct dentry *filename_create(int dfd, struct filename *name,
> >  	 * Do the final lookup.  Suppress 'create' if there is a trailing
> >  	 * '/', and a directory wasn't requested.
> >  	 */
> > -	if (last.name[last.len] && !want_dir)
> > +	if (trailing_slashes(last) && !want_dir)
> >  		create_flags &= ~LOOKUP_CREATE;
> >  	dentry = start_dirop(path->dentry, &last, reval_flag | create_flags);
> >  	if (IS_ERR(dentry))
> > @@ -5569,7 +5569,7 @@ int filename_unlinkat(int dfd, struct filename *name)
> >  		goto exit_drop_write;
> >  
> >  	/* Why not before? Because we want correct error value */
> > -	if (unlikely(last.name[last.len])) {
> > +	if (unlikely(trailing_slashes(last))) {
> >  		if (d_is_dir(dentry))
> >  			error = -EISDIR;
> >  		else
> > @@ -6171,16 +6171,16 @@ int filename_renameat2(int olddfd, struct filename *from,
> >  	if (flags & RENAME_EXCHANGE) {
> >  		if (!d_is_dir(rd.new_dentry)) {
> >  			error = -ENOTDIR;
> > -			if (new_last.name[new_last.len])
> > +			if (trailing_slashes(new_last))
> >  				goto exit_unlock;
> >  		}
> >  	}
> >  	/* unless the source is a directory trailing slashes give -ENOTDIR */
> >  	if (!d_is_dir(rd.old_dentry)) {
> >  		error = -ENOTDIR;
> > -		if (old_last.name[old_last.len])
> > +		if (trailing_slashes(old_last))
> >  			goto exit_unlock;
> > -		if (!(flags & RENAME_EXCHANGE) && new_last.name[new_last.len])
> > +		if (!(flags & RENAME_EXCHANGE) && trailing_slashes(new_last))
> >  			goto exit_unlock;
> >  	}
> >  
> > -- 
> > 2.55.0
> > 
> >   
> 
> 

Re: [PATCH v3 01/14] fs/namei.c: use trailing_slashes()

[Jori Koolstra]

> Op 06-07-2026 09:29 CEST schreef David Laight <david.laight.linux@gmail.com>:
> 
>  
> On Mon, 06 Jul 2026 13:56:00 +1000
> NeilBrown <neilb@ownmail.net> wrote:
> 
> > On Sun, 05 Jul 2026, Jori Koolstra wrote:
> > > There are several places in fs/namei.c that can use the
> > > trailing_slashes() function to improve intent.  
> > 
> > I think it would help to state here that trailing_slashes() is changed
> > to take a qstr instead of a nameidata as that is useful in more places.

Yes, thanks.

> 
> It is also taking the struct 'by value' so should really be always_inline
> before something makes a compiler decide it should be a real function.
> 
> (Or make it take a pointer)
> 
> 	David

You know more about this compiler optimization stuff than I do, David. So:
is there any chance this does not get inlined? I am fine with always_inline,
but I don't see it often in these scenarios (just inline usually). Is there
a reason for that?

Pointer is also OK, but this is just two register copies (even if it does not get
inlined) on 64-bit. Is that really less fast than one copy and a pointer deref?
Genuine question, I am no expert on this.

Thanks,
Jori.

Re: [PATCH v3 13/14] selftest: fix headers in fclog.c

[Jori Koolstra]

> Op 06-07-2026 07:57 CEST schreef NeilBrown <neilb@ownmail.net>:
> 
>  
> On Sun, 05 Jul 2026, Jori Koolstra wrote:
> > Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> 
> Maybe say a bit more about what needs fixing?
> 

I should really be a bit more explanative, it's a skill I am working on :)

> I guess the ones you removed are already included in 
>   kselftest_harness.h
> 
> and fcntl.h is needed for O_RDONLY etc.  Is that all?
> Lots of .c files in tools/testing/selftests include fcntl.h.
> I wonder if it should go in kselftest_harness.h too?
> 

Yes. Really it does not belong in this series, so maybe I should drop it
but the filesystems selftests don't compile for me without adding fcntl.h,
and clangd was warning me those other headers were not needed.

Let me take a look at kselftest_harness.h; I like your suggestion and then we
can simply drop this noise from this patchset.

> NeilBrown
> 
> > ---
> >  tools/testing/selftests/filesystems/fclog.c | 4 +---
> >  1 file changed, 1 insertion`h+), 3 deletions(-)
> > 
> > diff --git a/tools/testing/selftests/filesystems/fclog.c b/tools/testing/selftests/filesystems/fclog.c
> > index 551c4a0f395a..593a5136e991 100644
> > --- a/tools/testing/selftests/filesystems/fclog.c
> > +++ b/tools/testing/selftests/filesystems/fclog.c
> > @@ -6,10 +6,8 @@
> >  
> >  #include <assert.h>
> >  #include <errno.h>
> > +#include <fcntl.h>
> >  #include <sched.h>
> > -#include <stdio.h>
> > -#include <stdlib.h>
> > -#include <string.h>
> >  #include <unistd.h>
> >  #include <sys/mount.h>
> >  
> > -- 
> > 2.55.0
> > 
> >

Re: [PATCH v3 04/14] vfs: call audit_inode_child() in lookup_open() on failure too

[Jori Koolstra]

> Op 06-07-2026 07:33 CEST schreef NeilBrown <neilb@ownmail.net>:
> 
>  
> On Sun, 05 Jul 2026, Jori Koolstra wrote:
> > audit_inode_child() is called in may_create_dentry() so that failed
> > filesystem operations still register an audit entry. On success, the
> > entry is overwritten when, for instance, fsnotify_create() is called.
> > This is the calling convention in vfs_create() and vfs_mkdir().
> > In lookup_open(), however, when atomic_open() should have created a
> > file but didn't, no call to audit_inode_child() is made. The same is
> > true for the regular ->create() path.
> > 
> > Fix the calling of audit_inode_child() in lookup_open() to match the
> > vfs_create() path. For the ->atomic_open() filesystems this logic has
> > been pushed into atomic_open(). This function is also reordered a bit
> > to make the case distinction of the possible returns from
> > ->atomic_open() more explicit (i.e. finish_open() or finish_no_open()).
> > 
> > When retrying delegation breaking, audit_inode_child() could be called
> > more than once, but this is OK because those entries are reused.
> > 
> > Signed-off-by: Jori Koolstra <jkoolstra@xs4all.nl>
> > ---
> >  fs/namei.c | 75 ++++++++++++++++++++++++++++++++----------------------
> >  1 file changed, 45 insertions(+), 30 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index ee47d3f5159f..956adcd14c4a 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4369,10 +4369,11 @@ static int may_o_create(struct mnt_idmap *idmap,
> >   */
> >  static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
> >  				  struct file *file,
> > -				  int open_flag, umode_t mode)
> > +				  int open_flag, umode_t mode, bool create_error)
> >  {
> >  	struct dentry *const DENTRY_NOT_SET = (void *) -1UL;
> >  	struct inode *dir =  path->dentry->d_inode;
> > +	bool failed_create = false;
> >  	int error;
> >  
> >  	file->__f_path.dentry = DENTRY_NOT_SET;
> > @@ -4382,23 +4383,33 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
> >  	d_lookup_done(dentry);
> >  	if (!error) {
> >  		if (file->f_mode & FMODE_OPENED) {
> > -			if (unlikely(dentry != file->f_path.dentry)) {
> > +			// finish_open() called
> > +			struct dentry *opened = file->f_path.dentry;
> > +			if (unlikely(opened != dentry)) {
> >  				dput(dentry);
> > -				dentry = dget(file->f_path.dentry);
> > +				dentry = dget(opened);
> >  			}
> > -		} else if (WARN_ON(file->f_path.dentry == DENTRY_NOT_SET)) {
> > -			error = -EIO;
> > -		} else {
> > -			if (file->f_path.dentry) {
> > +		} else if (likely(file->f_path.dentry != DENTRY_NOT_SET)) {
> > +			// finish_no_open() called
> > +			struct dentry *replaced = file->f_path.dentry;
> > +			if (replaced) {
> >  				dput(dentry);
> > -				dentry = file->f_path.dentry;
> > +				dentry = replaced;
> >  			}
> >  			if (unlikely(d_is_negative(dentry)))
> >  				error = -ENOENT;
> 
> I think this can now be
>    if (unlikely(d_is_negative(dentry))
>          error = create_error ?: -ENOENT;
> or something similar if people don't like "?:".
> 

create_error is a bool, and the caller still expects -ENOENT because we have

		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
			dentry = ERR_PTR(create_error);

> > +			if (error && create_error) // should have created, but errored before
> > +				failed_create = true;
> 
> Why have 'failed_create' ? Why not just put the audit_inode_child() like
> here?
> 

Because I am a doofus and didn't notice during several rewrites.

If we reach the point where you propose we move audit_inode_child() to, then error
is true and so we reach audit_inode_child(). Conversely, if we reached
audit_inode_child() then failed_create is true, and this is only set at the proposed
place to move the function to. So this is indeed equivalent.

The failed_create name is terrible anyway and it tripped Sashiko over. It was meant
to convey rather "we know for sure that we should have created a file."

Thanks a lot for taking the time to review and the helpful feedback!

Best,
Jori.

> There rest of the patch look pretty good.
> 
> Thanks,
> NeilBrown
> 
> 
> > +		} else {
> > +			const char *fsname = dentry->d_sb->s_type->name;
> > +			WARN(1, "%s: ->atomic_open() left file->f_path.dentry unset!\n", fsname);
> > +			error = -EIO;
> >  		}
> >  	}
> >  
> >  	if (error) {
> > +		if (failed_create)
> > +			audit_inode_child(dir, dentry, AUDIT_TYPE_CHILD_CREATE);
> >  		dput(dentry);
> >  		dentry = ERR_PTR(error);
> >  	} else {
> > @@ -4462,7 +4473,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> >  		dentry = NULL;
> >  	}
> >  	if (dentry->d_inode) {
> > -		/* Cached positive dentry: will open in f_op->open */
> > +		/* Cached positive dentry: will open in do_open(). */
> >  		return dentry;
> >  	}
> >  
> > @@ -4496,7 +4507,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> >  	if (dir_inode->i_op->atomic_open) {
> >  		if (nd->flags & LOOKUP_DIRECTORY)
> >  			open_flag |= O_DIRECTORY;
> > -		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
> > +		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode, create_error);
> >  		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
> >  			dentry = ERR_PTR(create_error);
> >  		return dentry;
> > @@ -4515,33 +4526,37 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
> >  			dentry = res;
> >  		}
> >  	}
> > +	if (dentry->d_inode || !(op->open_flag & O_CREAT)) {
> > +		/* No need to create a file. If lookup returned a positive
> > +		 * dentry, the file will be opened in do_open(). */
> > +		return dentry;
> > +	}
> > +
> > +	/* Negative dentry with O_CREAT flag set */
> > +	audit_inode_child(dir->d_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> >  
> > -	if (unlikely(create_error) && !dentry->d_inode) {
> > +	if (unlikely(create_error)) {
> > +		/* Should have done a create, but we already errored */
> >  		error = create_error;
> >  		goto out_dput;
> >  	}
> >  
> > -	/* Negative dentry, just create the file */
> > -	if (!dentry->d_inode && (open_flag & O_CREAT)) {
> > -		/* but break the directory lease first! */
> > -		error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
> > -		if (error)
> > -			goto out_dput;
> > +	error = try_break_deleg(dir_inode, LEASE_BREAK_DIR_CREATE, delegated_inode);
> > +	if (error)
> > +		goto out_dput;
> >  
> > -		file->f_mode |= FMODE_CREATED;
> > -		audit_inode_child(dir_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> > -		if (!dir_inode->i_op->create) {
> > -			error = -EACCES;
> > -			goto out_dput;
> > -		}
> > +	file->f_mode |= FMODE_CREATED;
> > +	if (!dir_inode->i_op->create) {
> > +		error = -EACCES;
> > +		goto out_dput;
> > +	}
> >  
> > -		error = dir_inode->i_op->create(idmap, dir_inode, dentry,
> > -						mode, open_flag & O_EXCL);
> > -		if (error)
> > -			goto out_dput;
> > +	error = dir_inode->i_op->create(idmap, dir_inode, dentry,
> > +					mode, open_flag & O_EXCL);
> > +	if (error)
> > +		goto out_dput;
> >  
> > -		fsnotify_create(dir_inode, dentry);
> > -	}
> > +	fsnotify_create(dir_inode, dentry);
> >  
> >  	return dentry;
> >  
> > @@ -5071,7 +5086,7 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
> >  
> >  		/* atomic_open will dput(dentry) on error */
> >  		dget(orig_dentry);
> > -		dentry = atomic_open(path, dentry, file, flags, mode);
> > +		dentry = atomic_open(path, dentry, file, flags, mode, create_error);
> >  		error = PTR_ERR_OR_ZERO(dentry);
> >  
> >  		if (IS_ERR(dentry))
> > -- 
> > 2.55.0
> > 
> >