[PATCH] vfs: show filesystem name at dump_inode()

[PATCH] vfs: show filesystem name at dump_inode()

[Tetsuo Handa]

Commit 8b17e540969a ("vfs: add initial support for CONFIG_DEBUG_VFS") added
dump_inode(), but dump_inode() currently reports only raw pointer address.
Comment says that adding a proper inode dumping routine is a TODO.

However, syzkaller concurrently tests multiple filesystems, and several
filesystems started calling dump_inode() due to hitting VFS_BUG_ON_INODE()
added by commit af153bb63a33 ("vfs: catch invalid modes in may_open()")
before a proper inode dumping routine is implemented.

Show filesystem name at dump_inode() so that we can find which filesystem
has passed an invalid mode to may_open() from syzkaller's crash reports.

Link: https://syzkaller.appspot.com/bug?extid=895c23f6917da440ed0d
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 fs/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index 01ebdc40021e..8a60aec94245 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2914,7 +2914,7 @@ EXPORT_SYMBOL(mode_strip_sgid);
  */
 void dump_inode(struct inode *inode, const char *reason)
 {
-       pr_warn("%s encountered for inode %px", reason, inode);
+	pr_warn("%s encountered for inode %px (%s)\n", reason, inode, inode->i_sb->s_type->name);
 }
 
 EXPORT_SYMBOL(dump_inode);
-- 
2.50.1

Re: [PATCH] vfs: show filesystem name at dump_inode()

[Christian Brauner]

On Mon, 11 Aug 2025 15:50:28 +0900, Tetsuo Handa wrote:
> Commit 8b17e540969a ("vfs: add initial support for CONFIG_DEBUG_VFS") added
> dump_inode(), but dump_inode() currently reports only raw pointer address.
> Comment says that adding a proper inode dumping routine is a TODO.
> 
> However, syzkaller concurrently tests multiple filesystems, and several
> filesystems started calling dump_inode() due to hitting VFS_BUG_ON_INODE()
> added by commit af153bb63a33 ("vfs: catch invalid modes in may_open()")
> before a proper inode dumping routine is implemented.
> 
> [...]

Applied to the vfs-6.18.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.18.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.18.misc

[1/1] vfs: show filesystem name at dump_inode()
      https://git.kernel.org/vfs/vfs/c/ecb060536446

Re: [PATCH] vfs: show filesystem name at dump_inode()

[Tetsuo Handa]

On 2025/08/11 22:51, Christian Brauner wrote:
> On Mon, 11 Aug 2025 15:50:28 +0900, Tetsuo Handa wrote:
>> Commit 8b17e540969a ("vfs: add initial support for CONFIG_DEBUG_VFS") added
>> dump_inode(), but dump_inode() currently reports only raw pointer address.
>> Comment says that adding a proper inode dumping routine is a TODO.
>>
>> However, syzkaller concurrently tests multiple filesystems, and several
>> filesystems started calling dump_inode() due to hitting VFS_BUG_ON_INODE()
>> added by commit af153bb63a33 ("vfs: catch invalid modes in may_open()")
>> before a proper inode dumping routine is implemented.
>>
>> [...]
> 
> Applied to the vfs-6.18.misc branch of the vfs/vfs.git tree.
> Patches in the vfs-6.18.misc branch should appear in linux-next soon.

It would be nice if we can get this patch merged into linux.git in 2 weeks, for
https://syzkaller.appspot.com/bug?extid=895c23f6917da440ed0d is hitting on more
filesystems where a reproducer is not yet available.

> 
> Please report any outstanding bugs that were missed during review in a
> new review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the
> patch has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs-6.18.misc
> 
> [1/1] vfs: show filesystem name at dump_inode()
>       https://git.kernel.org/vfs/vfs/c/ecb060536446

Re: [PATCH] vfs: show filesystem name at dump_inode()

[Mateusz Guzik]

On Mon, Aug 11, 2025 at 8:50 AM Tetsuo Handa
<penguin-kernel@i-love.sakura.ne.jp> wrote:
>
> Commit 8b17e540969a ("vfs: add initial support for CONFIG_DEBUG_VFS") added
> dump_inode(), but dump_inode() currently reports only raw pointer address.
> Comment says that adding a proper inode dumping routine is a TODO.
>
> However, syzkaller concurrently tests multiple filesystems, and several
> filesystems started calling dump_inode() due to hitting VFS_BUG_ON_INODE()
> added by commit af153bb63a33 ("vfs: catch invalid modes in may_open()")
> before a proper inode dumping routine is implemented.
>
> Show filesystem name at dump_inode() so that we can find which filesystem
> has passed an invalid mode to may_open() from syzkaller's crash reports.
>
> Link: https://syzkaller.appspot.com/bug?extid=895c23f6917da440ed0d
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> ---
>  fs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 01ebdc40021e..8a60aec94245 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2914,7 +2914,7 @@ EXPORT_SYMBOL(mode_strip_sgid);
>   */
>  void dump_inode(struct inode *inode, const char *reason)
>  {
> -       pr_warn("%s encountered for inode %px", reason, inode);
> +       pr_warn("%s encountered for inode %px (%s)\n", reason, inode, inode->i_sb->s_type->name);
>  }
>
>  EXPORT_SYMBOL(dump_inode);
> --
> 2.50.1

Better printing is a TODO in part because the routine must not trip
over arbitrarily bogus state, in this case notably that's unset
->i_sb.

See mm/debug.c:dump_vmg for an example.

I could swear one of the dumping routines in mm was using something
special to deref pointers without tripping over it either, but now I
can't find it.

All that said, I suggest this direction:
diff --git a/fs/inode.c b/fs/inode.c
index 01ebdc40021e..113fcb8da983 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2914,7 +2914,9 @@ EXPORT_SYMBOL(mode_strip_sgid);
  */
 void dump_inode(struct inode *inode, const char *reason)
 {
-       pr_warn("%s encountered for inode %px", reason, inode);
+       struct super_block *sb = inode->i_sb; /* will be careful deref later */
+
+       pr_warn("%s encountered for inode %px [fs %s]", reason, inode,
sb ? sb->s_type->name : "NOT SET");
 }

 EXPORT_SYMBOL(dump_inode);

Can't do a proper submission at the moment and I'm not going to argue
about authorship should this land. :)

-- 
Mateusz Guzik <mjguzik gmail.com>

Re: [PATCH] vfs: show filesystem name at dump_inode()

[Al Viro]

On Mon, Aug 11, 2025 at 09:45:52PM +0200, Mateusz Guzik wrote:

> Better printing is a TODO in part because the routine must not trip
> over arbitrarily bogus state, in this case notably that's unset
> ->i_sb.

That... is a strange state.  It means having never been passed to
inode_init_always().  How do you get to it?  I mean, if the argument
is not pointing to a struct inode instance, sure, but then NULL is
not the only possibility - we are talking about the valur of
arbitrary word of memory that might contain anything whatsoever.

If, OTOH, it is a genuine struct inode, it must be in a very strange
point in the lifecycle - somewhere in the middle of alloc_inode(),
definitely before its address gets returned to the caller...

> See mm/debug.c:dump_vmg for an example.

Not quite relevant here...

>  void dump_inode(struct inode *inode, const char *reason)
>  {
> -       pr_warn("%s encountered for inode %px", reason, inode);
> +       struct super_block *sb = inode->i_sb; /* will be careful deref later */
> +
> +       pr_warn("%s encountered for inode %px [fs %s]", reason, inode,
> sb ? sb->s_type->name : "NOT SET");

That's really misleading - this "NOT SET" is not a valid state; ->i_sb is
an assign-once member that gets set by constructor before the object is
returned and it's never modified afterwards.  In particular, it is never
cleared.

There is a weird debugging in generic_shutdown_super() that goes through
the inodes of dying superblock that had survived the fs shutdown
("Busy inodes after unmount" thing) and poisons their ->i_sb, but that's
VFS_PTR_POINSON, not NULL.

We literally never store NULL there.  Not even with kmem_cache_zalloc()...

Re: [PATCH] vfs: show filesystem name at dump_inode()

[Mateusz Guzik]

On Mon, Aug 11, 2025 at 09:38:15PM +0100, Al Viro wrote:
> On Mon, Aug 11, 2025 at 09:45:52PM +0200, Mateusz Guzik wrote:
> 
> > Better printing is a TODO in part because the routine must not trip
> > over arbitrarily bogus state, in this case notably that's unset
> > ->i_sb.
> 
> That... is a strange state.  It means having never been passed to
> inode_init_always().  How do you get to it?  I mean, if the argument
> is not pointing to a struct inode instance, sure, but then NULL is
> not the only possibility - we are talking about the valur of
> arbitrary word of memory that might contain anything whatsoever.
> 
> If, OTOH, it is a genuine struct inode, it must be in a very strange
> point in the lifecycle - somewhere in the middle of alloc_inode(),
> definitely before its address gets returned to the caller...
> 
> > See mm/debug.c:dump_vmg for an example.
> 
> Not quite relevant here...
> 
> >  void dump_inode(struct inode *inode, const char *reason)
> >  {
> > -       pr_warn("%s encountered for inode %px", reason, inode);
> > +       struct super_block *sb = inode->i_sb; /* will be careful deref later */
> > +
> > +       pr_warn("%s encountered for inode %px [fs %s]", reason, inode,
> > sb ? sb->s_type->name : "NOT SET");
> 
> That's really misleading - this "NOT SET" is not a valid state; ->i_sb is
> an assign-once member that gets set by constructor before the object is
> returned and it's never modified afterwards.  In particular, it is never
> cleared.
> 
> There is a weird debugging in generic_shutdown_super() that goes through
> the inodes of dying superblock that had survived the fs shutdown
> ("Busy inodes after unmount" thing) and poisons their ->i_sb, but that's
> VFS_PTR_POINSON, not NULL.
> 
> We literally never store NULL there.  Not even with kmem_cache_zalloc()...

So I copied the stuff from mm/ and have distinct recollection they used
a special routine to deref pointers (or fail) to avoid faulting on
arbitrary breakage, even pointers which are expected to be sound on
crashes.

Based on that I assumed this is the expected treatment and I could not
be arsed to sort it out in dump_inode(), hence the stub and the remark
in my previous e-mail.

Now that I look at their dump_* routines I don't see anything of the
sort, so maybe I was tripping hard.

If the routine is fine just reading values from the passed inode
(including pointer derefs), perhaps one can sit through expanding the
output beyond just fs name?

Also note it would be nice (tm) if there was a callback in inode ops to
let the fs dump stuff on top of the whatever dump_inode() is doing.

I'm not in position to sort it out for the time being (fwiw FreeBSD has
one, see vn_printf -> VOP_PRINT).

However, bare minimum which should be immediately added in this case are
the state and flag fields.

With this in mind, here is a completely untested diff which prints
fields in order they are specified in struct inode in the range i_mode
to i_default_acl, then few extra fields (again in order). Preferably
someone(tm) would print all the fields and branch on inode type to know
how to handle unions.

I'm not in position to even compile test or validate format specifiers
work as expected on funky platforms though, so I'm just throwing this to
illustrate and perhaps save someone a bit of hand work (just in case
I'll note I don't want or need credit for the thing below, should
someone decide sort it out):

diff --git a/fs/inode.c b/fs/inode.c
index 01ebdc40021e..4022f1d009dc 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -2914,7 +2914,14 @@ EXPORT_SYMBOL(mode_strip_sgid);
  */
 void dump_inode(struct inode *inode, const char *reason)
 {
-       pr_warn("%s encountered for inode %px", reason, inode);
+	struct super_block *sb = inode->i_sb;
+
+	pr_warn("%s encountered for inode %px fs %s mode %ho opflags %hx\n"
+		"uid %d gid %d flags %u acl %px default_acl %px inode %lu state %u\n",
+		"nlink %u size %u"
+		reason, inode, sb->s_type->name, inode->i_mode, inode->i_opflags,
+		inode->i_uid, inode->i_gid, inode->i_flags, inode->i_acl, inode->i_default_acl,
+		inode->i_ino, inode->i_state, inode->i_nlink, inode->i_size);
 }
 
 EXPORT_SYMBOL(dump_inode);