lkml.org 
[lkml]   [2023]   [Nov]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH V2] btrfs: fix warning in create_pending_snapshot
From


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

\
 
 \ /
  Last update: 2023-11-20 13:57    [W:0.160 / U:0.204 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site