[syzbot] [btrfs?] WARNING in create_pending_snapshot

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 305230142ae0 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: upstream
console+strace: https://syzkaller.appspot.com/x/log.txt?x=11777b60e80000
kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
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=14900138e80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=10907197680000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/0aab25a831ba/disk-30523014.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/9d1b7b8fdf8a/vmlinux-30523014.xz
kernel image: https://storage.googleapis.com/syzbot-assets/e9b6822fcd5f/bzImage-30523014.xz
mounted in repro: https://storage.googleapis.com/syzbot-assets/82e806de5984/mount_0.gz

The issue was bisected to:

commit 6ed05643ddb166c0fddabac8ee092659006214a9
Author: Boris Burkov <bo...@bur.io>
Date: Wed Jun 28 18:00:05 2023 +0000

btrfs: create qgroup earlier in snapshot creation

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=117a7050e80000
final oops: https://syzkaller.appspot.com/x/report.txt?x=137a7050e80000
console output: https://syzkaller.appspot.com/x/log.txt?x=157a7050e80000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+4d8101...@syzkaller.appspotmail.com
Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")

------------[ cut here ]------------
BTRFS: Transaction aborted (error -17)
WARNING: CPU: 0 PID: 5057 at fs/btrfs/transaction.c:1778 create_pending_snapshot+0x25f4/0x2b70 fs/btrfs/transaction.c:1778
Modules linked in:
CPU: 0 PID: 5057 Comm: syz-executor225 Not tainted 6.6.0-syzkaller-15365-g305230142ae0 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
RIP: 0010:create_pending_snapshot+0x25f4/0x2b70 fs/btrfs/transaction.c:1778
Code: f8 fd 48 c7 c7 00 43 ab 8b 89 de e8 76 4b be fd 0f 0b e9 30 f3 ff ff e8 7a 8d f8 fd 48 c7 c7 00 43 ab 8b 89 de e8 5c 4b be fd <0f> 0b e9 f8 f6 ff ff e8 60 8d f8 fd 48 c7 c7 00 43 ab 8b 89 de e8
RSP: 0018:ffffc90003abf580 EFLAGS: 00010246
RAX: 10fb7cf24e10ea00 RBX: 00000000ffffffef RCX: ffff888023ea9dc0
RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000000
RBP: ffffc90003abf870 R08: ffffffff81547c82 R09: 1ffff11017305172
R10: dffffc0000000000 R11: ffffed1017305173 R12: ffff888078ae2878
R13: 00000000ffffffef R14: 0000000000000000 R15: ffff888078ae2818
FS: 000055555667d380(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f6ff7bf2304 CR3: 0000000079f17000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
create_pending_snapshots+0x195/0x1d0 fs/btrfs/transaction.c:1967
btrfs_commit_transaction+0xf1c/0x3730 fs/btrfs/transaction.c:2440
create_snapshot+0x4a5/0x7e0 fs/btrfs/ioctl.c:845
btrfs_mksubvol+0x5d0/0x750 fs/btrfs/ioctl.c:995
btrfs_mksnapshot+0xb5/0xf0 fs/btrfs/ioctl.c:1041
__btrfs_ioctl_snap_create+0x344/0x460 fs/btrfs/ioctl.c:1294
btrfs_ioctl_snap_create+0x13c/0x190 fs/btrfs/ioctl.c:1321
btrfs_ioctl+0xbbf/0xd40
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:871 [inline]
__se_sys_ioctl+0xf8/0x170 fs/ioctl.c:857
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x7f2f791127b9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 61 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:00007ffc5dc597b8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0030656c69662f2e RCX: 00007f2f791127b9
RDX: 0000000020000a80 RSI: 0000000050009401 RDI: 0000000000000004
RBP: 00007f2f7918b610 R08: 00007ffc5dc59988 R09: 00007ffc5dc59988
R10: 00007ffc5dc59988 R11: 0000000000000246 R12: 0000000000000001
R13: 00007ffc5dc59978 R14: 0000000000000001 R15: 0000000000000001
</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 syzk...@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 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

[Lizhi Xu]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 752acff2c734..bae6c54e4f87 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3806,6 +3806,10 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
}

if (sa->create) {
+ if (sa->qgroupid == BTRFS_FIRST_FREE_OBJECTID) {
+ ret = -EINVAL;
+ goto out;
+ }
ret = btrfs_create_qgroup(trans, sa->qgroupid);
} else {
ret = btrfs_remove_qgroup(trans, sa->qgroupid);

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
WARNING: lock held when returning to user space in btrfs_ioctl_qgroup_create

BTRFS info (device loop0): clearing compat-ro feature flag for FREE_SPACE_TREE (0x1)
BTRFS info (device loop0): clearing compat-ro feature flag for FREE_SPACE_TREE_VALID (0x2)
BTRFS info (device loop0): checking UUID tree
================================================
WARNING: lock held when returning to user space!
6.6.0-syzkaller-15365-g305230142ae0-dirty #0 Not tainted
------------------------------------------------
syz-executor.0/5537 is leaving the kernel with locks still held!
3 locks held by syz-executor.0/5537:
#0: ffff88806e8a2608 (sb_internal#2){.+.+}-{0:0}, at: btrfs_ioctl_qgroup_create+0x103/0x200 fs/btrfs/ioctl.c:3802
#1: ffff888052082390 (btrfs_trans_num_writers){++++}-{0:0}, at: join_transaction+0x401/0xce0 fs/btrfs/transaction.c:294
#2: ffff8880520823b8 (btrfs_trans_num_extwriters){++++}-{0:0}, at: join_transaction+0x401/0xce0 fs/btrfs/transaction.c:294


Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
console output: https://syzkaller.appspot.com/x/log.txt?x=17cb5797680000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=11c9591f680000

[Lizhi Xu]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c

index 752acff2c734..21cf7a7f18ab 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3799,6 +3799,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
goto out;
}

+ if (sa->create && sa->qgroupid == BTRFS_FIRST_FREE_OBJECTID) {

+ ret = -EINVAL;
+ goto out;
+ }

+
trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
ret = PTR_ERR(trans);

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+4d8101...@syzkaller.appspotmail.com

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

console output: https://syzkaller.appspot.com/x/log.txt?x=17929338e80000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=10c4f8b7680000

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

[Lizhi Xu]

r0 = open(&(0x7f0000000080)='./file0\x00', 0x0, 0x0)
ioctl$BTRFS_IOC_QUOTA_CTL(r0, 0xc0109428, &(0x7f0000000000)={0x1})
r1 = openat$cgroup_ro(0xffffffffffffff9c, &(0x7f0000000100)='blkio.bfq.time_recursive\x00', 0x275a, 0x0)
ioctl$BTRFS_IOC_QGROUP_CREATE(r1, 0x4010942a, &(0x7f0000000640)={0x1, 0x100})
r2 = openat(0xffffffffffffff9c, &(0x7f0000000500)='.\x00', 0x0, 0x0)
ioctl$BTRFS_IOC_SNAP_CREATE(r0, 0x50009401, &(0x7f0000000a80)={{r2},

From the logs, it can be seen that syz can execute to btrfs_ioctl_qgroup_create()
through two paths.
Syz enters btrfs_ioctl_qgroup_create() by calling ioctl$BTRFS_IOC_QGROUP_CREATE(
r1, 0x4010942a,&(0x7f000000 640)={0x1, 0x100}) or ioctl$BTRFS_IOC_SNAP_CREATE(r0,
0x50009401,&(0x7f000000 a80)={r2}," respectively;

The most crucial thing is that when calling ioctl$BTRFS_IOC_QGROUP_CREATE,
the passed parameter qgroupid value is 256, while BTRFS_FIRST_FREE_OBJECTID
is also equal to 256, indicating that the passed parameter qgroupid is
obviously incorrect.

Reported-and-tested-by: syzbot+4d8101...@syzkaller.appspotmail.com

Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")

Signed-off-by: Lizhi Xu <lizh...@windriver.com>
---
fs/btrfs/ioctl.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 752acff2c734..21cf7a7f18ab 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3799,6 +3799,11 @@ static long btrfs_ioctl_qgroup_create(struct file *file, void __user *arg)
goto out;
}

+ if (sa->create && sa->qgroupid == BTRFS_FIRST_FREE_OBJECTID) {
+ ret = -EINVAL;
+ goto out;
+ }
+
trans = btrfs_join_transaction(root);
if (IS_ERR(trans)) {
ret = PTR_ERR(trans);

--
2.25.1

[Qu Wenruo]

On 2023/11/10 22:18, Lizhi Xu wrote:
> r0 = open(&(0x7f0000000080)='./file0\x00', 0x0, 0x0)
> ioctl$BTRFS_IOC_QUOTA_CTL(r0, 0xc0109428, &(0x7f0000000000)={0x1})
> r1 = openat$cgroup_ro(0xffffffffffffff9c, &(0x7f0000000100)='blkio.bfq.time_recursive\x00', 0x275a, 0x0)
> ioctl$BTRFS_IOC_QGROUP_CREATE(r1, 0x4010942a, &(0x7f0000000640)={0x1, 0x100})
> r2 = openat(0xffffffffffffff9c, &(0x7f0000000500)='.\x00', 0x0, 0x0)
> ioctl$BTRFS_IOC_SNAP_CREATE(r0, 0x50009401, &(0x7f0000000a80)={{r2},
>
> From the logs, it can be seen that syz can execute to btrfs_ioctl_qgroup_create()
> through two paths.
> Syz enters btrfs_ioctl_qgroup_create() by calling ioctl$BTRFS_IOC_QGROUP_CREATE(
> r1, 0x4010942a,&(0x7f000000 640)={0x1, 0x100}) or ioctl$BTRFS_IOC_SNAP_CREATE(r0,
> 0x50009401,&(0x7f000000 a80)={r2}," respectively;
>
> The most crucial thing is that when calling ioctl$BTRFS_IOC_QGROUP_CREATE,
> the passed parameter qgroupid value is 256, while BTRFS_FIRST_FREE_OBJECTID
> is also equal to 256, indicating that the passed parameter qgroupid is
> obviously incorrect.

This conclusion looks incorrect to me.

Subvolumes are allowed to have any id in the range
[BTRFS_FIRST_TREE_OBJECTID, BTRFS_LAST_TREE_OBJECTID].

In fact, you can easily create a subvolume with 256 as its subvolumeid.
Just create an empty fs, and create a new subvolume in it, then you got;

item 11 key (256 ROOT_ITEM 0) itemoff 12961 itemsize 439
generation 7 root_dirid 256 bytenr 30441472 byte_limit 0 bytes_used 16384
...

So it's completely valid.


The root cause is just snapshot creation conflicts with an existing qgroup.

Thanks,
Qu

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 401ea09ae4b8..d2b6e4d18c89 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
goto out;
}

- *objectid = root->free_objectid++;
+ while (find_qgroup_rb(root->fs_info, root->free_objectid++);
+ *objectid = root->free_objectid;
ret = 0;
out:
mutex_unlock(&root->objectid_mutex);

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

fs/btrfs/disk-io.c:4934:9: error: call to undeclared function 'find_qgroup_rb'; ISO C99 and later do not support implicit function declarations [-Werror,-Wimplicit-function-declaration]
fs/btrfs/disk-io.c:4934:61: error: expected ')'

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=124c62b7680000

[Edward Adam Davis]

diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 855a4f978761..05b4b8dd0fcb 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -425,4 +425,6 @@ bool btrfs_check_quota_leak(struct btrfs_fs_info *fs_info);
int btrfs_record_squota_delta(struct btrfs_fs_info *fs_info,
struct btrfs_squota_delta *delta);

+static struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
+ u64 qgroupid);
#endif
--
2.25.1

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

fs/btrfs/disk-io.c:4934:61: error: expected ')'


Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=16e441ff680000

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 401ea09ae4b8..d2b6e4d18c89 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
goto out;
}

- *objectid = root->free_objectid++;

+ while (find_qgroup_rb(root->fs_info, root->free_objectid++));

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

fs/btrfs/disk-io.c:4934: undefined reference to `find_qgroup_rb'

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=1263c55b680000

[Edward Adam Davis]

[syzbot]

patch: https://syzkaller.appspot.com/x/patch.diff?x=10a8c3c0e80000

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c

index 401ea09ae4b8..3bc6abbd64db 100644

--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
goto out;
}

- *objectid = root->free_objectid++;

+ while (exist_qgroup_rb(root->fs_info, root->free_objectid++));

+ *objectid = root->free_objectid;
ret = 0;
out:
mutex_unlock(&root->objectid_mutex);
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h

index 855a4f978761..a8da8e11734a 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -425,4 +425,11 @@ bool btrfs_check_quota_leak(struct btrfs_fs_info *fs_info);

int btrfs_record_squota_delta(struct btrfs_fs_info *fs_info,
struct btrfs_squota_delta *delta);

+static struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
+ u64 qgroupid);

+
+bool exist_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)
+{
+ return find_qgroup_rb(fs_info, qgroupid);
+}
#endif
--
2.25.1

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

fs/btrfs/qgroup.h:432: multiple definition of `exist_qgroup_rb'; fs/btrfs/super.o:fs/btrfs/qgroup.h:432: first defined here

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=17cc7338e80000

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 401ea09ae4b8..3bc6abbd64db 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
goto out;
}

- *objectid = root->free_objectid++;
+ while (exist_qgroup_rb(root->fs_info, root->free_objectid++));
+ *objectid = root->free_objectid;
ret = 0;
out:
mutex_unlock(&root->objectid_mutex);
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 855a4f978761..a8da8e11734a 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -425,4 +425,11 @@ bool btrfs_check_quota_leak(struct btrfs_fs_info *fs_info);
int btrfs_record_squota_delta(struct btrfs_fs_info *fs_info,
struct btrfs_squota_delta *delta);

+static struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
+ u64 qgroupid);
+

+static bool exist_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)

[syzbot]

Hello,

syzbot tried to test the proposed patch but the build/boot failed:

fs/btrfs/qgroup.h:433: undefined reference to `find_qgroup_rb'

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=11ce1eff680000

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 401ea09ae4b8..3bc6abbd64db 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
goto out;
}

- *objectid = root->free_objectid++;
+ while (exist_qgroup_rb(root->fs_info, root->free_objectid++));
+ *objectid = root->free_objectid;
ret = 0;
out:
mutex_unlock(&root->objectid_mutex);

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index edb84cc03237..3705e7d57057 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -171,7 +171,7 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
static void qgroup_rescan_zero_tracking(struct btrfs_fs_info *fs_info);

/* must be called with qgroup_ioctl_lock held */
-static struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
+struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
u64 qgroupid)
{
struct rb_node *n = fs_info->qgroup_tree.rb_node;
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h
index 855a4f978761..f2a95fe165f0 100644

--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -425,4 +425,11 @@ bool btrfs_check_quota_leak(struct btrfs_fs_info *fs_info);
int btrfs_record_squota_delta(struct btrfs_fs_info *fs_info,
struct btrfs_squota_delta *delta);

+struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+4d8101...@syzkaller.appspotmail.com

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

console output: https://syzkaller.appspot.com/x/log.txt?x=164c7d88e80000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=1047e947680000

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c

index 401ea09ae4b8..97050a3edc32 100644

--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
goto out;
}

- *objectid = root->free_objectid++;

+ while (find_qgroup_rb(root->fs_info, root->free_objectid++));

+ *objectid = root->free_objectid;
ret = 0;
out:
mutex_unlock(&root->objectid_mutex);
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index edb84cc03237..3705e7d57057 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -171,7 +171,7 @@ qgroup_rescan_init(struct btrfs_fs_info *fs_info, u64 progress_objectid,
static void qgroup_rescan_zero_tracking(struct btrfs_fs_info *fs_info);

/* must be called with qgroup_ioctl_lock held */
-static struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
+struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
u64 qgroupid)
{
struct rb_node *n = fs_info->qgroup_tree.rb_node;
diff --git a/fs/btrfs/qgroup.h b/fs/btrfs/qgroup.h

index 855a4f978761..96c6aa31ca91 100644
--- a/fs/btrfs/qgroup.h
+++ b/fs/btrfs/qgroup.h
@@ -425,4 +425,6 @@ bool btrfs_check_quota_leak(struct btrfs_fs_info *fs_info);

int btrfs_record_squota_delta(struct btrfs_fs_info *fs_info,
struct btrfs_squota_delta *delta);

+struct btrfs_qgroup *find_qgroup_rb(struct btrfs_fs_info *fs_info,
+ u64 qgroupid);

#endif
--
2.25.1

[Edward Adam Davis]

The create_snapshot will use the objectid that already exists in the qgroup_tree
tree, so when calculating the free_ojectid, it is added to determine whether it
exists in the qgroup_tree tree.

Reported-and-tested-by: syzbot+4d8101...@syzkaller.appspotmail.com

Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")

Signed-off-by: Edward Adam Davis <ead...@qq.com>
---
fs/btrfs/disk-io.c | 3 ++-
fs/btrfs/qgroup.c | 2 +-
fs/btrfs/qgroup.h | 2 ++
3 files changed, 5 insertions(+), 2 deletions(-)

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+4d8101...@syzkaller.appspotmail.com

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

console output: https://syzkaller.appspot.com/x/log.txt?x=13fa93e0e80000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=13fb5fdf680000

[Matthew Wilcox]

On Sat, Nov 11, 2023 at 01:06:01PM +0800, Edward Adam Davis wrote:
> +++ b/fs/btrfs/disk-io.c
> @@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
> goto out;
> }
>
> - *objectid = root->free_objectid++;
> + while (find_qgroup_rb(root->fs_info, root->free_objectid++));
> + *objectid = root->free_objectid;

This looks buggy to me. Let's say that free_objectid is currently 3.

Before, it would assign 3 to *objectid, and increment free_objectid to
4. After (assuming the loop terminates on first iteration), it will
increment free_objectid to 4, then assign 4 to *objectid.

I think you meant to write:

while (find_qgroup_rb(root->fs_info, root->free_objectid))
root->free_objectid++;
*objectid = root->free_objectid++;

And the lesson here is that more compact code is not necessarily more
correct code.

(I'm not making any judgement about whether this is the correct fix;
I don't understand btrfs well enough to have an opinion. Just that
this is not an equivalent transformation)

[Qu Wenruo]

On 2023/11/11 15:36, Edward Adam Davis wrote:
> The create_snapshot will use the objectid that already exists in the qgroup_tree
> tree, so when calculating the free_ojectid, it is added to determine whether it
> exists in the qgroup_tree tree.
>
> Reported-and-tested-by: syzbot+4d8101...@syzkaller.appspotmail.com
> Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
> Signed-off-by: Edward Adam Davis <ead...@qq.com>
> ---
> fs/btrfs/disk-io.c | 3 ++-
> fs/btrfs/qgroup.c | 2 +-
> fs/btrfs/qgroup.h | 2 ++
> 3 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 401ea09ae4b8..97050a3edc32 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
> goto out;
> }
>
> - *objectid = root->free_objectid++;
> + while (find_qgroup_rb(root->fs_info, root->free_objectid++));

I don't think this is correct.

Firstly you didn't take qgroup_ioctl_lock.

Secondly, please explain why you believe the free objectid of a
subvolume is related to the qgroup id?


For any one who really wants to fix the syzbot bug, please explain the
bug clearly before doing any fix.

If you can not explain the bug clearly, then you're doing it wrong.

Thanks,
Qu

[Edward Adam Davis]

On Sat, 11 Nov 2023 06:20:59 +0000, Matthew Wilcox wrote:
> > +++ b/fs/btrfs/disk-io.c
> > @@ -4931,7 +4931,8 @@ int btrfs_get_free_objectid(struct btrfs_root *root, u64 *objectid)
> > goto out;
> > }
> >
> > - *objectid = root->free_objectid++;
> > + while (find_qgroup_rb(root->fs_info, root->free_objectid++));
> > + *objectid = root->free_objectid;
>
> This looks buggy to me. Let's say that free_objectid is currently 3.
>
> Before, it would assign 3 to *objectid, and increment free_objectid to
> 4. After (assuming the loop terminates on first iteration), it will
> increment free_objectid to 4, then assign 4 to *objectid.
>
> I think you meant to write:
>
> while (find_qgroup_rb(root->fs_info, root->free_objectid))
> root->free_objectid++;
> *objectid = root->free_objectid++;

Yes, your guess is correct.

>
> And the lesson here is that more compact code is not necessarily more
> correct code.
>
> (I'm not making any judgement about whether this is the correct fix;
> I don't understand btrfs well enough to have an opinion. Just that
> this is not an equivalent transformation)

I don't have much knowledge about btrfs too, but one thing is clear: the qgroupid
taken by create_snapshot() is calculated from btrfs_get_free_ojectid().
At the same time, when calculating the new value in btrfs_get_free_ojectid(),
it is clearly unreasonable to not determine whether the new value exists in the
qgroup_tree tree.
Perhaps there are other methods to obtain a new qgroupid, but before obtaining
a new value, it is necessary to perform a duplicate value judgment on qgroup_tree,
otherwise similar problems may still occur.

edward

[Qu Wenruo]

Nope, it's totally wrong.

Qgroupid is bound to subvolumeid, thus getting a different id for
qgroupid is going to screw the whole thing up.

> Perhaps there are other methods to obtain a new qgroupid, but before obtaining
> a new value, it is necessary to perform a duplicate value judgment on qgroup_tree,
> otherwise similar problems may still occur.

If you don't really understand the context, the fix is never going to be
correct.

Thanks,
Qu

>
> edward
>
>

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index edb84cc03237..6cd89248e530 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -204,6 +204,7 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,
struct rb_node **p = &fs_info->qgroup_tree.rb_node;
struct rb_node *parent = NULL;
struct btrfs_qgroup *qgroup;
+ u64 objid;

/* Caller must have pre-allocated @prealloc. */
ASSERT(prealloc);
@@ -232,6 +233,7 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,

rb_link_node(&qgroup->node, parent, p);
rb_insert_color(&qgroup->node, &fs_info->qgroup_tree);
+ while (!btrfs_get_free_objectid(fs_info->tree_root, &objid) && objid <= qgroupid);

return qgroup;
}
--
2.25.1

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:
BUG: sleeping function called from invalid context in btrfs_get_free_objectid

BUG: sleeping function called from invalid context at kernel/locking/mutex.c:580
in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 5526, name: syz-executor.0
preempt_count: 1, expected: 0
RCU nest depth: 0, expected: 0
10 locks held by syz-executor.0/5526:
#0: ffff8880751f4418 (sb_writers#14){.+.+}-{0:0}, at: mnt_want_write_file+0x61/0x200 fs/namespace.c:448
#1: ffff888073da1818 (&type->i_mutex_dir_key#8/1){+.+.}-{3:3}, at: btrfs_mksubvol+0x1c9/0x750 fs/btrfs/ioctl.c:967
#2: ffff888020974be8 (&fs_info->subvol_sem){++++}-{3:3}, at: btrfs_mksubvol+0x52a/0x750 fs/btrfs/ioctl.c:989
#3: ffff8880751f4608 (sb_internal#2){.+.+}-{0:0}, at: create_snapshot+0x437/0x7e0 fs/btrfs/ioctl.c:837
#4: ffff888020976458 (btrfs_trans_completed){.+.+}-{0:0}, at: btrfs_commit_transaction+0x17b/0x3730 fs/btrfs/transaction.c:2214
#5: ffff888020976430 (btrfs_trans_super_committed){.+.+}-{0:0}, at: btrfs_commit_transaction+0x17b/0x3730 fs/btrfs/transaction.c:2214
#6: ffff888020976408 (btrfs_trans_unblocked){++++}-{0:0}, at: btrfs_commit_transaction+0x17b/0x3730 fs/btrfs/transaction.c:2214
#7: ffff888020974cb8 (&fs_info->reloc_mutex){+.+.}-{3:3}, at: btrfs_commit_transaction+0xf14/0x3730 fs/btrfs/transaction.c:2433
#8: ffff888020975818 (&fs_info->qgroup_ioctl_lock){+.+.}-{3:3}, at: btrfs_create_qgroup+0xa2/0x280 fs/btrfs/qgroup.c:1707
#9: ffff888020975780 (&fs_info->qgroup_lock){+.+.}-{2:2}, at: spin_lock include/linux/spinlock.h:351 [inline]
#9: ffff888020975780 (&fs_info->qgroup_lock){+.+.}-{2:2}, at: btrfs_create_qgroup+0x226/0x280 fs/btrfs/qgroup.c:1729
Preemption disabled at:
[<0000000000000000>] 0x0
CPU: 1 PID: 5526 Comm: syz-executor.0 Not tainted 6.6.0-syzkaller-15365-g305230142ae0-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x1e7/0x2d0 lib/dump_stack.c:106
__might_resched+0x5cf/0x780 kernel/sched/core.c:10151
__mutex_lock_common kernel/locking/mutex.c:580 [inline]
__mutex_lock+0xc1/0xd60 kernel/locking/mutex.c:747
btrfs_get_free_objectid+0x34/0x180 fs/btrfs/disk-io.c:4924
add_qgroup_rb+0x3a0/0x440 fs/btrfs/qgroup.c:236
btrfs_create_qgroup+0x234/0x280 fs/btrfs/qgroup.c:1730
create_pending_snapshot+0x8cc/0x2b70 fs/btrfs/transaction.c:1776
create_pending_snapshots+0x195/0x1d0 fs/btrfs/transaction.c:1967
btrfs_commit_transaction+0xf1c/0x3730 fs/btrfs/transaction.c:2440
create_snapshot+0x4a5/0x7e0 fs/btrfs/ioctl.c:845
btrfs_mksubvol+0x5d0/0x750 fs/btrfs/ioctl.c:995
btrfs_mksnapshot+0xb5/0xf0 fs/btrfs/ioctl.c:1041
__btrfs_ioctl_snap_create+0x344/0x460 fs/btrfs/ioctl.c:1294
btrfs_ioctl_snap_create+0x13c/0x190 fs/btrfs/ioctl.c:1321
btrfs_ioctl+0xbbf/0xd40
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:871 [inline]
__se_sys_ioctl+0xf8/0x170 fs/ioctl.c:857
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x7f6d5147cae9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 20 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 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f6d5220d0c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f6d5159c050 RCX: 00007f6d5147cae9
RDX: 0000000020000a80 RSI: 0000000050009401 RDI: 0000000000000004
RBP: 00007f6d514c847a R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 000000000000006e R14: 00007f6d5159c050 R15: 00007ffcea145098
</TASK>

=============================
[ BUG: Invalid wait context ]
6.6.0-syzkaller-15365-g305230142ae0-dirty #0 Tainted: G W
-----------------------------
syz-executor.0/5526 is trying to lock:
ffff8880277522d8 (&root->objectid_mutex){+.+.}-{3:3}, at: btrfs_get_free_objectid+0x34/0x180 fs/btrfs/disk-io.c:4924
other info that might help us debug this:
context-{4:4}
10 locks held by syz-executor.0/5526:
#0: ffff8880751f4418 (sb_writers#14){.+.+}-{0:0}, at: mnt_want_write_file+0x61/0x200 fs/namespace.c:448
#1: ffff888073da1818 (&type->i_mutex_dir_key#8/1){+.+.}-{3:3}, at: btrfs_mksubvol+0x1c9/0x750 fs/btrfs/ioctl.c:967
#2: ffff888020974be8 (&fs_info->subvol_sem){++++}-{3:3}, at: btrfs_mksubvol+0x52a/0x750 fs/btrfs/ioctl.c:989
#3: ffff8880751f4608 (sb_internal#2){.+.+}-{0:0}, at: create_snapshot+0x437/0x7e0 fs/btrfs/ioctl.c:837
#4: ffff888020976458 (btrfs_trans_completed){.+.+}-{0:0}, at: btrfs_commit_transaction+0x17b/0x3730 fs/btrfs/transaction.c:2214
#5: ffff888020976430 (btrfs_trans_super_committed){.+.+}-{0:0}, at: btrfs_commit_transaction+0x17b/0x3730 fs/btrfs/transaction.c:2214
#6: ffff888020976408 (btrfs_trans_unblocked){++++}-{0:0}, at: btrfs_commit_transaction+0x17b/0x3730 fs/btrfs/transaction.c:2214
#7: ffff888020974cb8 (&fs_info->reloc_mutex){+.+.}-{3:3}, at: btrfs_commit_transaction+0xf14/0x3730 fs/btrfs/transaction.c:2433
#8: ffff888020975818 (&fs_info->qgroup_ioctl_lock){+.+.}-{3:3}, at: btrfs_create_qgroup+0xa2/0x280 fs/btrfs/qgroup.c:1707
#9: ffff888020975780 (&fs_info->qgroup_lock){+.+.}-{2:2}, at: spin_lock include/linux/spinlock.h:351 [inline]
#9: ffff888020975780 (&fs_info->qgroup_lock){+.+.}-{2:2}, at: btrfs_create_qgroup+0x226/0x280 fs/btrfs/qgroup.c:1729
stack backtrace:
CPU: 1 PID: 5526 Comm: syz-executor.0 Tainted: G W 6.6.0-syzkaller-15365-g305230142ae0-dirty #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
Call Trace:
<TASK>
__dump_stack lib/dump_stack.c:88 [inline]
dump_stack_lvl+0x1e7/0x2d0 lib/dump_stack.c:106
print_lock_invalid_wait_context kernel/locking/lockdep.c:4750 [inline]
check_wait_context kernel/locking/lockdep.c:4820 [inline]
__lock_acquire+0x1825/0x7f70 kernel/locking/lockdep.c:5086
lock_acquire+0x1e3/0x520 kernel/locking/lockdep.c:5753
__mutex_lock_common kernel/locking/mutex.c:603 [inline]
__mutex_lock+0x136/0xd60 kernel/locking/mutex.c:747
btrfs_get_free_objectid+0x34/0x180 fs/btrfs/disk-io.c:4924
add_qgroup_rb+0x3a0/0x440 fs/btrfs/qgroup.c:236
btrfs_create_qgroup+0x234/0x280 fs/btrfs/qgroup.c:1730
create_pending_snapshot+0x8cc/0x2b70 fs/btrfs/transaction.c:1776
create_pending_snapshots+0x195/0x1d0 fs/btrfs/transaction.c:1967
btrfs_commit_transaction+0xf1c/0x3730 fs/btrfs/transaction.c:2440
create_snapshot+0x4a5/0x7e0 fs/btrfs/ioctl.c:845
btrfs_mksubvol+0x5d0/0x750 fs/btrfs/ioctl.c:995
btrfs_mksnapshot+0xb5/0xf0 fs/btrfs/ioctl.c:1041
__btrfs_ioctl_snap_create+0x344/0x460 fs/btrfs/ioctl.c:1294
btrfs_ioctl_snap_create+0x13c/0x190 fs/btrfs/ioctl.c:1321
btrfs_ioctl+0xbbf/0xd40
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:871 [inline]
__se_sys_ioctl+0xf8/0x170 fs/ioctl.c:857
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b
RIP: 0033:0x7f6d5147cae9
Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 20 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 b0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f6d5220d0c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f6d5159c050 RCX: 00007f6d5147cae9
RDX: 0000000020000a80 RSI: 0000000050009401 RDI: 0000000000000004
RBP: 00007f6d514c847a R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 000000000000006e R14: 00007f6d5159c050 R15: 00007ffcea145098
</TASK>

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

console output: https://syzkaller.appspot.com/x/log.txt?x=12c3dc70e80000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=122f123f680000

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c

index edb84cc03237..9be5a836c9c0 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -218,6 +218,7 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,
p = &(*p)->rb_right;
} else {
kfree(prealloc);
+ prealloc = NULL;
return qgroup;
}
}
@@ -1697,6 +1698,7 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
struct btrfs_root *quota_root;
struct btrfs_qgroup *qgroup;
struct btrfs_qgroup *prealloc = NULL;
+ u64 objid;
int ret = 0;

if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED)
@@ -1727,6 +1729,8 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
spin_lock(&fs_info->qgroup_lock);
qgroup = add_qgroup_rb(fs_info, prealloc, qgroupid);
spin_unlock(&fs_info->qgroup_lock);
+ while (!prealloc && !btrfs_get_free_objectid(fs_info->tree_root,
+ &objid) && objid <= qgroupid);
prealloc = NULL;

ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);
--
2.25.1

[Edward Adam Davis]

#syz test https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 305230142ae0

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index edb84cc03237..9be5a836c9c0 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -218,6 +218,7 @@ static struct btrfs_qgroup *add_qgroup_rb(struct btrfs_fs_info *fs_info,
p = &(*p)->rb_right;
} else {
kfree(prealloc);
+ prealloc = NULL;
return qgroup;
}
}
@@ -1697,6 +1698,7 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
struct btrfs_root *quota_root;
struct btrfs_qgroup *qgroup;
struct btrfs_qgroup *prealloc = NULL;
+ u64 objid;
int ret = 0;

if (btrfs_qgroup_mode(fs_info) == BTRFS_QGROUP_MODE_DISABLED)
@@ -1727,6 +1729,8 @@ int btrfs_create_qgroup(struct btrfs_trans_handle *trans, u64 qgroupid)
spin_lock(&fs_info->qgroup_lock);
qgroup = add_qgroup_rb(fs_info, prealloc, qgroupid);
spin_unlock(&fs_info->qgroup_lock);

+ while (prealloc && !btrfs_get_free_objectid(fs_info->tree_root,

[syzbot]

Hello,

syzbot has tested the proposed patch but the reproducer is still triggering an issue:

WARNING in create_pending_snapshot

BTRFS info (device loop0): checking UUID tree
------------[ cut here ]------------
BTRFS: Transaction aborted (error -17)
WARNING: CPU: 0 PID: 5480 at fs/btrfs/transaction.c:1778 create_pending_snapshot+0x25f4/0x2b70 fs/btrfs/transaction.c:1778
Modules linked in:
CPU: 0 PID: 5480 Comm: syz-executor.0 Not tainted 6.6.0-syzkaller-15365-g305230142ae0-dirty #0

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023

RIP: 0010:create_pending_snapshot+0x25f4/0x2b70 fs/btrfs/transaction.c:1778
Code: f8 fd 48 c7 c7 00 43 ab 8b 89 de e8 76 4b be fd 0f 0b e9 30 f3 ff ff e8 7a 8d f8 fd 48 c7 c7 00 43 ab 8b 89 de e8 5c 4b be fd <0f> 0b e9 f8 f6 ff ff e8 60 8d f8 fd 48 c7 c7 00 43 ab 8b 89 de e8
RSP: 0018:ffffc900094cf580 EFLAGS: 00010246
RAX: ca9849a0d3eb4500 RBX: 00000000ffffffef RCX: ffff88807ac39dc0
RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000000
RBP: ffffc900094cf870 R08: ffffffff81547c82 R09: 1ffff92001299e04
R10: dffffc0000000000 R11: fffff52001299e05 R12: ffff88807ca30500
R13: 00000000ffffffef R14: 0000000000000000 R15: ffff88807ca304a0
FS: 00007f9af6b8a6c0(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f15547ad018 CR3: 0000000028d0c000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>

create_pending_snapshots+0x195/0x1d0 fs/btrfs/transaction.c:1967
btrfs_commit_transaction+0xf1c/0x3730 fs/btrfs/transaction.c:2440
create_snapshot+0x4a5/0x7e0 fs/btrfs/ioctl.c:845
btrfs_mksubvol+0x5d0/0x750 fs/btrfs/ioctl.c:995
btrfs_mksnapshot+0xb5/0xf0 fs/btrfs/ioctl.c:1041
__btrfs_ioctl_snap_create+0x344/0x460 fs/btrfs/ioctl.c:1294
btrfs_ioctl_snap_create+0x13c/0x190 fs/btrfs/ioctl.c:1321
btrfs_ioctl+0xbbf/0xd40
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:871 [inline]
__se_sys_ioctl+0xf8/0x170 fs/ioctl.c:857
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b

RIP: 0033:0x7f9af5e7cae9

Code: 28 00 00 00 75 05 48 83 c4 28 c3 e8 e1 20 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 b0 ff ff ff f7 d8 64 89 01 48

RSP: 002b:00007f9af6b8a0c8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00007f9af5f9bf80 RCX: 00007f9af5e7cae9

RDX: 0000000020000a80 RSI: 0000000050009401 RDI: 0000000000000004

RBP: 00007f9af5ec847a R08: 0000000000000000 R09: 0000000000000000

R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000

R13: 000000000000000b R14: 00007f9af5f9bf80 R15: 00007ffcf277e6d8

</TASK>


Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

console output: https://syzkaller.appspot.com/x/log.txt?x=112cafc4e80000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=15c3dc70e80000

[syzbot]

Hello,

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

Reported-and-tested-by: syzbot+4d8101...@syzkaller.appspotmail.com

Tested on:

commit: 30523014 Merge tag 'pm-6.7-rc1-2' of git://git.kernel...
git tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

console output: https://syzkaller.appspot.com/x/log.txt?x=17aced04e80000

kernel config: https://syzkaller.appspot.com/x/.config?x=beb32a598fd79db9
dashboard link: https://syzkaller.appspot.com/bug?extid=4d81015bc10889fd12ea
compiler: Debian clang version 15.0.6, GNU ld (GNU Binutils for Debian) 2.40

patch: https://syzkaller.appspot.com/x/patch.diff?x=12185f38e80000

[Edward Adam Davis]

[syz logs]
1.syz reported:
open("./file0", O_RDONLY) = 4
ioctl(4, BTRFS_IOC_QUOTA_CTL, {cmd=BTRFS_QUOTA_CTL_ENABLE}) = 0
openat(AT_FDCWD, "blkio.bfq.time_recursive", O_RDWR|O_CREAT|O_NOCTTY|O_TRUNC|O_APPEND|FASYNC|0x18, 000) = 5
ioctl(5, BTRFS_IOC_QGROUP_CREATE, {create=1, qgroupid=256}) = 0
openat(AT_FDCWD, ".", O_RDONLY) = 6

------------[ cut here ]------------
BTRFS: Transaction aborted (error -17)

WARNING: CPU: 0 PID: 5057 at fs/btrfs/transaction.c:1778 create_pending_snapshot+0x25f4/0x2b70 fs/btrfs/transaction.c:1778
Modules linked in:
CPU: 0 PID: 5057 Comm: syz-executor225 Not tainted 6.6.0-syzkaller-15365-g305230142ae0 #0

Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 10/09/2023
RIP: 0010:create_pending_snapshot+0x25f4/0x2b70 fs/btrfs/transaction.c:1778
Code: f8 fd 48 c7 c7 00 43 ab 8b 89 de e8 76 4b be fd 0f 0b e9 30 f3 ff ff e8 7a 8d f8 fd 48 c7 c7 00 43 ab 8b 89 de e8 5c 4b be fd <0f> 0b e9 f8 f6 ff ff e8 60 8d f8 fd 48 c7 c7 00 43 ab 8b 89 de e8

RSP: 0018:ffffc90003abf580 EFLAGS: 00010246
RAX: 10fb7cf24e10ea00 RBX: 00000000ffffffef RCX: ffff888023ea9dc0

RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000000

RBP: ffffc90003abf870 R08: ffffffff81547c82 R09: 1ffff11017305172
R10: dffffc0000000000 R11: ffffed1017305173 R12: ffff888078ae2878
R13: 00000000ffffffef R14: 0000000000000000 R15: ffff888078ae2818
FS: 000055555667d380(0000) GS:ffff8880b9800000(0000) knlGS:0000000000000000

CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033

CR2: 00007f6ff7bf2304 CR3: 0000000079f17000 CR4: 00000000003506f0

DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
create_pending_snapshots+0x195/0x1d0 fs/btrfs/transaction.c:1967
btrfs_commit_transaction+0xf1c/0x3730 fs/btrfs/transaction.c:2440
create_snapshot+0x4a5/0x7e0 fs/btrfs/ioctl.c:845
btrfs_mksubvol+0x5d0/0x750 fs/btrfs/ioctl.c:995
btrfs_mksnapshot+0xb5/0xf0 fs/btrfs/ioctl.c:1041
__btrfs_ioctl_snap_create+0x344/0x460 fs/btrfs/ioctl.c:1294
btrfs_ioctl_snap_create+0x13c/0x190 fs/btrfs/ioctl.c:1321
btrfs_ioctl+0xbbf/0xd40
vfs_ioctl fs/ioctl.c:51 [inline]
__do_sys_ioctl fs/ioctl.c:871 [inline]
__se_sys_ioctl+0xf8/0x170 fs/ioctl.c:857
do_syscall_x64 arch/x86/entry/common.c:51 [inline]
do_syscall_64+0x44/0x110 arch/x86/entry/common.c:82
entry_SYSCALL_64_after_hwframe+0x63/0x6b

2. syz repro:
r0 = open(&(0x7f0000000080)='./file0\x00', 0x0, 0x0)
ioctl$BTRFS_IOC_QUOTA_CTL(r0, 0xc0109428, &(0x7f0000000000)={0x1})
r1 = openat$cgroup_ro(0xffffffffffffff9c, &(0x7f0000000100)='blkio.bfq.time_recursive\x00', 0x275a, 0x0)
ioctl$BTRFS_IOC_QGROUP_CREATE(r1, 0x4010942a, &(0x7f0000000640)={0x1, 0x100})
r2 = openat(0xffffffffffffff9c, &(0x7f0000000500)='.\x00', 0x0, 0x0)
ioctl$BTRFS_IOC_SNAP_CREATE(r0, 0x50009401, &(0x7f0000000a80)={{r2},

[Analysis]
1. ioctl$BTRFS_IOC_QGROUP_CREATE(r1, 0x4010942a, &(0x7f0000000640)={0x1, 0x100})
After executing create qgroup, a qgroup of "qgroupid=256" will be created,
which corresponds to the file "blkio.bfq.time_recursive".

2. ioctl$BTRFS_IOC_SNAP_CREATE(r0, 0x50009401, &(0x7f0000000a80)={{r2},
Create snap is to create a subvolume for the file0.

Therefore, the qgroup created for the file 'blkio.bfq.time_recursive' cannot
be used for file0.

[Fix]
After added new qgroup to qgroup tree, we need to sync free_objectid use
the qgroupid, avoiding subvolume creation failure.

Reported-and-tested-by: syzbot+4d8101...@syzkaller.appspotmail.com
Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
Signed-off-by: Edward Adam Davis <ead...@qq.com>
---

fs/btrfs/qgroup.c | 4 ++++
1 file changed, 4 insertions(+)

[Qu Wenruo]

What? That sentence makes no sense.

It seems you didn't even understand qgroup is for subvolume, not for
'file0'.
Btrfs just uses that fd to locate a btrfs, not to do operation on that file.

Thus your analyze still makes no sense, even you have already reached
the core problem, we have a qgroup created before a subvolume with the
same id to be created.

I have replied several times on this, if you didn't receive it, then let
me make it clear AGAIN:

This is the wrong way to fix it.

When creating a qgroup, the qgroupid is either specified by the end user
or by the subvolume to be created.

In that case, it's the create_pending_snapshot() trying to create a
qgroup, which has the same id already created by previous ioctl:

ioctl(5, BTRFS_IOC_QGROUP_CREATE, {create=1, qgroupid=256}) = 0

Now due to the new timing where try to create a new qgroup when creating
a subvolume, we found there is an existing one, and return -EEXIST.

The new call site just treat this as an critical error, and abort the
transaction.

The proper fix is to handle -EEXIST and continue, no need to abort
transaction as it's not a critical error.

See the proper fix here:
https://lore.kernel.org/linux-btrfs/b305a5b0228b40fc62923b0133957...@suse.com/T/#u