| Message ID | tencent_B9A7767566563D4D376C96BE36524B147409@qq.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] btrfs: fix warning in create_pending_snapshot | expand |
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);
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);
[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(+)