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 <[email protected]>
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: [email protected]
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 [email protected].
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
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.
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
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.
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?
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?
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.
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?
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().
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.
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> will upstream this commit later this week after it lands -next.
Thanks,
Acked-by: Christian Brauner <[email protected]>
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 <[email protected]>