Re: [PATCH v3 1/2] btrfs: qgroup: fix race between quota disable and quota rescan ioctl

[Qu Wenruo <wqu@suse.com>]

在 2025/6/30 22:37, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> There's a race between a task disabling quotas and another running the
> rescan ioctl that can result in a use-after-free of qgroup records from
> the fs_info->qgroup_tree rbtree.
> 
> This happens as follows:
> 
> 1) Task A enters btrfs_ioctl_quota_rescan() -> btrfs_qgroup_rescan();
> 
> 2) Task B enters btrfs_quota_disable() and calls
>     btrfs_qgroup_wait_for_completion(), which does nothing because at that
>     point fs_info->qgroup_rescan_running is false (it wasn't set yet by
>     task A);
> 
> 3) Task B calls btrfs_free_qgroup_config() which starts freeing qgroups
>     from fs_info->qgroup_tree without taking the lock fs_info->qgroup_lock;
> 
> 4) Task A enters qgroup_rescan_zero_tracking() which starts iterating
>     the fs_info->qgroup_tree tree while holding fs_info->qgroup_lock,
>     but task B is freeing qgroup records from that tree without holding
>     the lock, resulting in a use-after-free.
> 
> Fix this by taking fs_info->qgroup_lock at btrfs_free_qgroup_config().
> Also at btrfs_qgroup_rescan() don't start the rescan worker if quotas
> were already disabled.
> 
> Reported-by: cen zhang <zzzccc427@gmail.com>
> Link: https://lore.kernel.org/linux-btrfs/CAFRLqsV+cMDETFuzqdKSHk_FDm6tneea45krsHqPD6B3FetLpQ@mail.gmail.com/
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu


> ---
>   fs/btrfs/qgroup.c | 26 +++++++++++++++++++-------
>   1 file changed, 19 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index b83d9534adae..8fa874ef80b3 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -636,22 +636,30 @@ bool btrfs_check_quota_leak(const struct btrfs_fs_info *fs_info)
>   
>   /*
>    * This is called from close_ctree() or open_ctree() or btrfs_quota_disable(),
> - * first two are in single-threaded paths.And for the third one, we have set
> - * quota_root to be null with qgroup_lock held before, so it is safe to clean
> - * up the in-memory structures without qgroup_lock held.
> + * first two are in single-threaded paths.
>    */
>   void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
>   {
>   	struct rb_node *n;
>   	struct btrfs_qgroup *qgroup;
>   
> +	/*
> +	 * btrfs_quota_disable() can be called concurrently with
> +	 * btrfs_qgroup_rescan() -> qgroup_rescan_zero_tracking(), so take the
> +	 * lock.
> +	 */
> +	spin_lock(&fs_info->qgroup_lock);
>   	while ((n = rb_first(&fs_info->qgroup_tree))) {
>   		qgroup = rb_entry(n, struct btrfs_qgroup, node);
>   		rb_erase(n, &fs_info->qgroup_tree);
>   		__del_qgroup_rb(qgroup);
> +		spin_unlock(&fs_info->qgroup_lock);
>   		btrfs_sysfs_del_one_qgroup(fs_info, qgroup);
>   		kfree(qgroup);
> +		spin_lock(&fs_info->qgroup_lock);
>   	}
> +	spin_unlock(&fs_info->qgroup_lock);
> +
>   	/*
>   	 * We call btrfs_free_qgroup_config() when unmounting
>   	 * filesystem and disabling quota, so we set qgroup_ulist
> @@ -4036,12 +4044,16 @@ btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)
>   	qgroup_rescan_zero_tracking(fs_info);
>   
>   	mutex_lock(&fs_info->qgroup_rescan_lock);
> -	fs_info->qgroup_rescan_running = true;
> -	btrfs_queue_work(fs_info->qgroup_rescan_workers,
> -			 &fs_info->qgroup_rescan_work);
> +	if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) {
> +		fs_info->qgroup_rescan_running = true;
> +		btrfs_queue_work(fs_info->qgroup_rescan_workers,
> +				 &fs_info->qgroup_rescan_work);
> +	} else {
> +		ret = -ENOTCONN;
> +	}
>   	mutex_unlock(&fs_info->qgroup_rescan_lock);
>   
> -	return 0;
> +	return ret;
>   }
>   
>   int btrfs_qgroup_wait_for_completion(struct btrfs_fs_info *fs_info,