Re: [PATCH for 6.19-rc1] fs: preserve file type in make_bad_inode() unless invalid

Jan Kara
2025-12-10T10:09:16Z

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d222f4b7129379c3d5bc
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.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.

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.

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@suse.com>
SUSE Labs, CR