Message-ID: <lm6pyrrfjvrpam3xdlyfdnr4dsdge5gtw6jwths5wynvbcgcfs@b2xgnw55aedh>
Date: Tue, 12 Aug 2025 06:07:41 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Al Viro <viro@...iv.linux.org.uk>
Cc: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, 
	linux-fsdevel <linux-fsdevel@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] vfs: show filesystem name at dump_inode()

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);

Powered by blists - more mailing lists