Syzkaller reported UAF as follows:
==================================================================
BUG: KASAN: use-after-free in btrfs_get_global_root+0x663/0xa10
Read of size 4 at addr ffff88811ddbb3c0 by task kworker/u16:1/11
CPU: 4 PID: 11 Comm: kworker/u16:1 Not tainted 6.0.0-rc1-next-20220822+ #2
Workqueue: btrfs-qgroup-rescan btrfs_work_helper
Call Trace:
<TASK>
dump_stack_lvl+0x6e/0x91
print_report.cold+0xb2/0x6bb
kasan_report+0xa8/0x130
kasan_check_range+0x13f/0x1d0
btrfs_get_global_root+0x663/0xa10
btrfs_get_fs_root_commit_root+0xa5/0x150
find_parent_nodes+0x92f/0x2990
btrfs_find_all_roots_safe+0x12d/0x220
btrfs_find_all_roots+0xbb/0xd0
btrfs_qgroup_rescan_worker+0x600/0xc30
btrfs_work_helper+0xff/0x750
process_one_work+0x52c/0x930
worker_thread+0x352/0x8c0
kthread+0x1b9/0x200
ret_from_fork+0x22/0x30
</TASK>
Allocated by task 1895:
kasan_save_stack+0x1e/0x40
__kasan_kmalloc+0xa9/0xe0
btrfs_alloc_root+0x40/0x820
btrfs_create_tree+0xf8/0x500
btrfs_quota_enable+0x30a/0x1120
btrfs_ioctl+0x50a3/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
Freed by task 1895:
kasan_save_stack+0x1e/0x40
kasan_set_track+0x21/0x30
kasan_set_free_info+0x20/0x40
__kasan_slab_free+0x127/0x1c0
kfree+0xa8/0x2d0
btrfs_put_root+0x1ca/0x230
btrfs_quota_enable+0x87c/0x1120
btrfs_ioctl+0x50a3/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
==================================================================
Above issue may happens as follows:
p1 p2
btrfs_quota_enable
spin_lock(&fs_info->qgroup_lock);
fs_info->quota_root = quota_root;
spin_unlock(&fs_info->qgroup_lock);
ret = qgroup_rescan_init -> return error
if (ret)
btrfs_put_root(quota_root);
kfree(root);
if (ret) {
ulist_free(fs_info->qgroup_ulist);
fs_info->qgroup_ulist = NULL;
btrfs_sysfs_del_qgroups(fs_info);
} btrfs_qgroup_rescan_worker
btrfs_find_all_roots
btrfs_find_all_roots_safe
find_parent_nodes
btrfs_get_fs_root_commit_root
btrfs_grab_root(fs_info->quota_root)
-> quota_root already freed
Syzkaller also reported another issue:
==================================================================
BUG: KASAN: use-after-free in ulist_release+0x30/0xb3
Read of size 8 at addr ffff88811413d048 by task rep/2921
CPU: 3 PID: 2921 Comm: rep Not tainted 6.0.0-rc1-next-20220822+ #3
rep[2921] cmdline: ./rep
Call Trace:
<TASK>
dump_stack_lvl+0x6e/0x91
print_report.cold+0xb2/0x6bb
kasan_report+0xa8/0x130
ulist_release+0x30/0xb3
ulist_reinit+0x16/0x56
btrfs_qgroup_free_refroot+0x288/0x3f0
btrfs_qgroup_free_meta_all_pertrans+0xed/0x1e0
commit_fs_roots+0x28c/0x430
btrfs_commit_transaction+0x9a6/0x1b40
btrfs_qgroup_rescan+0x7e/0x130
btrfs_ioctl+0x48ed/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
</TASK>
Allocated by task 2900:
kasan_save_stack+0x1e/0x40
__kasan_kmalloc+0xa9/0xe0
ulist_alloc+0x5c/0xe0
btrfs_quota_enable+0x1b2/0x1160
btrfs_ioctl+0x50a3/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
Freed by task 2900:
kasan_save_stack+0x1e/0x40
kasan_set_track+0x21/0x30
kasan_set_free_info+0x20/0x40
__kasan_slab_free+0x127/0x1c0
kfree+0xa8/0x2d0
ulist_free.cold+0x15/0x1a
btrfs_quota_enable+0x8bf/0x1160
btrfs_ioctl+0x50a3/0x59f0
__x64_sys_ioctl+0x130/0x170
do_syscall_64+0x3b/0x90
entry_SYSCALL_64_after_hwframe+0x63/0xcd
==================================================================
To solve above issues just set 'fs_info->quota_root' after qgroup_rescan_init
return success.
Signed-off-by: Ye Bin <[email protected]>
---
fs/btrfs/qgroup.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index db723c0026bd..16f0b038295a 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1158,18 +1158,18 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
if (ret)
goto out_free_path;
- /*
- * Set quota enabled flag after committing the transaction, to avoid
- * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
- * creation.
- */
- spin_lock(&fs_info->qgroup_lock);
- fs_info->quota_root = quota_root;
- set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
- spin_unlock(&fs_info->qgroup_lock);
-
ret = qgroup_rescan_init(fs_info, 0, 1);
if (!ret) {
+ /*
+ * Set quota enabled flag after committing the transaction, to
+ * avoid deadlocks on fs_info->qgroup_ioctl_lock with concurrent
+ * snapshot creation.
+ */
+ spin_lock(&fs_info->qgroup_lock);
+ fs_info->quota_root = quota_root;
+ set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
+ spin_unlock(&fs_info->qgroup_lock);
+
qgroup_rescan_zero_tracking(fs_info);
fs_info->qgroup_rescan_running = true;
btrfs_queue_work(fs_info->qgroup_rescan_workers,
--
2.31.1
On Tue, Aug 23, 2022 at 09:59:31AM +0800, Ye Bin wrote:
> Syzkaller reported UAF as follows:
> ==================================================================
> BUG: KASAN: use-after-free in btrfs_get_global_root+0x663/0xa10
> Read of size 4 at addr ffff88811ddbb3c0 by task kworker/u16:1/11
>
> CPU: 4 PID: 11 Comm: kworker/u16:1 Not tainted 6.0.0-rc1-next-20220822+ #2
> Workqueue: btrfs-qgroup-rescan btrfs_work_helper
> Call Trace:
> <TASK>
> dump_stack_lvl+0x6e/0x91
> print_report.cold+0xb2/0x6bb
> kasan_report+0xa8/0x130
> kasan_check_range+0x13f/0x1d0
> btrfs_get_global_root+0x663/0xa10
> btrfs_get_fs_root_commit_root+0xa5/0x150
> find_parent_nodes+0x92f/0x2990
> btrfs_find_all_roots_safe+0x12d/0x220
> btrfs_find_all_roots+0xbb/0xd0
> btrfs_qgroup_rescan_worker+0x600/0xc30
> btrfs_work_helper+0xff/0x750
> process_one_work+0x52c/0x930
> worker_thread+0x352/0x8c0
> kthread+0x1b9/0x200
> ret_from_fork+0x22/0x30
> </TASK>
>
> Allocated by task 1895:
> kasan_save_stack+0x1e/0x40
> __kasan_kmalloc+0xa9/0xe0
> btrfs_alloc_root+0x40/0x820
> btrfs_create_tree+0xf8/0x500
> btrfs_quota_enable+0x30a/0x1120
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Freed by task 1895:
> kasan_save_stack+0x1e/0x40
> kasan_set_track+0x21/0x30
> kasan_set_free_info+0x20/0x40
> __kasan_slab_free+0x127/0x1c0
> kfree+0xa8/0x2d0
> btrfs_put_root+0x1ca/0x230
> btrfs_quota_enable+0x87c/0x1120
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> ==================================================================
>
> Above issue may happens as follows:
> p1 p2
> btrfs_quota_enable
> spin_lock(&fs_info->qgroup_lock);
> fs_info->quota_root = quota_root;
> spin_unlock(&fs_info->qgroup_lock);
>
> ret = qgroup_rescan_init -> return error
> if (ret)
> btrfs_put_root(quota_root);
> kfree(root);
>
> if (ret) {
> ulist_free(fs_info->qgroup_ulist);
> fs_info->qgroup_ulist = NULL;
> btrfs_sysfs_del_qgroups(fs_info);
> } btrfs_qgroup_rescan_worker
> btrfs_find_all_roots
> btrfs_find_all_roots_safe
> find_parent_nodes
> btrfs_get_fs_root_commit_root
> btrfs_grab_root(fs_info->quota_root)
> -> quota_root already freed
>
> Syzkaller also reported another issue:
> ==================================================================
> BUG: KASAN: use-after-free in ulist_release+0x30/0xb3
> Read of size 8 at addr ffff88811413d048 by task rep/2921
>
> CPU: 3 PID: 2921 Comm: rep Not tainted 6.0.0-rc1-next-20220822+ #3
> rep[2921] cmdline: ./rep
> Call Trace:
> <TASK>
> dump_stack_lvl+0x6e/0x91
> print_report.cold+0xb2/0x6bb
> kasan_report+0xa8/0x130
> ulist_release+0x30/0xb3
> ulist_reinit+0x16/0x56
> btrfs_qgroup_free_refroot+0x288/0x3f0
> btrfs_qgroup_free_meta_all_pertrans+0xed/0x1e0
> commit_fs_roots+0x28c/0x430
> btrfs_commit_transaction+0x9a6/0x1b40
> btrfs_qgroup_rescan+0x7e/0x130
> btrfs_ioctl+0x48ed/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> </TASK>
>
> Allocated by task 2900:
> kasan_save_stack+0x1e/0x40
> __kasan_kmalloc+0xa9/0xe0
> ulist_alloc+0x5c/0xe0
> btrfs_quota_enable+0x1b2/0x1160
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Freed by task 2900:
> kasan_save_stack+0x1e/0x40
> kasan_set_track+0x21/0x30
> kasan_set_free_info+0x20/0x40
> __kasan_slab_free+0x127/0x1c0
> kfree+0xa8/0x2d0
> ulist_free.cold+0x15/0x1a
> btrfs_quota_enable+0x8bf/0x1160
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> ==================================================================
>
> To solve above issues just set 'fs_info->quota_root' after qgroup_rescan_init
> return success.
>
> Signed-off-by: Ye Bin <[email protected]>
> ---
> fs/btrfs/qgroup.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index db723c0026bd..16f0b038295a 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1158,18 +1158,18 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> if (ret)
> goto out_free_path;
>
> - /*
> - * Set quota enabled flag after committing the transaction, to avoid
> - * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
> - * creation.
> - */
> - spin_lock(&fs_info->qgroup_lock);
> - fs_info->quota_root = quota_root;
> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> - spin_unlock(&fs_info->qgroup_lock);
> -
> ret = qgroup_rescan_init(fs_info, 0, 1);
> if (!ret) {
> + /*
> + * Set quota enabled flag after committing the transaction, to
> + * avoid deadlocks on fs_info->qgroup_ioctl_lock with concurrent
> + * snapshot creation.
> + */
> + spin_lock(&fs_info->qgroup_lock);
> + fs_info->quota_root = quota_root;
> + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> + spin_unlock(&fs_info->qgroup_lock);
> +
But how can the race happen? The changelog should explain that.
To me this suggests that after we set BTRFS_FS_QUOTA_ENABLED and set the
quota root, but before we called qgroup_rescan_init() at btrfs_quota_enable(),
some other task started the rescan worker first - I can only think of
someone else calling the ioctl to start the rescan worker (btrfs_ioctl_quota_rescan()).
In that case we get "ret == -EINPROGRESS" at btrfs_quota_enable().
So please provide a detailed explanation in the log of how the race can
happen.
This solution is also buggy. Because in case of an error, we will leave the
quota tree created, the qgroup relation, etc. That is, we don't undo
what btrfs_create_tree(), add_qgroup_item(), add_qgroup_rb(), etc did
Which means a future btrfs_quota_enable() call would fail, and calling
btrfs_quota_disable() to undo all those things will not work either,
because fs_info->quota_root is NULL.
I would suggest ignoring the error of qgroup_rescan_init() if it's
-EINPROGRESS, and ASSERT if it's anything different from 0 or
-EINPROGRESS. Also add a comment mentioning we can get -EINPROGRESS
because someone may have called the qgroup rescan ioctl.
Thanks.
> qgroup_rescan_zero_tracking(fs_info);
> fs_info->qgroup_rescan_running = true;
> btrfs_queue_work(fs_info->qgroup_rescan_workers,
> --
> 2.31.1
>
On Tue, Aug 23, 2022 at 09:59:31AM +0800, Ye Bin wrote:
> Syzkaller reported UAF as follows:
> ==================================================================
> BUG: KASAN: use-after-free in btrfs_get_global_root+0x663/0xa10
> Read of size 4 at addr ffff88811ddbb3c0 by task kworker/u16:1/11
>
> CPU: 4 PID: 11 Comm: kworker/u16:1 Not tainted 6.0.0-rc1-next-20220822+ #2
> Workqueue: btrfs-qgroup-rescan btrfs_work_helper
> Call Trace:
> <TASK>
> dump_stack_lvl+0x6e/0x91
> print_report.cold+0xb2/0x6bb
> kasan_report+0xa8/0x130
> kasan_check_range+0x13f/0x1d0
> btrfs_get_global_root+0x663/0xa10
> btrfs_get_fs_root_commit_root+0xa5/0x150
> find_parent_nodes+0x92f/0x2990
> btrfs_find_all_roots_safe+0x12d/0x220
> btrfs_find_all_roots+0xbb/0xd0
> btrfs_qgroup_rescan_worker+0x600/0xc30
> btrfs_work_helper+0xff/0x750
> process_one_work+0x52c/0x930
> worker_thread+0x352/0x8c0
> kthread+0x1b9/0x200
> ret_from_fork+0x22/0x30
> </TASK>
>
> Allocated by task 1895:
> kasan_save_stack+0x1e/0x40
> __kasan_kmalloc+0xa9/0xe0
> btrfs_alloc_root+0x40/0x820
> btrfs_create_tree+0xf8/0x500
> btrfs_quota_enable+0x30a/0x1120
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Freed by task 1895:
> kasan_save_stack+0x1e/0x40
> kasan_set_track+0x21/0x30
> kasan_set_free_info+0x20/0x40
> __kasan_slab_free+0x127/0x1c0
> kfree+0xa8/0x2d0
> btrfs_put_root+0x1ca/0x230
> btrfs_quota_enable+0x87c/0x1120
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> ==================================================================
>
> Above issue may happens as follows:
> p1 p2
> btrfs_quota_enable
> spin_lock(&fs_info->qgroup_lock);
> fs_info->quota_root = quota_root;
> spin_unlock(&fs_info->qgroup_lock);
>
> ret = qgroup_rescan_init -> return error
> if (ret)
> btrfs_put_root(quota_root);
> kfree(root);
>
> if (ret) {
> ulist_free(fs_info->qgroup_ulist);
> fs_info->qgroup_ulist = NULL;
> btrfs_sysfs_del_qgroups(fs_info);
> } btrfs_qgroup_rescan_worker
> btrfs_find_all_roots
> btrfs_find_all_roots_safe
> find_parent_nodes
> btrfs_get_fs_root_commit_root
> btrfs_grab_root(fs_info->quota_root)
> -> quota_root already freed
>
> Syzkaller also reported another issue:
> ==================================================================
> BUG: KASAN: use-after-free in ulist_release+0x30/0xb3
> Read of size 8 at addr ffff88811413d048 by task rep/2921
>
> CPU: 3 PID: 2921 Comm: rep Not tainted 6.0.0-rc1-next-20220822+ #3
> rep[2921] cmdline: ./rep
> Call Trace:
> <TASK>
> dump_stack_lvl+0x6e/0x91
> print_report.cold+0xb2/0x6bb
> kasan_report+0xa8/0x130
> ulist_release+0x30/0xb3
> ulist_reinit+0x16/0x56
> btrfs_qgroup_free_refroot+0x288/0x3f0
> btrfs_qgroup_free_meta_all_pertrans+0xed/0x1e0
> commit_fs_roots+0x28c/0x430
> btrfs_commit_transaction+0x9a6/0x1b40
> btrfs_qgroup_rescan+0x7e/0x130
> btrfs_ioctl+0x48ed/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> </TASK>
>
> Allocated by task 2900:
> kasan_save_stack+0x1e/0x40
> __kasan_kmalloc+0xa9/0xe0
> ulist_alloc+0x5c/0xe0
> btrfs_quota_enable+0x1b2/0x1160
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Freed by task 2900:
> kasan_save_stack+0x1e/0x40
> kasan_set_track+0x21/0x30
> kasan_set_free_info+0x20/0x40
> __kasan_slab_free+0x127/0x1c0
> kfree+0xa8/0x2d0
> ulist_free.cold+0x15/0x1a
> btrfs_quota_enable+0x8bf/0x1160
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> ==================================================================
>
> To solve above issues just set 'fs_info->quota_root' after qgroup_rescan_init
> return success.
>
> Signed-off-by: Ye Bin <[email protected]>
> ---
> fs/btrfs/qgroup.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index db723c0026bd..16f0b038295a 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1158,18 +1158,18 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> if (ret)
> goto out_free_path;
>
> - /*
> - * Set quota enabled flag after committing the transaction, to avoid
> - * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
> - * creation.
> - */
> - spin_lock(&fs_info->qgroup_lock);
> - fs_info->quota_root = quota_root;
> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> - spin_unlock(&fs_info->qgroup_lock);
> -
> ret = qgroup_rescan_init(fs_info, 0, 1);
In addition to what I mentioned in the previous mail, this is also not
correct because the qgroup_rescan_init() call will always fail if
BTRFS_FS_QUOTA_ENABLED is not set (with an -EBUSY error).
Have you tested this at all? Every attempt to enable quotas will result
in a -EBUSY being returned from the ioctl.
All we need is:
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index db723c0026bd..67818b2f316f 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1174,6 +1174,17 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
fs_info->qgroup_rescan_running = true;
btrfs_queue_work(fs_info->qgroup_rescan_workers,
&fs_info->qgroup_rescan_work);
+ } else {
+ /*
+ * We have set both BTRFS_FS_QUOTA_ENABLED and
+ * BTRFS_QGROUP_STATUS_FLAG_ON, so we can only fail with
+ * -EINPROGRESS because someone started the rescan worker with
+ * the ioctl before we attempted to initialize it.
+ * Ignore such error, and any other error would need to undo
+ * everything we did in the transaction we just committed.
+ */
+ ASSERT(ret == -EINPROGRESS);
+ ret = 0;
}
out_free_path:
> if (!ret) {
> + /*
> + * Set quota enabled flag after committing the transaction, to
> + * avoid deadlocks on fs_info->qgroup_ioctl_lock with concurrent
> + * snapshot creation.
> + */
> + spin_lock(&fs_info->qgroup_lock);
> + fs_info->quota_root = quota_root;
> + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> + spin_unlock(&fs_info->qgroup_lock);
> +
> qgroup_rescan_zero_tracking(fs_info);
> fs_info->qgroup_rescan_running = true;
> btrfs_queue_work(fs_info->qgroup_rescan_workers,
> --
> 2.31.1
>
On 2022/8/23 09:59, Ye Bin wrote:
> Syzkaller reported UAF as follows:
> ==================================================================
> BUG: KASAN: use-after-free in btrfs_get_global_root+0x663/0xa10
> Read of size 4 at addr ffff88811ddbb3c0 by task kworker/u16:1/11
>
> CPU: 4 PID: 11 Comm: kworker/u16:1 Not tainted 6.0.0-rc1-next-20220822+ #2
> Workqueue: btrfs-qgroup-rescan btrfs_work_helper
> Call Trace:
> <TASK>
> dump_stack_lvl+0x6e/0x91
> print_report.cold+0xb2/0x6bb
> kasan_report+0xa8/0x130
> kasan_check_range+0x13f/0x1d0
> btrfs_get_global_root+0x663/0xa10
> btrfs_get_fs_root_commit_root+0xa5/0x150
> find_parent_nodes+0x92f/0x2990
> btrfs_find_all_roots_safe+0x12d/0x220
> btrfs_find_all_roots+0xbb/0xd0
This means, we're looking for all of the parents subvolumes roots of a
bytenr.
Function btrfs_get_fs_root_commit_root() can only be called for indirect
ref, and at that stage, we're pretty sure that we're grabbing a
subvolume root.
> btrfs_qgroup_rescan_worker+0x600/0xc30
> btrfs_work_helper+0xff/0x750
> process_one_work+0x52c/0x930
> worker_thread+0x352/0x8c0
> kthread+0x1b9/0x200
> ret_from_fork+0x22/0x30
> </TASK>
>
> Allocated by task 1895:
> kasan_save_stack+0x1e/0x40
> __kasan_kmalloc+0xa9/0xe0
> btrfs_alloc_root+0x40/0x820
> btrfs_create_tree+0xf8/0x500
But the allocated-by stack shows, the root belongs to quota root, the
only root that is created during qgroup enabling.
This doesn't sound sane to me.
The stacks are saying that, at least some tree blocks are shared by both
a subvolume and quota root, which is already against the on-disk format.
Mind to share the reproducer?
Thanks,
Qu
> btrfs_quota_enable+0x30a/0x1120
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Freed by task 1895:
> kasan_save_stack+0x1e/0x40
> kasan_set_track+0x21/0x30
> kasan_set_free_info+0x20/0x40
> __kasan_slab_free+0x127/0x1c0
> kfree+0xa8/0x2d0
> btrfs_put_root+0x1ca/0x230
> btrfs_quota_enable+0x87c/0x1120
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> ==================================================================
>
> Above issue may happens as follows:
> p1 p2
> btrfs_quota_enable
> spin_lock(&fs_info->qgroup_lock);
> fs_info->quota_root = quota_root;
> spin_unlock(&fs_info->qgroup_lock);
>
> ret = qgroup_rescan_init -> return error
> if (ret)
> btrfs_put_root(quota_root);
> kfree(root);
>
> if (ret) {
> ulist_free(fs_info->qgroup_ulist);
> fs_info->qgroup_ulist = NULL;
> btrfs_sysfs_del_qgroups(fs_info);
> } btrfs_qgroup_rescan_worker
> btrfs_find_all_roots
> btrfs_find_all_roots_safe
> find_parent_nodes
> btrfs_get_fs_root_commit_root
> btrfs_grab_root(fs_info->quota_root)
> -> quota_root already freed
>
> Syzkaller also reported another issue:
> ==================================================================
> BUG: KASAN: use-after-free in ulist_release+0x30/0xb3
> Read of size 8 at addr ffff88811413d048 by task rep/2921
>
> CPU: 3 PID: 2921 Comm: rep Not tainted 6.0.0-rc1-next-20220822+ #3
> rep[2921] cmdline: ./rep
> Call Trace:
> <TASK>
> dump_stack_lvl+0x6e/0x91
> print_report.cold+0xb2/0x6bb
> kasan_report+0xa8/0x130
> ulist_release+0x30/0xb3
> ulist_reinit+0x16/0x56
> btrfs_qgroup_free_refroot+0x288/0x3f0
> btrfs_qgroup_free_meta_all_pertrans+0xed/0x1e0
> commit_fs_roots+0x28c/0x430
> btrfs_commit_transaction+0x9a6/0x1b40
> btrfs_qgroup_rescan+0x7e/0x130
> btrfs_ioctl+0x48ed/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> </TASK>
>
> Allocated by task 2900:
> kasan_save_stack+0x1e/0x40
> __kasan_kmalloc+0xa9/0xe0
> ulist_alloc+0x5c/0xe0
> btrfs_quota_enable+0x1b2/0x1160
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>
> Freed by task 2900:
> kasan_save_stack+0x1e/0x40
> kasan_set_track+0x21/0x30
> kasan_set_free_info+0x20/0x40
> __kasan_slab_free+0x127/0x1c0
> kfree+0xa8/0x2d0
> ulist_free.cold+0x15/0x1a
> btrfs_quota_enable+0x8bf/0x1160
> btrfs_ioctl+0x50a3/0x59f0
> __x64_sys_ioctl+0x130/0x170
> do_syscall_64+0x3b/0x90
> entry_SYSCALL_64_after_hwframe+0x63/0xcd
> ==================================================================
>
> To solve above issues just set 'fs_info->quota_root' after qgroup_rescan_init
> return success.
>
> Signed-off-by: Ye Bin <[email protected]>
> ---
> fs/btrfs/qgroup.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index db723c0026bd..16f0b038295a 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1158,18 +1158,18 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> if (ret)
> goto out_free_path;
>
> - /*
> - * Set quota enabled flag after committing the transaction, to avoid
> - * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
> - * creation.
> - */
> - spin_lock(&fs_info->qgroup_lock);
> - fs_info->quota_root = quota_root;
> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> - spin_unlock(&fs_info->qgroup_lock);
> -
> ret = qgroup_rescan_init(fs_info, 0, 1);
> if (!ret) {
> + /*
> + * Set quota enabled flag after committing the transaction, to
> + * avoid deadlocks on fs_info->qgroup_ioctl_lock with concurrent
> + * snapshot creation.
> + */
> + spin_lock(&fs_info->qgroup_lock);
> + fs_info->quota_root = quota_root;
> + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> + spin_unlock(&fs_info->qgroup_lock);
> +
> qgroup_rescan_zero_tracking(fs_info);
> fs_info->qgroup_rescan_running = true;
> btrfs_queue_work(fs_info->qgroup_rescan_workers,
On 2022/8/23 17:06, Filipe Manana wrote:
> On Tue, Aug 23, 2022 at 09:59:31AM +0800, Ye Bin wrote:
>> Syzkaller reported UAF as follows:
>> ==================================================================
>> BUG: KASAN: use-after-free in btrfs_get_global_root+0x663/0xa10
>> Read of size 4 at addr ffff88811ddbb3c0 by task kworker/u16:1/11
>>
>> CPU: 4 PID: 11 Comm: kworker/u16:1 Not tainted 6.0.0-rc1-next-20220822+ #2
>> Workqueue: btrfs-qgroup-rescan btrfs_work_helper
>> Call Trace:
>> <TASK>
>> dump_stack_lvl+0x6e/0x91
>> print_report.cold+0xb2/0x6bb
>> kasan_report+0xa8/0x130
>> kasan_check_range+0x13f/0x1d0
>> btrfs_get_global_root+0x663/0xa10
>> btrfs_get_fs_root_commit_root+0xa5/0x150
>> find_parent_nodes+0x92f/0x2990
>> btrfs_find_all_roots_safe+0x12d/0x220
>> btrfs_find_all_roots+0xbb/0xd0
>> btrfs_qgroup_rescan_worker+0x600/0xc30
>> btrfs_work_helper+0xff/0x750
>> process_one_work+0x52c/0x930
>> worker_thread+0x352/0x8c0
>> kthread+0x1b9/0x200
>> ret_from_fork+0x22/0x30
>> </TASK>
>>
>> Allocated by task 1895:
>> kasan_save_stack+0x1e/0x40
>> __kasan_kmalloc+0xa9/0xe0
>> btrfs_alloc_root+0x40/0x820
>> btrfs_create_tree+0xf8/0x500
>> btrfs_quota_enable+0x30a/0x1120
>> btrfs_ioctl+0x50a3/0x59f0
>> __x64_sys_ioctl+0x130/0x170
>> do_syscall_64+0x3b/0x90
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> Freed by task 1895:
>> kasan_save_stack+0x1e/0x40
>> kasan_set_track+0x21/0x30
>> kasan_set_free_info+0x20/0x40
>> __kasan_slab_free+0x127/0x1c0
>> kfree+0xa8/0x2d0
>> btrfs_put_root+0x1ca/0x230
>> btrfs_quota_enable+0x87c/0x1120
>> btrfs_ioctl+0x50a3/0x59f0
>> __x64_sys_ioctl+0x130/0x170
>> do_syscall_64+0x3b/0x90
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> ==================================================================
>>
>> Above issue may happens as follows:
>> p1 p2
>> btrfs_quota_enable
>> spin_lock(&fs_info->qgroup_lock);
>> fs_info->quota_root = quota_root;
>> spin_unlock(&fs_info->qgroup_lock);
>>
>> ret = qgroup_rescan_init -> return error
>> if (ret)
>> btrfs_put_root(quota_root);
>> kfree(root);
>>
>> if (ret) {
>> ulist_free(fs_info->qgroup_ulist);
>> fs_info->qgroup_ulist = NULL;
>> btrfs_sysfs_del_qgroups(fs_info);
>> } btrfs_qgroup_rescan_worker
>> btrfs_find_all_roots
>> btrfs_find_all_roots_safe
>> find_parent_nodes
>> btrfs_get_fs_root_commit_root
>> btrfs_grab_root(fs_info->quota_root)
>> -> quota_root already freed
I have described the process of the issue here. I can write it in more
detail.
>> Syzkaller also reported another issue:
>> ==================================================================
>> BUG: KASAN: use-after-free in ulist_release+0x30/0xb3
>> Read of size 8 at addr ffff88811413d048 by task rep/2921
>>
>> CPU: 3 PID: 2921 Comm: rep Not tainted 6.0.0-rc1-next-20220822+ #3
>> rep[2921] cmdline: ./rep
>> Call Trace:
>> <TASK>
>> dump_stack_lvl+0x6e/0x91
>> print_report.cold+0xb2/0x6bb
>> kasan_report+0xa8/0x130
>> ulist_release+0x30/0xb3
>> ulist_reinit+0x16/0x56
>> btrfs_qgroup_free_refroot+0x288/0x3f0
>> btrfs_qgroup_free_meta_all_pertrans+0xed/0x1e0
>> commit_fs_roots+0x28c/0x430
>> btrfs_commit_transaction+0x9a6/0x1b40
>> btrfs_qgroup_rescan+0x7e/0x130
>> btrfs_ioctl+0x48ed/0x59f0
>> __x64_sys_ioctl+0x130/0x170
>> do_syscall_64+0x3b/0x90
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> </TASK>
>>
>> Allocated by task 2900:
>> kasan_save_stack+0x1e/0x40
>> __kasan_kmalloc+0xa9/0xe0
>> ulist_alloc+0x5c/0xe0
>> btrfs_quota_enable+0x1b2/0x1160
>> btrfs_ioctl+0x50a3/0x59f0
>> __x64_sys_ioctl+0x130/0x170
>> do_syscall_64+0x3b/0x90
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> Freed by task 2900:
>> kasan_save_stack+0x1e/0x40
>> kasan_set_track+0x21/0x30
>> kasan_set_free_info+0x20/0x40
>> __kasan_slab_free+0x127/0x1c0
>> kfree+0xa8/0x2d0
>> ulist_free.cold+0x15/0x1a
>> btrfs_quota_enable+0x8bf/0x1160
>> btrfs_ioctl+0x50a3/0x59f0
>> __x64_sys_ioctl+0x130/0x170
>> do_syscall_64+0x3b/0x90
>> entry_SYSCALL_64_after_hwframe+0x63/0xcd
>> ==================================================================
>>
>> To solve above issues just set 'fs_info->quota_root' after qgroup_rescan_init
>> return success.
>>
>> Signed-off-by: Ye Bin <[email protected]>
>> ---
>> fs/btrfs/qgroup.c | 20 ++++++++++----------
>> 1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index db723c0026bd..16f0b038295a 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -1158,18 +1158,18 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
>> if (ret)
>> goto out_free_path;
>>
>> - /*
>> - * Set quota enabled flag after committing the transaction, to avoid
>> - * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
>> - * creation.
>> - */
>> - spin_lock(&fs_info->qgroup_lock);
>> - fs_info->quota_root = quota_root;
>> - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>> - spin_unlock(&fs_info->qgroup_lock);
>> -
>> ret = qgroup_rescan_init(fs_info, 0, 1);
>> if (!ret) {
>> + /*
>> + * Set quota enabled flag after committing the transaction, to
>> + * avoid deadlocks on fs_info->qgroup_ioctl_lock with concurrent
>> + * snapshot creation.
>> + */
>> + spin_lock(&fs_info->qgroup_lock);
>> + fs_info->quota_root = quota_root;
>> + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
>> + spin_unlock(&fs_info->qgroup_lock);
>> +
> But how can the race happen? The changelog should explain that.
>
> To me this suggests that after we set BTRFS_FS_QUOTA_ENABLED and set the
> quota root, but before we called qgroup_rescan_init() at btrfs_quota_enable(),
> some other task started the rescan worker first - I can only think of
> someone else calling the ioctl to start the rescan worker (btrfs_ioctl_quota_rescan()).
Yes, rescan worker is triggered by ioctl(btrfs_ioctl_quota_rescan()).
> In that case we get "ret == -EINPROGRESS" at btrfs_quota_enable().
>
> So please provide a detailed explanation in the log of how the race can
> happen.
>
> This solution is also buggy. Because in case of an error, we will leave the
> quota tree created, the qgroup relation, etc. That is, we don't undo
> what btrfs_create_tree(), add_qgroup_item(), add_qgroup_rb(), etc did
> Which means a future btrfs_quota_enable() call would fail, and calling
> btrfs_quota_disable() to undo all those things will not work either,
> because fs_info->quota_root is NULL.
As 'ret' is non-zero will release resources which you mentioned.
btrfs_quota_enable
...
out_free_path:
btrfs_free_path(path);
out_free_root:
if (ret)
btrfs_put_root(quota_root);
out:
if (ret) {
ulist_free(fs_info->qgroup_ulist);
fs_info->qgroup_ulist = NULL;
btrfs_sysfs_del_qgroups(fs_info);
}
mutex_unlock(&fs_info->qgroup_ioctl_lock);
if (ret && trans)
btrfs_end_transaction(trans);
else if (trans)
ret = btrfs_end_transaction(trans);
ulist_free(ulist);
...
>
> I would suggest ignoring the error of qgroup_rescan_init() if it's
> -EINPROGRESS, and ASSERT if it's anything different from 0 or
> -EINPROGRESS. Also add a comment mentioning we can get -EINPROGRESS
> because someone may have called the qgroup rescan ioctl.
>
> Thanks.
>
>
>
>
>> qgroup_rescan_zero_tracking(fs_info);
>> fs_info->qgroup_rescan_running = true;
>> btrfs_queue_work(fs_info->qgroup_rescan_workers,
>> --
>> 2.31.1
>>
> .
>
On Tue, Aug 23, 2022 at 07:05:37PM +0800, yebin wrote:
>
>
> On 2022/8/23 17:06, Filipe Manana wrote:
> > On Tue, Aug 23, 2022 at 09:59:31AM +0800, Ye Bin wrote:
> > > Syzkaller reported UAF as follows:
> > > ==================================================================
> > > BUG: KASAN: use-after-free in btrfs_get_global_root+0x663/0xa10
> > > Read of size 4 at addr ffff88811ddbb3c0 by task kworker/u16:1/11
> > >
> > > CPU: 4 PID: 11 Comm: kworker/u16:1 Not tainted 6.0.0-rc1-next-20220822+ #2
> > > Workqueue: btrfs-qgroup-rescan btrfs_work_helper
> > > Call Trace:
> > > <TASK>
> > > dump_stack_lvl+0x6e/0x91
> > > print_report.cold+0xb2/0x6bb
> > > kasan_report+0xa8/0x130
> > > kasan_check_range+0x13f/0x1d0
> > > btrfs_get_global_root+0x663/0xa10
> > > btrfs_get_fs_root_commit_root+0xa5/0x150
> > > find_parent_nodes+0x92f/0x2990
> > > btrfs_find_all_roots_safe+0x12d/0x220
> > > btrfs_find_all_roots+0xbb/0xd0
> > > btrfs_qgroup_rescan_worker+0x600/0xc30
> > > btrfs_work_helper+0xff/0x750
> > > process_one_work+0x52c/0x930
> > > worker_thread+0x352/0x8c0
> > > kthread+0x1b9/0x200
> > > ret_from_fork+0x22/0x30
> > > </TASK>
> > >
> > > Allocated by task 1895:
> > > kasan_save_stack+0x1e/0x40
> > > __kasan_kmalloc+0xa9/0xe0
> > > btrfs_alloc_root+0x40/0x820
> > > btrfs_create_tree+0xf8/0x500
> > > btrfs_quota_enable+0x30a/0x1120
> > > btrfs_ioctl+0x50a3/0x59f0
> > > __x64_sys_ioctl+0x130/0x170
> > > do_syscall_64+0x3b/0x90
> > > entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > >
> > > Freed by task 1895:
> > > kasan_save_stack+0x1e/0x40
> > > kasan_set_track+0x21/0x30
> > > kasan_set_free_info+0x20/0x40
> > > __kasan_slab_free+0x127/0x1c0
> > > kfree+0xa8/0x2d0
> > > btrfs_put_root+0x1ca/0x230
> > > btrfs_quota_enable+0x87c/0x1120
> > > btrfs_ioctl+0x50a3/0x59f0
> > > __x64_sys_ioctl+0x130/0x170
> > > do_syscall_64+0x3b/0x90
> > > entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > > ==================================================================
> > >
> > > Above issue may happens as follows:
> > > p1 p2
> > > btrfs_quota_enable
> > > spin_lock(&fs_info->qgroup_lock);
> > > fs_info->quota_root = quota_root;
> > > spin_unlock(&fs_info->qgroup_lock);
> > >
> > > ret = qgroup_rescan_init -> return error
> > > if (ret)
> > > btrfs_put_root(quota_root);
> > > kfree(root);
> > >
> > > if (ret) {
> > > ulist_free(fs_info->qgroup_ulist);
> > > fs_info->qgroup_ulist = NULL;
> > > btrfs_sysfs_del_qgroups(fs_info);
> > > } btrfs_qgroup_rescan_worker
> > > btrfs_find_all_roots
> > > btrfs_find_all_roots_safe
> > > find_parent_nodes
> > > btrfs_get_fs_root_commit_root
> > > btrfs_grab_root(fs_info->quota_root)
> > > -> quota_root already freed
> I have described the process of the issue here. I can write it in more
> detail.
> > > Syzkaller also reported another issue:
> > > ==================================================================
> > > BUG: KASAN: use-after-free in ulist_release+0x30/0xb3
> > > Read of size 8 at addr ffff88811413d048 by task rep/2921
> > >
> > > CPU: 3 PID: 2921 Comm: rep Not tainted 6.0.0-rc1-next-20220822+ #3
> > > rep[2921] cmdline: ./rep
> > > Call Trace:
> > > <TASK>
> > > dump_stack_lvl+0x6e/0x91
> > > print_report.cold+0xb2/0x6bb
> > > kasan_report+0xa8/0x130
> > > ulist_release+0x30/0xb3
> > > ulist_reinit+0x16/0x56
> > > btrfs_qgroup_free_refroot+0x288/0x3f0
> > > btrfs_qgroup_free_meta_all_pertrans+0xed/0x1e0
> > > commit_fs_roots+0x28c/0x430
> > > btrfs_commit_transaction+0x9a6/0x1b40
> > > btrfs_qgroup_rescan+0x7e/0x130
> > > btrfs_ioctl+0x48ed/0x59f0
> > > __x64_sys_ioctl+0x130/0x170
> > > do_syscall_64+0x3b/0x90
> > > entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > > </TASK>
> > >
> > > Allocated by task 2900:
> > > kasan_save_stack+0x1e/0x40
> > > __kasan_kmalloc+0xa9/0xe0
> > > ulist_alloc+0x5c/0xe0
> > > btrfs_quota_enable+0x1b2/0x1160
> > > btrfs_ioctl+0x50a3/0x59f0
> > > __x64_sys_ioctl+0x130/0x170
> > > do_syscall_64+0x3b/0x90
> > > entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > >
> > > Freed by task 2900:
> > > kasan_save_stack+0x1e/0x40
> > > kasan_set_track+0x21/0x30
> > > kasan_set_free_info+0x20/0x40
> > > __kasan_slab_free+0x127/0x1c0
> > > kfree+0xa8/0x2d0
> > > ulist_free.cold+0x15/0x1a
> > > btrfs_quota_enable+0x8bf/0x1160
> > > btrfs_ioctl+0x50a3/0x59f0
> > > __x64_sys_ioctl+0x130/0x170
> > > do_syscall_64+0x3b/0x90
> > > entry_SYSCALL_64_after_hwframe+0x63/0xcd
> > > ==================================================================
> > >
> > > To solve above issues just set 'fs_info->quota_root' after qgroup_rescan_init
> > > return success.
> > >
> > > Signed-off-by: Ye Bin <[email protected]>
> > > ---
> > > fs/btrfs/qgroup.c | 20 ++++++++++----------
> > > 1 file changed, 10 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > > index db723c0026bd..16f0b038295a 100644
> > > --- a/fs/btrfs/qgroup.c
> > > +++ b/fs/btrfs/qgroup.c
> > > @@ -1158,18 +1158,18 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info)
> > > if (ret)
> > > goto out_free_path;
> > > - /*
> > > - * Set quota enabled flag after committing the transaction, to avoid
> > > - * deadlocks on fs_info->qgroup_ioctl_lock with concurrent snapshot
> > > - * creation.
> > > - */
> > > - spin_lock(&fs_info->qgroup_lock);
> > > - fs_info->quota_root = quota_root;
> > > - set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> > > - spin_unlock(&fs_info->qgroup_lock);
> > > -
> > > ret = qgroup_rescan_init(fs_info, 0, 1);
> > > if (!ret) {
> > > + /*
> > > + * Set quota enabled flag after committing the transaction, to
> > > + * avoid deadlocks on fs_info->qgroup_ioctl_lock with concurrent
> > > + * snapshot creation.
> > > + */
> > > + spin_lock(&fs_info->qgroup_lock);
> > > + fs_info->quota_root = quota_root;
> > > + set_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags);
> > > + spin_unlock(&fs_info->qgroup_lock);
> > > +
> > But how can the race happen? The changelog should explain that.
> >
> > To me this suggests that after we set BTRFS_FS_QUOTA_ENABLED and set the
> > quota root, but before we called qgroup_rescan_init() at btrfs_quota_enable(),
> > some other task started the rescan worker first - I can only think of
> > someone else calling the ioctl to start the rescan worker (btrfs_ioctl_quota_rescan()).
> Yes, rescan worker is triggered by ioctl(btrfs_ioctl_quota_rescan()).
> > In that case we get "ret == -EINPROGRESS" at btrfs_quota_enable().
> >
> > So please provide a detailed explanation in the log of how the race can
> > happen.
> >
> > This solution is also buggy. Because in case of an error, we will leave the
> > quota tree created, the qgroup relation, etc. That is, we don't undo
> > what btrfs_create_tree(), add_qgroup_item(), add_qgroup_rb(), etc did
> > Which means a future btrfs_quota_enable() call would fail, and calling
> > btrfs_quota_disable() to undo all those things will not work either,
> > because fs_info->quota_root is NULL.
> As 'ret' is non-zero will release resources which you mentioned.
Nop, it won't.
I'm talking about what was *done in the transaction*, which at this point
was already committed.
Nothing below under the labels will undo that...
> btrfs_quota_enable
> ...
> out_free_path:
> btrfs_free_path(path);
> out_free_root:
> if (ret)
> btrfs_put_root(quota_root);
> out:
> if (ret) {
> ulist_free(fs_info->qgroup_ulist);
> fs_info->qgroup_ulist = NULL;
> btrfs_sysfs_del_qgroups(fs_info);
> }
> mutex_unlock(&fs_info->qgroup_ioctl_lock);
> if (ret && trans)
> btrfs_end_transaction(trans);
> else if (trans)
> ret = btrfs_end_transaction(trans);
> ulist_free(ulist);
> ...
> >
> > I would suggest ignoring the error of qgroup_rescan_init() if it's
> > -EINPROGRESS, and ASSERT if it's anything different from 0 or
> > -EINPROGRESS. Also add a comment mentioning we can get -EINPROGRESS
> > because someone may have called the qgroup rescan ioctl.
> >
> > Thanks.
> >
> >
> >
> >
> > > qgroup_rescan_zero_tracking(fs_info);
> > > fs_info->qgroup_rescan_running = true;
> > > btrfs_queue_work(fs_info->qgroup_rescan_workers,
> > > --
> > > 2.31.1
> > >
> > .
> >
>