[PATCH 0/3] fs/namei.c: comment and coding style fixups

[PATCH 0/3] fs/namei.c: comment and coding style fixups

[Christian Brauner]

Neil,

Three comment and coding style fixups for your lookup_open() rework and
Jori's audit series as they sit in vfs-7.3.lookup. No functional changes.

* Update the comments above lookup_open(). It takes the parent inode lock
  itself now but the comment still tells the caller to take it. A caller
  following it deadlocks and the series added a second caller. The claim
  that it returns 0 and updates @path goes as well, that one has been
  wrong since v5.7.

* Give the return description of atomic_open() the colon that kernel-doc
  needs. Without it the description is dropped and W=1 warns about it.
  The summary line has to stand on its own line too, so the "from a
  negative dentry" part moves into the body.

* Fix a space indented continuation line, three declarations without a
  following blank line and a trailing */ on the last line of a block
  comment.

Christian

---
Christian Brauner (3):
      fs/namei.c: update stale comments in lookup_open()
      fs/namei.c: fix kerneldoc of atomic_open() and vfs_lookup_open()
      fs/namei.c: fix coding style in atomic_open() and lookup_open()

 fs/namei.c | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)
---
base-commit: b2f1e6301efa4a80becdb0715416c3cbc693fbb4
change-id: 20260731-work-lookup-fixes-73bd87cc2d24

[PATCH 1/3] fs/namei.c: update stale comments in lookup_open()

[Christian Brauner]

Commit ddb6e6c72a0a ("VFS: move mnt_want_write() and locking into
lookup_open()") moved the parent inode locking into lookup_open(), but
left the comment claiming the caller has to take it.  A caller following
that comment now deadlocks, and the series added a second caller.

Describe what the function actually does.  While at it drop the claim
that it returns 0 on success and updates @path, wrong ever since
lookup_open() started returning a dentry in v5.7, and fix the reference
to lookup_open() in a comment that now sits inside lookup_open() itself.

Fixes: ddb6e6c72a0a ("VFS: move mnt_want_write() and locking into lookup_open()")
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/namei.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 6db5b7e8547b..226abf613983 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4417,17 +4417,16 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 /*
  * Look up and maybe create and open the last component.
  *
- * Must be called with parent locked (exclusive in O_CREAT case).
+ * Takes the parent inode lock itself, exclusive if O_CREAT was requested and
+ * shared otherwise, and drops it again before returning.  The caller must not
+ * hold it.
  *
- * Returns 0 on success, that is, if
- *  the file was successfully atomically created (if necessary) and opened, or
- *  the file was not completely opened at this time, though lookups and
- *  creations were performed.
- * These case are distinguished by presence of FMODE_OPENED on file->f_mode.
- * In the latter case dentry returned in @path might be negative if O_CREAT
- * hadn't been specified.
+ * On success returns the dentry of the last component.  If FMODE_OPENED is set
+ * on file->f_mode the file was also opened and attached to @file; otherwise
+ * only lookup and creation were performed and the caller has to open it.  In
+ * the latter case the dentry may be negative if O_CREAT hadn't been specified.
  *
- * An error code is returned on failure.
+ * Returns ERR_PTR() on failure.
  */
 static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 				  const struct open_flags *op)
@@ -4452,8 +4451,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		got_write = !mnt_want_write(nd->path.mnt);
 		/*
 		 * do _not_ fail yet - we might not need that or fail with
-		 * a different error; let lookup_open() decide; we'll be
-		 * dropping this one anyway.
+		 * a different error; we'll be dropping this one anyway.
 		 */
 	}
 	if (open_flag & O_CREAT)

-- 
2.53.0

[PATCH 2/3] fs/namei.c: fix kerneldoc of atomic_open() and vfs_lookup_open()

[Christian Brauner]

Commit ba0e87026613 ("fs/namei.c: update kerneldoc of atomic_open()")
turned the comment above atomic_open() into kerneldoc, but wrote the
return description as running text.  kernel-doc only recognises a return
section introduced by "Return:" or "Returns:", so this added a warning
under W=1:

  fs/namei.c:4362 No description found for return value of 'atomic_open'

Give it the missing colon.  The summary line also has to stand on its
own line, so move the "from a negative dentry" part into the body, where
it can say that the caller has to hand over a negative dentry.

Also add the "to" missing from vfs_lookup_open()'s description.

Fixes: ba0e87026613 ("fs/namei.c: update kerneldoc of atomic_open()")
Fixes: 536227b814bd ("VFS: add vfs_lookup_open() for nfsd")
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/namei.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 226abf613983..e31905dfeb20 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4337,8 +4337,7 @@ static int may_o_create(struct mnt_idmap *idmap,
 }
 
 /**
- * atomic_open() - attempt to atomically look up, create and open a file
- * from a negative dentry.
+ * atomic_open() - atomically look up, create and open a file
  * @path:          parent directory path
  * @dentry:        child to ->atomic_open()
  * @file:          file to attach child to
@@ -4346,6 +4345,9 @@ static int may_o_create(struct mnt_idmap *idmap,
  * @mode:          create mode
  * @create_error:  return value from may_o_create()
  *
+ * Attempt to look up, create and open @dentry, which must be negative, in a
+ * single call into the filesystem.
+ *
  * If a non-error dentry is returned then: when 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
@@ -4354,8 +4356,8 @@ static int may_o_create(struct mnt_idmap *idmap,
  * 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.
+ * Returns: the opened or looked-up dentry, or ERR_PTR() on failure.  The
+ * reference to @dentry is consumed in either case.
  */
 static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
 				  struct file *file,
@@ -4605,7 +4607,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
  * @mode: initial permissions for file
  *
  * Open a file after lookup and/or create.  This provides similar
- * functionality open_last_lookups() for non-VFS users, particularly
+ * functionality to open_last_lookups() for non-VFS users, particularly
  * nfsd.
  * It uses ->atomic_open or ->lookup / ->create / ->open as appropriate.
  *

-- 
2.53.0

[PATCH 3/3] fs/namei.c: fix coding style in atomic_open() and lookup_open()

[Christian Brauner]

Commit 4886c80eef20 ("vfs: call audit_inode_child() in lookup_open() on
failure") indented a continuation line with spaces, left three
declarations without a following blank line and used a trailing */ on the
last line of a block comment.  Clean all of that up, no functional change.

Fixes: 4886c80eef20 ("vfs: call audit_inode_child() in lookup_open() on failure")
Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
---
 fs/namei.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index e31905dfeb20..c0da9b5dd47a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4377,6 +4377,7 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 		if (file->f_mode & FMODE_OPENED) {
 			/* finish_open() called */
 			struct dentry *opened = file->f_path.dentry;
+
 			if (unlikely(opened != dentry)) {
 				dput(dentry);
 				dentry = dget(opened);
@@ -4384,6 +4385,7 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *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 = replaced;
@@ -4392,8 +4394,9 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
 				error = -ENOENT;
 		} else {
 			const char *fsname = dentry->d_sb->s_type->name;
+
 			WARN(1, "%s: ->atomic_open() left file->f_path.dentry unset!\n",
-			        fsname);
+			     fsname);
 			error = -EIO;
 		}
 	}
@@ -4540,8 +4543,10 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
 		}
 	}
 	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(). */
+		/*
+		 * No need to create a file.  If lookup returned a positive
+		 * dentry, the file will be opened in do_open().
+		 */
 		goto out;
 	}
 

-- 
2.53.0

Re: [PATCH 0/3] fs/namei.c: comment and coding style fixups

[NeilBrown]

On Fri, 31 Jul 2026, Christian Brauner wrote:
> Neil,
> 
> Three comment and coding style fixups for your lookup_open() rework and
> Jori's audit series as they sit in vfs-7.3.lookup. No functional changes.

Thanks for doing that - looks good.  Feel free to add

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

if you like.

NeilBrown


> 
> * Update the comments above lookup_open(). It takes the parent inode lock
>   itself now but the comment still tells the caller to take it. A caller
>   following it deadlocks and the series added a second caller. The claim
>   that it returns 0 and updates @path goes as well, that one has been
>   wrong since v5.7.
> 
> * Give the return description of atomic_open() the colon that kernel-doc
>   needs. Without it the description is dropped and W=1 warns about it.
>   The summary line has to stand on its own line too, so the "from a
>   negative dentry" part moves into the body.
> 
> * Fix a space indented continuation line, three declarations without a
>   following blank line and a trailing */ on the last line of a block
>   comment.
> 
> Christian
> 
> ---
> Christian Brauner (3):
>       fs/namei.c: update stale comments in lookup_open()
>       fs/namei.c: fix kerneldoc of atomic_open() and vfs_lookup_open()
>       fs/namei.c: fix coding style in atomic_open() and lookup_open()
> 
>  fs/namei.c | 43 ++++++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 19 deletions(-)
> ---
> base-commit: b2f1e6301efa4a80becdb0715416c3cbc693fbb4
> change-id: 20260731-work-lookup-fixes-73bd87cc2d24
> 
> 

Re: [PATCH 2/3] fs/namei.c: fix kerneldoc of atomic_open() and vfs_lookup_open()

[Jori Koolstra]

Hi Christian,

> Op 31-07-2026 10:36 CEST schreef Christian Brauner <brauner@kernel.org>:
> 
>  
> Commit ba0e87026613 ("fs/namei.c: update kerneldoc of atomic_open()")
> turned the comment above atomic_open() into kerneldoc, but wrote the
> return description as running text.  kernel-doc only recognises a return
> section introduced by "Return:" or "Returns:", so this added a warning
> under W=1:
> 
>   fs/namei.c:4362 No description found for return value of 'atomic_open'
> 
> Give it the missing colon.  The summary line also has to stand on its
> own line, so move the "from a negative dentry" part into the body, where
> it can say that the caller has to hand over a negative dentry.
> 
> Also add the "to" missing from vfs_lookup_open()'s description.
> 
> Fixes: ba0e87026613 ("fs/namei.c: update kerneldoc of atomic_open()")
> Fixes: 536227b814bd ("VFS: add vfs_lookup_open() for nfsd")
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> ---
>  fs/namei.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 226abf613983..e31905dfeb20 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4337,8 +4337,7 @@ static int may_o_create(struct mnt_idmap *idmap,
>  }
>  
>  /**
> - * atomic_open() - attempt to atomically look up, create and open a file
> - * from a negative dentry.
> + * atomic_open() - atomically look up, create and open a file
>   * @path:          parent directory path
>   * @dentry:        child to ->atomic_open()
>   * @file:          file to attach child to
> @@ -4346,6 +4345,9 @@ static int may_o_create(struct mnt_idmap *idmap,
>   * @mode:          create mode
>   * @create_error:  return value from may_o_create()
>   *
> + * Attempt to look up, create and open @dentry, which must be negative, in a
> + * single call into the filesystem.
> + *
>   * If a non-error dentry is returned then: when 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
> @@ -4354,8 +4356,8 @@ static int may_o_create(struct mnt_idmap *idmap,
>   * 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.
> + * Returns: the opened or looked-up dentry, or ERR_PTR() on failure.  The
> + * reference to @dentry is consumed in either case.

Is this true? Maybe I am confused about the semantics of "consumed." If a function
returns a dentry (or anything else refcounted), without dput()-ing it (or whatever),
does that count as consuming that dentry in that function?

It's not mentioned in the commit log, that's why I ask :)

Thanks for the fixup!

Best,
Jori.

Re: [PATCH 3/3] fs/namei.c: fix coding style in atomic_open() and lookup_open()

[Jori Koolstra]

> Op 31-07-2026 10:36 CEST schreef Christian Brauner <brauner@kernel.org>:
> 
>  
> Commit 4886c80eef20 ("vfs: call audit_inode_child() in lookup_open() on
> failure") indented a continuation line with spaces, left three
> declarations without a following blank line and used a trailing */ on the
> last line of a block comment.  Clean all of that up, no functional change.
> 
> Fixes: 4886c80eef20 ("vfs: call audit_inode_child() in lookup_open() on failure")
> Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> 

I know checkpatch complains about this, but since namei.c has a bit of its own
style, I didn't pay too much attention to that. Do we adhere strictly to
checkpatch style in VFS? Anyhow, I'll add new lines after declarations in the
future :)

Thanks,
Jori.

Re: [PATCH 3/3] fs/namei.c: fix coding style in atomic_open() and lookup_open()

[Christian Brauner]

On Fri, Jul 31, 2026 at 11:20:04AM +0200, Jori Koolstra wrote:
> 
> > Op 31-07-2026 10:36 CEST schreef Christian Brauner <brauner@kernel.org>:
> > 
> >  
> > Commit 4886c80eef20 ("vfs: call audit_inode_child() in lookup_open() on
> > failure") indented a continuation line with spaces, left three
> > declarations without a following blank line and used a trailing */ on the
> > last line of a block comment.  Clean all of that up, no functional change.
> > 
> > Fixes: 4886c80eef20 ("vfs: call audit_inode_child() in lookup_open() on failure")
> > Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> > 
> 
> I know checkpatch complains about this, but since namei.c has a bit of its own
> style, I didn't pay too much attention to that. Do we adhere strictly to
> checkpatch style in VFS? Anyhow, I'll add new lines after declarations in the

We don't care about checkpatch per se but we do leave newlines after
declarations. I think that's just pretty standard all around afaict.

Re: [PATCH 2/3] fs/namei.c: fix kerneldoc of atomic_open() and vfs_lookup_open()

[Christian Brauner]

On Fri, Jul 31, 2026 at 11:11:22AM +0200, Jori Koolstra wrote:
> Hi Christian,
> 
> > Op 31-07-2026 10:36 CEST schreef Christian Brauner <brauner@kernel.org>:
> > 
> >  
> > Commit ba0e87026613 ("fs/namei.c: update kerneldoc of atomic_open()")
> > turned the comment above atomic_open() into kerneldoc, but wrote the
> > return description as running text.  kernel-doc only recognises a return
> > section introduced by "Return:" or "Returns:", so this added a warning
> > under W=1:
> > 
> >   fs/namei.c:4362 No description found for return value of 'atomic_open'
> > 
> > Give it the missing colon.  The summary line also has to stand on its
> > own line, so move the "from a negative dentry" part into the body, where
> > it can say that the caller has to hand over a negative dentry.
> > 
> > Also add the "to" missing from vfs_lookup_open()'s description.
> > 
> > Fixes: ba0e87026613 ("fs/namei.c: update kerneldoc of atomic_open()")
> > Fixes: 536227b814bd ("VFS: add vfs_lookup_open() for nfsd")
> > Signed-off-by: Christian Brauner (Amutable) <brauner@kernel.org>
> > ---
> >  fs/namei.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 226abf613983..e31905dfeb20 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -4337,8 +4337,7 @@ static int may_o_create(struct mnt_idmap *idmap,
> >  }
> >  
> >  /**
> > - * atomic_open() - attempt to atomically look up, create and open a file
> > - * from a negative dentry.
> > + * atomic_open() - atomically look up, create and open a file
> >   * @path:          parent directory path
> >   * @dentry:        child to ->atomic_open()
> >   * @file:          file to attach child to
> > @@ -4346,6 +4345,9 @@ static int may_o_create(struct mnt_idmap *idmap,
> >   * @mode:          create mode
> >   * @create_error:  return value from may_o_create()
> >   *
> > + * Attempt to look up, create and open @dentry, which must be negative, in a
> > + * single call into the filesystem.
> > + *
> >   * If a non-error dentry is returned then: when 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
> > @@ -4354,8 +4356,8 @@ static int may_o_create(struct mnt_idmap *idmap,
> >   * 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.
> > + * Returns: the opened or looked-up dentry, or ERR_PTR() on failure.  The
> > + * reference to @dentry is consumed in either case.
> 
> Is this true? Maybe I am confused about the semantics of "consumed." If a function
> returns a dentry (or anything else refcounted), without dput()-ing it (or whatever),
> does that count as consuming that dentry in that function?

The caller gives up ownership of reference of the dentry they passed.
Only the _return_ value can be used. On success you get the reference
for the same dentry back. On failure you get another dentry and the
reference of the dentry passed in is dropped. finish_no_open() is
documented similarly iirc.