linux-kernel - Re: [PATCH] btrfs: Add assert or condition

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] [day] [month] [year] [list]

Message-ID: <20240831124141.11022-1-lizhi.xu@windriver.com>
Date: Sat, 31 Aug 2024 20:41:41 +0800
From: Lizhi Xu <lizhi.xu@...driver.com>
To: <fdmanana@...nel.org>
CC: <clm@...com>, <dsterba@...e.com>, <josef@...icpanda.com>,
        <linux-btrfs@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <lizhi.xu@...driver.com>,
        <syzbot+4704b3cc972bd76024f1@...kaller.appspotmail.com>,
        <syzkaller-bugs@...glegroups.com>
Subject: Re: [PATCH] btrfs: Add assert or condition

On Sat, 31 Aug 2024 11:55:53 +0100, Filipe Manana wrote:
> > -       ASSERT(inode_is_locked(&inode->vfs_inode));
> > +       ASSERT(inode_is_locked(&inode->vfs_inode) ||
> > +              rwsem_is_locked(&inode->i_mmap_lock));
> 
> This definitely fixes the syzbot report, in the sense the assertion
> won't fail anymore.
> But it's wrong, very, very, very, very wrong.
> 
> The inode must be locked during the course of the fsync, that's why
> the assertion is there.
> 
> Why do you think it's ok to not have the inode locked?
> Have you done any analysis about that?
> 
> You mention "fsync_skip_inode_lock is true" in the changelog, but have
> you checked where and why it's set to true?
> 
> Where we set it to true, at btrfs_direct_write(), there's a comment
> which explains it's to avoid a deadlock on the inode lock at
> btrfs_sync_file().
> 
> This is a perfect example of trying a patch not only without having
> any idea how the code works but also being very lazy,
> as there's a very explicit comment in the code about why the variable
> is set to true, and even much more detailed in the
> change log of the commit that introduced it.
> 
> And btw, there's already a patch to fix this issue:
> 
> https://lore.kernel.org/linux-btrfs/717029440fe379747b9548a9c91eb7801bc5a813.1724972507.git.fdmanana@suse.com/
In your patch, I get what the mean of fsync_skip_inode_lock.

Thanks.