| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4e5ec572-9489-4c4e-a2c6-1ed497fe3491@suse.com>
Date: Sun, 12 Nov 2023 18:05:55 +1030
From: Qu Wenruo <wqu@...e.com>
To: Edward Adam Davis <eadavis@...com>,
syzbot+4d81015bc10889fd12ea@...kaller.appspotmail.com
Cc: boris@....io, clm@...com, dsterba@...e.com, josef@...icpanda.com,
linux-btrfs@...r.kernel.org, linux-fsdevel@...r.kernel.org,
linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH V2] btrfs: fix warning in create_pending_snapshot
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@...kaller.appspotmail.com
> Fixes: 6ed05643ddb1 ("btrfs: create qgroup earlier in snapshot creation")
> Signed-off-by: Edward Adam Davis <eadavis@...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);
Powered by blists - more mailing lists