[syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: d7b3af5a77e8 Add linux-next specific files for 20230728
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=13018b26a80000
kernel config: https://syzkaller.appspot.com/x/.config?x=62dd327c382e3fe
dashboard link: https://syzkaller.appspot.com/bug?extid=69c477e882e44ce41ad9
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17b490c5a80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=139a9c7ea80000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/5efa5e68267f/disk-d7b3af5a.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/b1f5d3e10263/vmlinux-d7b3af5a.xz
kernel image: https://storage.googleapis.com/syzbot-assets/57cab469d186/bzImage-d7b3af5a.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/2cf2189f623b/mount_0.gz

The issue was bisected to:

commit 1dbd9ceb390c4c61d28cf2c9251dd2015946ce51
Author: Jan Kara <ja...@suse.cz>
Date: Mon Jul 24 17:51:45 2023 +0000

fs: open block device after superblock creation

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1613d586a80000
final oops: https://syzkaller.appspot.com/x/report.txt?x=1513d586a80000
console output: https://syzkaller.appspot.com/x/log.txt?x=1113d586a80000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+69c477...@syzkaller.appspotmail.com
Fixes: 1dbd9ceb390c ("fs: open block device after superblock creation")

/dev/nullb0: Can't open blockdev
------------[ cut here ]------------
WARNING: CPU: 0 PID: 5047 at fs/erofs/super.c:862 erofs_kill_sb+0x206/0x260 fs/erofs/super.c:862
Modules linked in:
CPU: 0 PID: 5047 Comm: syz-executor356 Not tainted 6.5.0-rc3-next-20230728-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/12/2023
RIP: 0010:erofs_kill_sb+0x206/0x260 fs/erofs/super.c:862
Code: 00 00 5b 5d 41 5c 41 5d e9 e7 27 b7 fd e8 e2 27 b7 fd 48 89 df e8 6a 81 1b fe 5b 5d 41 5c 41 5d e9 cf 27 b7 fd e8 ca 27 b7 fd <0f> 0b e9 41 fe ff ff e8 5e f6 0b fe e9 1a fe ff ff e8 54 f6 0b fe
RSP: 0018:ffffc90003c2fca0 EFLAGS: 00010293
RAX: 0000000000000000 RBX: ffff888029aee000 RCX: 0000000000000000
RDX: ffff8880268e9dc0 RSI: ffffffff83cfdc26 RDI: 0000000000000007
RBP: 00000000e0f5e1e2 R08: 0000000000000007 R09: 00000000e0f5e1e2
R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000000
R13: 1ffff92000785fa0 R14: 00000000fffffff0 R15: ffff88807d747cf8
FS: 0000555556001380(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000055aecc7b6468 CR3: 000000007b9d9000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
deactivate_locked_super+0x9a/0x170 fs/super.c:330
get_tree_bdev+0x4c7/0x6a0 fs/super.c:1347
vfs_get_tree+0x88/0x350 fs/super.c:1521
do_new_mount fs/namespace.c:3335 [inline]
path_mount+0x1492/0x1ed0 fs/namespace.c:3662
do_mount fs/namespace.c:3675 [inline]
__do_sys_mount fs/namespace.c:3884 [inline]
__se_sys_mount fs/namespace.c:3861 [inline]
__x64_sys_mount+0x293/0x310 fs/namespace.c:3861
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x38/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f7610a75f09
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 f1 17 00 00 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 b8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fffaf351d38 EFLAGS: 00000246 ORIG_RAX: 00000000000000a5
RAX: ffffffffffffffda RBX: 6c756e2f7665642f RCX: 00007f7610a75f09
RDX: 0000000020000080 RSI: 0000000020000040 RDI: 0000000020000000
RBP: 00000000000f4240 R08: 0000000000000000 R09: 0000555556002378
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000001
R13: 00007fffaf351d70 R14: 00007fffaf351d5c R15: 00007f7610abf03b
</TASK>


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection

If the bug is already fixed, let syzbot know by replying with:
#syz fix: exact-commit-title

If you want syzbot to run the reproducer, reply with:
#syz test: git://repo/address.git branch-or-commit-hash
If you attach or paste a git patch, syzbot will apply it before testing.

If you want to change bug's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

If the bug is a duplicate of another bug, reply with:
#syz dup: exact-subject-of-another-report

If you want to undo deduplication, reply with:
#syz undup

[Christoph Hellwig]

On Mon, Jul 31, 2023 at 12:57:58AM -0700, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: d7b3af5a77e8 Add linux-next specific files for 20230728

Hmm, the current linux-next tree does not seem to have that commit ID
any more, and the line numbers don't match up. I think it is the
WARN_ON for the magic, which could probably be just removed. I'll
try the reproducers when I find a bit more time.

[Gao Xiang]

Previously, deactivate_locked_super() or .kill_sb() will only be
called after fill_super is called, and .s_magic will be set at
the very beginning of erofs_fc_fill_super().

After ("fs: open block device after superblock creation"), such
convension is changed now. Yet at a quick glance,

WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);

in erofs_kill_sb() can be removed since deactivate_locked_super()
will also be called if setup_bdev_super() is falled. I'd suggest
that removing this WARN_ON() in the related commit, or as
a following commit of the related branch of the pull request if
possible.

Thanks,
Gao Xiang

[Christoph Hellwig]

On Mon, Jul 31, 2023 at 06:58:14PM +0800, Gao Xiang wrote:
> Previously, deactivate_locked_super() or .kill_sb() will only be
> called after fill_super is called, and .s_magic will be set at
> the very beginning of erofs_fc_fill_super().
>
> After ("fs: open block device after superblock creation"), such
> convension is changed now. Yet at a quick glance,
>
> WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);
>
> in erofs_kill_sb() can be removed since deactivate_locked_super()
> will also be called if setup_bdev_super() is falled. I'd suggest
> that removing this WARN_ON() in the related commit, or as
> a following commit of the related branch of the pull request if
> possible.

Agreed. I wonder if we should really call into ->kill_sb before
calling into fill_super, but I need to carefull look into the
details.

[Christian Brauner]

I think checking for s_magic in erofs kill sb is wrong as it introduces
a dependency on both fill_super() having been called and that s_magic is
initialized first. If someone reorders erofs_kill_sb() such that s_magic
is only filled in once everything else succeeded it would cause the same
bug. That doesn't sound nice to me.

I think ->fill_super() should only be called after successfull
superblock allocation and after the device has been successfully opened.
Just as this code does now. So ->kill_sb() should only be called after
we're guaranteed that ->fill_super() has been called.

We already mostly express that logic through the fs_context object.
Anything that's allocated in fs_context->init_fs_context() is freed in
fs_context->free() before fill_super() is called. After ->fill_super()
is called fs_context->s_fs_info will have been transferred to
sb->s_fs_info and will have to be killed via ->kill_sb().

Does that make sense?

[Christian Brauner]

Uh, no. I vasty underestimated how sensitive that change would be. Plus
arguably ->kill_sb() really should be callable once the sb is visible.

Are you looking into this or do you want me to, Christoph?

[Gao Xiang]

On 2023/7/31 20:43, Christian Brauner wrote:
> On Mon, Jul 31, 2023 at 01:16:22PM +0200, Christoph Hellwig wrote:
>> On Mon, Jul 31, 2023 at 06:58:14PM +0800, Gao Xiang wrote:
>>> Previously, deactivate_locked_super() or .kill_sb() will only be
>>> called after fill_super is called, and .s_magic will be set at
>>> the very beginning of erofs_fc_fill_super().
>>>
>>> After ("fs: open block device after superblock creation"), such
>>> convension is changed now. Yet at a quick glance,
>>>
>>> WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC);
>>>
>>> in erofs_kill_sb() can be removed since deactivate_locked_super()
>>> will also be called if setup_bdev_super() is falled. I'd suggest
>>> that removing this WARN_ON() in the related commit, or as
>>> a following commit of the related branch of the pull request if
>>> possible.
>>
>> Agreed. I wonder if we should really call into ->kill_sb before
>> calling into fill_super, but I need to carefull look into the
>> details.
>
> I think checking for s_magic in erofs kill sb is wrong as it introduces
> a dependency on both fill_super() having been called and that s_magic is
> initialized first. If someone reorders erofs_kill_sb() such that s_magic
> is only filled in once everything else succeeded it would cause the same
> bug. That doesn't sound nice to me.

Many many years ago, strange .kill_sb called on our smartphone products
without proper call chain. That was why it was added and s_magic was
initialized first and at least it reminds a slight behavior change for
us (this time).

Anyway, I also think it's almost useless upstream so I'm fine to drop
this WARN_ON().

Thanks,
Gao Xiang

[Christoph Hellwig]

On Mon, Jul 31, 2023 at 03:22:28PM +0200, Christian Brauner wrote:
> Uh, no. I vasty underestimated how sensitive that change would be. Plus
> arguably ->kill_sb() really should be callable once the sb is visible.
>
> Are you looking into this or do you want me to, Christoph?

I'm planning to look into it, but I won't get to it before tomorrow.

[Christian Brauner]

Ok, let me go through the callsites and make sure that all callers are
safe. I think we should just continue calling deactivate_locked_super()
exactly the way we do right now but remove shenanigans like the one we
have in erofs_kill_sb().

[Christian Brauner]

The only filesystem that did implicitly rely on fill_super() having been
called was indeed - sorry to single it out - erofs. Everyone else
doesn't have that sort of dependency afaict.

fs/erofs/super.c: return get_tree_bdev(fc, erofs_fc_fill_super);
=> ->kill_sb() has requirement that ->fill_super() has been called.

So I went and looked at all users of

(1) get_tree_bdev() -> new mount api
(2) mount_bdev -> old mount api

which are the two functions that were changed in Jan's and Christoph's
patch.

I'm listing the results below. One thing to note is that only 40% (10
out of the 26 I looked at) of block based filesystems reliant on
higher-level fs/super.c helpers directly (e.g., that excludes btrfs)
have converted to the new mount api.

In any case, Gao, can you remove that dependency of erofs_kill_sb() on
erofs_fill_super(), please? Once that hits upstream the syzbot report
will go away.

fs/exfat/super.c: return get_tree_bdev(fc, exfat_fill_super);
fs/ntfs3/super.c: return get_tree_bdev(fc, ntfs_fill_super);
fs/squashfs/super.c: return get_tree_bdev(fc, squashfs_fill_super);
fs/ext4/super.c: return get_tree_bdev(fc, ext4_fill_super);
fs/xfs/xfs_super.c: return get_tree_bdev(fc, xfs_fs_fill_super);
fs/adfs/super.c: return mount_bdev(fs_type, flags, dev_name, data, adfs_fill_super);
fs/befs/linuxvfs.c: return mount_bdev(fs_type, flags, dev_name, data, befs_fill_super);
fs/bfs/inode.c: return mount_bdev(fs_type, flags, dev_name, data, bfs_fill_super);
fs/ext2/super.c: return mount_bdev(fs_type, flags, dev_name, data, ext2_fill_super);
fs/fat/namei_msdos.c: return mount_bdev(fs_type, flags, dev_name, data, msdos_fill_super);
fs/fat/namei_vfat.c: return mount_bdev(fs_type, flags, dev_name, data, vfat_fill_super);
fs/freevxfs/vxfs_super.c: return mount_bdev(fs_type, flags, dev_name, data, vxfs_fill_super);
fs/hfs/super.c: return mount_bdev(fs_type, flags, dev_name, data, hfs_fill_super);
fs/hfsplus/super.c: return mount_bdev(fs_type, flags, dev_name, data, hfsplus_fill_super);
fs/hpfs/super.c: return mount_bdev(fs_type, flags, dev_name, data, hpfs_fill_super);
fs/isofs/inode.c: return mount_bdev(fs_type, flags, dev_name, data, isofs_fill_super);
fs/jfs/super.c: return mount_bdev(fs_type, flags, dev_name, data, jfs_fill_super);
fs/minix/inode.c: return mount_bdev(fs_type, flags, dev_name, data, minix_fill_super);
fs/ntfs/super.c: return mount_bdev(fs_type, flags, dev_name, data, ntfs_fill_super);
fs/ocfs2/super.c: return mount_bdev(fs_type, flags, dev_name, data, ocfs2_fill_super);
fs/omfs/inode.c: return mount_bdev(fs_type, flags, dev_name, data, omfs_fill_super);
fs/qnx6/inode.c: return mount_bdev(fs_type, flags, dev_name, data, qnx6_fill_super);
fs/sysv/super.c: return mount_bdev(fs_type, flags, dev_name, data, sysv_fill_super);
fs/udf/super.c: return mount_bdev(fs_type, flags, dev_name, data, udf_fill_super);
fs/ufs/super.c: return mount_bdev(fs_type, flags, dev_name, data, ufs_fill_super);
=> no custom ->kill_sb() method.

fs/cramfs/inode.c: ret = get_tree_bdev(fc, cramfs_blkdev_fill_super);
fs/fuse/inode.c: err = get_tree_bdev(fsc, fuse_fill_super);
fs/gfs2/ops_fstype.c: error = get_tree_bdev(fc, gfs2_fill_super);
fs/romfs/super.c: ret = get_tree_bdev(fc, romfs_fill_super);
fs/affs/super.c: return mount_bdev(fs_type, flags, dev_name, data, affs_fill_super);
fs/efs/super.c: return mount_bdev(fs_type, flags, dev_name, data, efs_fill_super);
fs/f2fs/super.c: return mount_bdev(fs_type, flags, dev_name, data, f2fs_fill_super);
fs/qnx4/inode.c: return mount_bdev(fs_type, flags, dev_name, data, qnx4_fill_super);
fs/reiserfs/super.c: return mount_bdev(fs_type, flags, dev_name, data, reiserfs_fill_super);
fs/zonefs/super.c: return mount_bdev(fs_type, flags, dev_name, data, zonefs_fill_super);
=> ->kill_sb() has no requirement that ->fill_super() has been called.

[Gao Xiang]

On 2023/7/31 15:57, syzbot wrote:
> Hello,
>
> syzbot found the following issue on:
>
> HEAD commit: d7b3af5a77e8 Add linux-next specific files for 20230728
> git tree: linux-next
> console+strace: https://syzkaller.appspot.com/x/log.txt?x=13018b26a80000
> kernel config: https://syzkaller.appspot.com/x/.config?x=62dd327c382e3fe
> dashboard link: https://syzkaller.appspot.com/bug?extid=69c477e882e44ce41ad9
> compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=17b490c5a80000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=139a9c7ea80000
>
> Downloadable assets:
> disk image: https://storage.googleapis.com/syzbot-assets/5efa5e68267f/disk-d7b3af5a.raw.xz
> vmlinux: https://storage.googleapis.com/syzbot-assets/b1f5d3e10263/vmlinux-d7b3af5a.xz
> kernel image: https://storage.googleapis.com/syzbot-assets/57cab469d186/bzImage-d7b3af5a.xz
> mounted in repro: https://storage.googleapis.com/syzbot-assets/2cf2189f623b/mount_0.gz

#syz test: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+69c477...@syzkaller.appspotmail.com

Tested on:

commit: 1c59d383 Merge tag 'linux-kselftest-nolibc-6.6-rc1' of..
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=14f819dfa80000
kernel config: https://syzkaller.appspot.com/x/.config?x=9678e210dd5e4a5f

dashboard link: https://syzkaller.appspot.com/bug?extid=69c477e882e44ce41ad9
compiler: gcc (Debian 12.2.0-14) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40

Note: no patches were applied.
Note: testing is done by a robot and is best-effort only.

[Gao Xiang]

#syz invalid

since commit 4da3c7183e18 ("erofs: drop unnecessary WARN_ON() in
erofs_kill_sb()") merged ahead of

fs: open block device after superblock creation

so this issue only existed in some previous -next versions.
This report doesn't impact anything upstream.

Thanks,
Gao Xiang