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

From: Qu Wenruo
Date: Sun Nov 12 2023 - 02:37:19 EST



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@xxxxxxxxxxxxxxxxxxxxxxxxx
Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
Signed-off-by: Edward Adam Davis <eadavis@xxxxxx>
---
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@xxxxxxxx/T/#u

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