[syzbot] [btrfs?] WARNING in create_pending_snapshot

[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 <boris@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+4d81015bc10889fd12ea@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 syzkaller@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

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: lizhi.xu@windriver.com

#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);

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: lizhi.xu@windriver.com

#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);

[PATCH] btrfs: fix warning in create_pending_snapshot

[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+4d81015bc10889fd12ea@syzkaller.appspotmail.com
Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
Signed-off-by: Lizhi Xu <lizhi.xu@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

Re: [PATCH] btrfs: fix warning in create_pending_snapshot

[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
>
> Reported-and-tested-by: syzbot+4d81015bc10889fd12ea@syzkaller.appspotmail.com
> Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
> Signed-off-by: Lizhi Xu <lizhi.xu@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);

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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);

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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);
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

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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);
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

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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);
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

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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)
+{
+	return find_qgroup_rb(fs_info, qgroupid);
+}
 #endif
-- 
2.25.1

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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,
+                                            u64 qgroupid);
+
+static bool exist_qgroup_rb(struct btrfs_fs_info *fs_info, u64 qgroupid)
+{
+	return find_qgroup_rb(fs_info, qgroupid);
+}
 #endif
-- 
2.25.1

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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

[PATCH] btrfs: fix warning in create_pending_snapshot

[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+4d81015bc10889fd12ea@syzkaller.appspotmail.com
Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
Signed-off-by: Edward Adam Davis <eadavis@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++));
+	*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

Re: [PATCH] btrfs: fix warning in create_pending_snapshot

[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)

Re: [PATCH] btrfs: fix warning in create_pending_snapshot

[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+4d81015bc10889fd12ea@syzkaller.appspotmail.com
> Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
> Signed-off-by: Edward Adam Davis <eadavis@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

> +	*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

[PATCH] test 305230142ae0

[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

Re: [PATCH] test 305230142ae0

[Qu Wenruo]

On 2023/11/11 18:43, Edward Adam Davis wrote:
> 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.

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

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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

Re: [syzbot] [PATCH] test 305230142ae0

[syzbot]

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

***

Subject: [PATCH] test 305230142ae0
Author: eadavis@qq.com

#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

[PATCH V2] btrfs: fix warning in create_pending_snapshot

[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+4d81015bc10889fd12ea@syzkaller.appspotmail.com
Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/btrfs/qgroup.c | 4 ++++
 1 file changed, 4 insertions(+)

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

Re: [PATCH V2] btrfs: fix warning in create_pending_snapshot

[Qu Wenruo]

On 2023/11/12 15:18, Edward Adam Davis wrote:
> [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.

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.

> 
> [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+4d81015bc10889fd12ea@syzkaller.appspotmail.com
> Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>   fs/btrfs/qgroup.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> 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);

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/b305a5b0228b40fc62923b0133957c72468600de.1699649085.git.wqu@suse.com/T/#u

>   	prealloc = NULL;
>   
>   	ret = btrfs_sysfs_add_one_qgroup(fs_info, qgroup);