[syzbot] [btrfs?] kernel BUG in btrfs_get_ordered_extents_for_logging

[syzbot] [btrfs?] kernel BUG in btrfs_get_ordered_extents_for_logging

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    5be63fc19fca Linux 6.11-rc5
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11f0800d980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=a0455552d0b27491
dashboard link: https://syzkaller.appspot.com/bug?extid=4704b3cc972bd76024f1
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=16692a29980000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/cad8c335ccde/disk-5be63fc1.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/c4d2dd818c33/vmlinux-5be63fc1.xz
kernel image: https://storage.googleapis.com/syzbot-assets/ab5cbe08133f/bzImage-5be63fc1.xz
mounted in repro #1: https://storage.googleapis.com/syzbot-assets/9b6630601ecf/mount_0.gz
mounted in repro #2: https://storage.googleapis.com/syzbot-assets/1eee9b57c8d5/mount_3.gz
mounted in repro #3: https://storage.googleapis.com/syzbot-assets/ba6ddfb704dc/mount_4.gz
mounted in repro #4: https://storage.googleapis.com/syzbot-assets/e681ae9a9992/mount_9.gz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+4704b3cc972bd76024f1@syzkaller.appspotmail.com

------------[ cut here ]------------
kernel BUG at fs/btrfs/ordered-data.c:1018!
Oops: invalid opcode: 0000 [#1] PREEMPT SMP KASAN PTI
CPU: 1 UID: 0 PID: 6479 Comm: syz.4.61 Not tainted 6.11.0-rc5-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024
RIP: 0010:btrfs_get_ordered_extents_for_logging+0x4fd/0x500 fs/btrfs/ordered-data.c:1018
Code: f7 07 90 0f 0b e8 23 f1 df fd 48 c7 c7 40 8a 2c 8c 48 c7 c6 e0 8c 2c 8c 48 c7 c2 20 8a 2c 8c b9 fa 03 00 00 e8 e4 ad f7 07 90 <0f> 0b 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f
RSP: 0018:ffffc90009627938 EFLAGS: 00010246
RAX: 0000000000000055 RBX: 0000000000000000 RCX: 3784e5b0ceb58c00
RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
RBP: ffff88806597d690 R08: ffffffff817402ac R09: 1ffff920012c4ec8
R10: dffffc0000000000 R11: fffff520012c4ec9 R12: 0000000000000000
R13: dffffc0000000000 R14: ffffc90009627ae0 R15: ffff88806597d690
FS:  00007f60c8eec6c0(0000) GS:ffff8880b9300000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fa599c4220a CR3: 000000007953c000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 btrfs_sync_file+0x929/0x10f0 fs/btrfs/file.c:1712
 generic_write_sync include/linux/fs.h:2821 [inline]
 btrfs_do_write_iter+0x5e2/0x760 fs/btrfs/file.c:1515
 new_sync_write fs/read_write.c:497 [inline]
 vfs_write+0xa72/0xc90 fs/read_write.c:590
 ksys_pwrite64 fs/read_write.c:705 [inline]
 __do_sys_pwrite64 fs/read_write.c:715 [inline]
 __se_sys_pwrite64 fs/read_write.c:712 [inline]
 __x64_sys_pwrite64+0x1aa/0x230 fs/read_write.c:712
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xf3/0x230 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7f60c8179e79
Code: ff ff c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 40 00 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 a8 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f60c8eec038 EFLAGS: 00000246 ORIG_RAX: 0000000000000012
RAX: ffffffffffffffda RBX: 00007f60c8316130 RCX: 00007f60c8179e79
RDX: 000000000000003d RSI: 00000000200001c0 RDI: 0000000000000007
RBP: 00007f60c81e793e R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 0000000000000001 R14: 00007f60c8316130 R15: 00007ffd4fce3d08
 </TASK>
Modules linked in:
---[ end trace 0000000000000000 ]---
RIP: 0010:btrfs_get_ordered_extents_for_logging+0x4fd/0x500 fs/btrfs/ordered-data.c:1018
Code: f7 07 90 0f 0b e8 23 f1 df fd 48 c7 c7 40 8a 2c 8c 48 c7 c6 e0 8c 2c 8c 48 c7 c2 20 8a 2c 8c b9 fa 03 00 00 e8 e4 ad f7 07 90 <0f> 0b 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 90 66 0f 1f
RSP: 0018:ffffc90009627938 EFLAGS: 00010246
RAX: 0000000000000055 RBX: 0000000000000000 RCX: 3784e5b0ceb58c00
RDX: 0000000000000000 RSI: 0000000080000000 RDI: 0000000000000000
RBP: ffff88806597d690 R08: ffffffff817402ac R09: 1ffff920012c4ec8
R10: dffffc0000000000 R11: fffff520012c4ec9 R12: 0000000000000000
R13: dffffc0000000000 R14: ffffc90009627ae0 R15: ffff88806597d690
FS:  00007f60c8eec6c0(0000) GS:ffff8880b9300000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f449fcc2723 CR3: 000000007953c000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400


---
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.

If the report is already addressed, 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 overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)

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

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

Re: [syzbot] Re: [btrfs?] kernel BUG in btrfs_get_ordered_extents_for_logging

[syzbot]

For archival purposes, forwarding an incoming command email to
linux-kernel@vger.kernel.org.

***

Subject: Re: [btrfs?] kernel BUG in btrfs_get_ordered_extents_for_logging
Author: lizhi.xu@windriver.com

or i_mmap_lock ?

#syz test

diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index 82a68394a89c..d0187e1fb941 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -1015,7 +1015,8 @@ void btrfs_get_ordered_extents_for_logging(struct btrfs_inode *inode,
 {
 	struct rb_node *n;
 
-	ASSERT(inode_is_locked(&inode->vfs_inode));
+	ASSERT(inode_is_locked(&inode->vfs_inode) ||
+	       rwsem_is_locked(&inode->i_mmap_lock));
 
 	spin_lock_irq(&inode->ordered_tree_lock);
 	for (n = rb_first(&inode->ordered_tree); n; n = rb_next(n)) {

Re: [syzbot] [btrfs?] kernel BUG in btrfs_get_ordered_extents_for_logging

[syzbot]

Hello,

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

Reported-by: syzbot+4704b3cc972bd76024f1@syzkaller.appspotmail.com
Tested-by: syzbot+4704b3cc972bd76024f1@syzkaller.appspotmail.com

Tested on:

commit:         20371ba1 Merge tag 'drm-fixes-2024-08-30' of https://g..
git tree:       upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=17f1608f980000
kernel config:  https://syzkaller.appspot.com/x/.config?x=a0455552d0b27491
dashboard link: https://syzkaller.appspot.com/bug?extid=4704b3cc972bd76024f1
compiler:       Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40
patch:          https://syzkaller.appspot.com/x/patch.diff?x=179f1edb980000

Note: testing is done by a robot and is best-effort only.

[PATCH] btrfs: Add assert or condition

[Lizhi Xu]

When the value of fsync_skip_inode_lock is true, i_mmap_lock is used,
so add it or condition in the ASSERT. 

Reported-and-tested-by: syzbot+4704b3cc972bd76024f1@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=4704b3cc972bd76024f1
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/btrfs/ordered-data.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
index 82a68394a89c..d0187e1fb941 100644
--- a/fs/btrfs/ordered-data.c
+++ b/fs/btrfs/ordered-data.c
@@ -1015,7 +1015,8 @@ void btrfs_get_ordered_extents_for_logging(struct btrfs_inode *inode,
 {
 	struct rb_node *n;
 
-	ASSERT(inode_is_locked(&inode->vfs_inode));
+	ASSERT(inode_is_locked(&inode->vfs_inode) ||
+	       rwsem_is_locked(&inode->i_mmap_lock));
 
 	spin_lock_irq(&inode->ordered_tree_lock);
 	for (n = rb_first(&inode->ordered_tree); n; n = rb_next(n)) {
-- 
2.43.0

Re: [PATCH] btrfs: Add assert or condition

[Filipe Manana]

On Sat, Aug 31, 2024 at 1:36 AM Lizhi Xu <lizhi.xu@windriver.com> wrote:
>
> When the value of fsync_skip_inode_lock is true, i_mmap_lock is used,
> so add it or condition in the ASSERT.
>
> Reported-and-tested-by: syzbot+4704b3cc972bd76024f1@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=4704b3cc972bd76024f1
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  fs/btrfs/ordered-data.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ordered-data.c b/fs/btrfs/ordered-data.c
> index 82a68394a89c..d0187e1fb941 100644
> --- a/fs/btrfs/ordered-data.c
> +++ b/fs/btrfs/ordered-data.c
> @@ -1015,7 +1015,8 @@ void btrfs_get_ordered_extents_for_logging(struct btrfs_inode *inode,
>  {
>         struct rb_node *n;
>
> -       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/

>
>         spin_lock_irq(&inode->ordered_tree_lock);
>         for (n = rb_first(&inode->ordered_tree); n; n = rb_next(n)) {
> --
> 2.43.0
>
>

Re: [PATCH] btrfs: Add assert or condition

[Lizhi Xu]

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.