* [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb @ 2023-07-31 7:57 syzbot 2023-07-31 9:37 ` Christoph Hellwig 0 siblings, 1 reply; 13+ messages in thread From: syzbot @ 2023-07-31 7:57 UTC (permalink / raw) To: brauner, chao, hch, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang 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 <jack@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+69c477e882e44ce41ad9@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 syzkaller@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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 7:57 [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb syzbot @ 2023-07-31 9:37 ` Christoph Hellwig 2023-07-31 10:58 ` Gao Xiang 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2023-07-31 9:37 UTC (permalink / raw) To: syzbot Cc: brauner, chao, hch, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 9:37 ` Christoph Hellwig @ 2023-07-31 10:58 ` Gao Xiang 2023-07-31 11:16 ` Christoph Hellwig 0 siblings, 1 reply; 13+ messages in thread From: Gao Xiang @ 2023-07-31 10:58 UTC (permalink / raw) To: Christoph Hellwig, syzbot Cc: brauner, chao, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang On 2023/7/31 17:37, Christoph Hellwig wrote: > 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. 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 10:58 ` Gao Xiang @ 2023-07-31 11:16 ` Christoph Hellwig 2023-07-31 12:43 ` Christian Brauner 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2023-07-31 11:16 UTC (permalink / raw) To: Gao Xiang Cc: Christoph Hellwig, syzbot, brauner, chao, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 11:16 ` Christoph Hellwig @ 2023-07-31 12:43 ` Christian Brauner 2023-07-31 13:22 ` Christian Brauner 2023-07-31 13:29 ` [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb Gao Xiang 0 siblings, 2 replies; 13+ messages in thread From: Christian Brauner @ 2023-07-31 12:43 UTC (permalink / raw) To: Christoph Hellwig Cc: Gao Xiang, syzbot, chao, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang 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. 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 12:43 ` Christian Brauner @ 2023-07-31 13:22 ` Christian Brauner 2023-07-31 13:53 ` Christoph Hellwig 2023-07-31 13:29 ` [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb Gao Xiang 1 sibling, 1 reply; 13+ messages in thread From: Christian Brauner @ 2023-07-31 13:22 UTC (permalink / raw) To: Christoph Hellwig Cc: Gao Xiang, syzbot, chao, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang On Mon, Jul 31, 2023 at 02:43:39PM +0200, 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. > > 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? 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 13:22 ` Christian Brauner @ 2023-07-31 13:53 ` Christoph Hellwig 2023-07-31 13:57 ` Christian Brauner 0 siblings, 1 reply; 13+ messages in thread From: Christoph Hellwig @ 2023-07-31 13:53 UTC (permalink / raw) To: Christian Brauner Cc: Christoph Hellwig, Gao Xiang, syzbot, chao, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 13:53 ` Christoph Hellwig @ 2023-07-31 13:57 ` Christian Brauner 2023-07-31 16:32 ` Christian Brauner 0 siblings, 1 reply; 13+ messages in thread From: Christian Brauner @ 2023-07-31 13:57 UTC (permalink / raw) To: Christoph Hellwig Cc: Gao Xiang, syzbot, chao, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang On Mon, Jul 31, 2023 at 03:53:25PM +0200, Christoph Hellwig wrote: > 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. 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(). ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 13:57 ` Christian Brauner @ 2023-07-31 16:32 ` Christian Brauner 2023-08-01 1:47 ` [PATCH] erofs: drop unnecessary WARN_ON() in erofs_kill_sb() Gao Xiang 0 siblings, 1 reply; 13+ messages in thread From: Christian Brauner @ 2023-07-31 16:32 UTC (permalink / raw) To: Christoph Hellwig, Gao Xiang Cc: syzbot, chao, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, xiang On Mon, Jul 31, 2023 at 03:57:57PM +0200, Christian Brauner wrote: > On Mon, Jul 31, 2023 at 03:53:25PM +0200, Christoph Hellwig wrote: > > 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. > > 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(). 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] erofs: drop unnecessary WARN_ON() in erofs_kill_sb() 2023-07-31 16:32 ` Christian Brauner @ 2023-08-01 1:47 ` Gao Xiang 2023-08-01 7:19 ` Christian Brauner 2023-08-01 7:51 ` Christoph Hellwig 0 siblings, 2 replies; 13+ messages in thread From: Gao Xiang @ 2023-08-01 1:47 UTC (permalink / raw) To: linux-erofs Cc: LKML, Christian Brauner, Christoph Hellwig, linux-fsdevel, Gao Xiang Previously, .kill_sb() will be called only after fill_super fails. It will be changed [1]. Besides, checking for s_magic in erofs_kill_sb() is unnecessary from any point of view. Let's get rid of it now. [1] https://lore.kernel.org/r/20230731-flugbereit-wohnlage-78acdf95ab7e@brauner Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com> --- will upstream this commit later this week after it lands -next. fs/erofs/super.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/fs/erofs/super.c b/fs/erofs/super.c index 9d6a3c6158bd..566f68ddfa36 100644 --- a/fs/erofs/super.c +++ b/fs/erofs/super.c @@ -889,8 +889,6 @@ static void erofs_kill_sb(struct super_block *sb) { struct erofs_sb_info *sbi; - WARN_ON(sb->s_magic != EROFS_SUPER_MAGIC); - /* pseudo mount for anon inodes */ if (sb->s_flags & SB_KERNMOUNT) { kill_anon_super(sb); -- 2.24.4 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] erofs: drop unnecessary WARN_ON() in erofs_kill_sb() 2023-08-01 1:47 ` [PATCH] erofs: drop unnecessary WARN_ON() in erofs_kill_sb() Gao Xiang @ 2023-08-01 7:19 ` Christian Brauner 2023-08-01 7:51 ` Christoph Hellwig 1 sibling, 0 replies; 13+ messages in thread From: Christian Brauner @ 2023-08-01 7:19 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-erofs, LKML, Christoph Hellwig, linux-fsdevel On Tue, Aug 01, 2023 at 09:47:37AM +0800, Gao Xiang wrote: > Previously, .kill_sb() will be called only after fill_super fails. > It will be changed [1]. > > Besides, checking for s_magic in erofs_kill_sb() is unnecessary from > any point of view. Let's get rid of it now. > > [1] https://lore.kernel.org/r/20230731-flugbereit-wohnlage-78acdf95ab7e@brauner > Signed-off-by: Gao Xiang <hsiangkao@linux.alibaba.com> > --- > will upstream this commit later this week after it lands -next. Thanks, Acked-by: Christian Brauner <brauner@kernel.org> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] erofs: drop unnecessary WARN_ON() in erofs_kill_sb() 2023-08-01 1:47 ` [PATCH] erofs: drop unnecessary WARN_ON() in erofs_kill_sb() Gao Xiang 2023-08-01 7:19 ` Christian Brauner @ 2023-08-01 7:51 ` Christoph Hellwig 1 sibling, 0 replies; 13+ messages in thread From: Christoph Hellwig @ 2023-08-01 7:51 UTC (permalink / raw) To: Gao Xiang Cc: linux-erofs, LKML, Christian Brauner, Christoph Hellwig, linux-fsdevel On Tue, Aug 01, 2023 at 09:47:37AM +0800, Gao Xiang wrote: > Previously, .kill_sb() will be called only after fill_super fails. > It will be changed [1]. > > Besides, checking for s_magic in erofs_kill_sb() is unnecessary from > any point of view. Let's get rid of it now. Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb 2023-07-31 12:43 ` Christian Brauner 2023-07-31 13:22 ` Christian Brauner @ 2023-07-31 13:29 ` Gao Xiang 1 sibling, 0 replies; 13+ messages in thread From: Gao Xiang @ 2023-07-31 13:29 UTC (permalink / raw) To: Christian Brauner, Christoph Hellwig Cc: syzbot, chao, huyue2, jack, jefflexu, linkinjeon, linux-erofs, linux-fsdevel, linux-kernel, sj1557.seo, syzkaller-bugs, 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 > > 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-08-01 7:51 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-07-31 7:57 [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb syzbot 2023-07-31 9:37 ` Christoph Hellwig 2023-07-31 10:58 ` Gao Xiang 2023-07-31 11:16 ` Christoph Hellwig 2023-07-31 12:43 ` Christian Brauner 2023-07-31 13:22 ` Christian Brauner 2023-07-31 13:53 ` Christoph Hellwig 2023-07-31 13:57 ` Christian Brauner 2023-07-31 16:32 ` Christian Brauner 2023-08-01 1:47 ` [PATCH] erofs: drop unnecessary WARN_ON() in erofs_kill_sb() Gao Xiang 2023-08-01 7:19 ` Christian Brauner 2023-08-01 7:51 ` Christoph Hellwig 2023-07-31 13:29 ` [syzbot] [erofs?] [fat?] WARNING in erofs_kill_sb Gao Xiang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).