linux-kernel - Re: [syzbot] [erofs?] [fat?] WARNING in erofs_kill

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230731-flugbereit-wohnlage-78acdf95ab7e@brauner>
Date:   Mon, 31 Jul 2023 18:32:52 +0200
From:   Christian Brauner <brauner@...nel.org>
To:     Christoph Hellwig <hch@....de>,
        Gao Xiang <hsiangkao@...ux.alibaba.com>
Cc:     syzbot <syzbot+69c477e882e44ce41ad9@...kaller.appspotmail.com>,
        chao@...nel.org, huyue2@...lpad.com, jack@...e.cz,
        jefflexu@...ux.alibaba.com, linkinjeon@...nel.org,
        linux-erofs@...ts.ozlabs.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, sj1557.seo@...sung.com,
        syzkaller-bugs@...glegroups.com, xiang@...nel.org
Subject: Re: [syzbot] [erofs?] [fat?] WARNING 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.