Message-ID: <CAGudoHE+WQBt4=Fb39qoYtwceHTWdgAamZDvDq1DAsAU9Qh=ng@mail.gmail.com>
Date: Wed, 10 Dec 2025 11:24:40 +0100
From: Mateusz Guzik <mjguzik@...il.com>
To: Jan Kara <jack@...e.cz>
Cc: Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>, Al Viro <viro@...iv.linux.org.uk>, 
	syzbot <syzbot+d222f4b7129379c3d5bc@...kaller.appspotmail.com>, brauner@...nel.org, 
	jlbec@...lplan.org, joseph.qi@...ux.alibaba.com, linkinjeon@...nel.org, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org, mark@...heh.com, 
	ocfs2-devel@...ts.linux.dev, sj1557.seo@...sung.com, 
	syzkaller-bugs@...glegroups.com, Chuck Lever <chuck.lever@...cle.com>
Subject: Re: [PATCH for 6.19-rc1] fs: preserve file type in make_bad_inode()
 unless invalid

On Wed, Dec 10, 2025 at 11:09 AM Jan Kara <jack@...e.cz> wrote:
>
> On Wed 10-12-25 18:45:26, Tetsuo Handa wrote:
> > syzbot is hitting VFS_BUG_ON_INODE(!S_ISDIR(inode->i_mode)) check
> > introduced by commit e631df89cd5d ("fs: speed up path lookup with cheaper
> > handling of MAY_EXEC"), for make_bad_inode() is blindly changing file type
> > to S_IFREG. Since make_bad_inode() might be called after an inode is fully
> > constructed, make_bad_inode() should not needlessly change file type.
> >
> > Reported-by: syzbot+d222f4b7129379c3d5bc@...kaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=d222f4b7129379c3d5bc
> > Signed-off-by: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
>
> No. make_bad_inode() must not be called once the inode is fully visible
> because that can cause all sorts of fun. That function is really only good
> for handling a situation when read of an inode from the disk failed or
> similar early error paths. It would be great if make_bad_inode() could do
> something like:
>
>         VFS_BUG_ON_INODE(!(inode_state_read_once(inode) & I_NEW));
>
> but sadly that is not currently possible because inodes start with i_state
> set to 0 and some places do call make_bad_inode() before I_NEW is set in
> i_state. Matheusz wanted to clean that up a bit AFAIK.
>

[ most unfortunate timing, I just sent an e-mail with an assumption
that make_bad_inode() has to be callable after the inode+dentries got
published. :> ]

I'm delighted to see the call is considered bogus.

As for being able to assert on it, I noted the current flag handling
for lifecycle tracking is unhelpful.

Per your response, i_state == 0 is overloaded to mean the inode is
fully sorted out *and* that it is brand new.

Instead clear-cut indicators are needed to track where the inode is in
its lifecycle.

I proposed 2 ways: a dedicated enum or fucking around with flags.

Indeed the easiest stepping stone for the time being would be to push
up I_NEW to alloc_inode and assert on it in places which set the flag.
I'm going to cook it up.

> Until the cleanup is done, perhaps we could add:
>
>         VFS_BUG_ON_INODE(inode->i_dentry->first);
>
> to make_bad_inode() and watch the fireworks from syzbot. But at least the
> bugs would be attributed to the place where they are happening.
>

Note the assert which is currently tripping over is very much
necessary for correctness as the new routine skips checking the type
on its own.

Thus the issue needs to get solved for 6.19.

Trying to weed out all of the make_bad_inode callers is probably too
much for the release cycle.

So I stand by patching this up to a state where the lookup routine can
reliably check that this is what happened, should it find a non-dir
inode and doing a proper fix for the next merge window.

>                                                                 Honza
>
> > ---
> > Should we implement all callbacks (except get_offset_ctx callback which is
> > currently used by only tmpfs which does not call make_bad_inode()) within
> > bad_inode_ops, for there might be a callback which is expected to be non-NULL
> > for !S_IFREG types? Implementing missing callbacks is good for eliminating
> > possibility of NULL function pointer call. Since VFS is using
> >
> >     if (!inode->i_op->foo)
> >         return error;
> >     inode->i_op->foo();
> >
> > pattern instead of
> >
> >     pFoo = READ_ONCE(inode->i_op->foo)
> >     if (!pFoo)
> >         return error;
> >     pFoo();
> >
> > pattern, suddenly replacing "one i_op with i_op->foo != NULL" with "another
> > i_op with i_op->foo == NULL" has possibility of NULL pointer function call
> > (e.g. https://lkml.kernel.org/r/18a58415-4aa9-4cba-97d2-b70384407313@I-love.SAKURA.ne.jp ).
> > If we implement missing callbacks, e.g. vfs_fileattr_get() will start
> > calling security_inode_file_getattr() on bad inode, but we can eliminate
> > possibility of inode->i_op->fileattr_get == NULL when make_bad_inode() is
> > called from security_inode_file_getattr() for some reason.
> >
> >  fs/bad_inode.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/bad_inode.c b/fs/bad_inode.c
> > index 0ef9bcb744dd..ff6c2daecd1c 100644
> > --- a/fs/bad_inode.c
> > +++ b/fs/bad_inode.c
> > @@ -207,7 +207,19 @@ void make_bad_inode(struct inode *inode)
> >  {
> >       remove_inode_hash(inode);
> >
> > -     inode->i_mode = S_IFREG;
> > +     switch (inode->i_mode & S_IFMT) {
> > +     case S_IFREG:
> > +     case S_IFDIR:
> > +     case S_IFLNK:
> > +     case S_IFCHR:
> > +     case S_IFBLK:
> > +     case S_IFIFO:
> > +     case S_IFSOCK:
> > +             inode->i_mode &= S_IFMT;
> > +             break;
> > +     default:
> > +             inode->i_mode = S_IFREG;
> > +     }
> >       simple_inode_init_ts(inode);
> >       inode->i_op = &bad_inode_ops;
> >       inode->i_opflags &= ~IOP_XATTR;
> > --
> > 2.47.3
> >
> >
> --
> Jan Kara <jack@...e.com>
> SUSE Labs, CR

Powered by blists - more mailing lists