[PATCH 0/2] btrfs: qgroup race fix and cleanup

[PATCH 0/2] btrfs: qgroup race fix and cleanup

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Fix a race that leads to a use-after-free plus cleanup no longer used
qgroup_ulist.

Filipe Manana (2):
  btrfs: qgroup: fix race between quota disable and quota rescan ioctl
  btrfs: qgroup: remove no longer used fs_info->qgroup_ulist

 fs/btrfs/disk-io.c |  1 -
 fs/btrfs/fs.h      |  6 ------
 fs/btrfs/qgroup.c  | 53 +++++++++++++++-------------------------------
 3 files changed, 17 insertions(+), 43 deletions(-)

-- 
2.47.2

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

[fdmanana]

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>
---
 fs/btrfs/qgroup.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index b83d9534adae..0b07431963e8 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,9 +4044,11 @@ 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);
+	}
 	mutex_unlock(&fs_info->qgroup_rescan_lock);
 
 	return 0;
-- 
2.47.2

[PATCH 2/2] btrfs: qgroup: remove no longer used fs_info->qgroup_ulist

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

It's not used anymore after commit 091344508249 ("btrfs: qgroup: use
qgroup_iterator in qgroup_convert_meta()"), so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/disk-io.c |  1 -
 fs/btrfs/fs.h      |  6 ------
 fs/btrfs/qgroup.c  | 31 +------------------------------
 3 files changed, 1 insertion(+), 37 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f6fa951c6be9..ee4911452cfd 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1947,7 +1947,6 @@ static void btrfs_init_qgroup(struct btrfs_fs_info *fs_info)
 	fs_info->qgroup_tree = RB_ROOT;
 	INIT_LIST_HEAD(&fs_info->dirty_qgroups);
 	fs_info->qgroup_seq = 1;
-	fs_info->qgroup_ulist = NULL;
 	fs_info->qgroup_rescan_running = false;
 	fs_info->qgroup_drop_subtree_thres = BTRFS_QGROUP_DROP_SUBTREE_THRES_DEFAULT;
 	mutex_init(&fs_info->qgroup_rescan_lock);
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index b239e4b8421c..a731c883687d 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -740,12 +740,6 @@ struct btrfs_fs_info {
 	struct rb_root qgroup_tree;
 	spinlock_t qgroup_lock;
 
-	/*
-	 * Used to avoid frequently calling ulist_alloc()/ulist_free()
-	 * when doing qgroup accounting, it must be protected by qgroup_lock.
-	 */
-	struct ulist *qgroup_ulist;
-
 	/*
 	 * Protect user change for quota operations. If a transaction is needed,
 	 * it must be started before locking this lock.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 0b07431963e8..aadd1dbf7fb0 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -397,12 +397,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
 	if (!fs_info->quota_root)
 		return 0;
 
-	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
-	if (!fs_info->qgroup_ulist) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
@@ -587,8 +581,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
 		if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)
 			ret = qgroup_rescan_init(fs_info, rescan_progress, 0);
 	} else {
-		ulist_free(fs_info->qgroup_ulist);
-		fs_info->qgroup_ulist = NULL;
 		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
 		btrfs_sysfs_del_qgroups(fs_info);
 	}
@@ -660,13 +652,6 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
 	}
 	spin_unlock(&fs_info->qgroup_lock);
 
-	/*
-	 * We call btrfs_free_qgroup_config() when unmounting
-	 * filesystem and disabling quota, so we set qgroup_ulist
-	 * to be null here to avoid double free.
-	 */
-	ulist_free(fs_info->qgroup_ulist);
-	fs_info->qgroup_ulist = NULL;
 	btrfs_sysfs_del_qgroups(fs_info);
 }
 
@@ -1012,7 +997,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	struct btrfs_qgroup *qgroup = NULL;
 	struct btrfs_qgroup *prealloc = NULL;
 	struct btrfs_trans_handle *trans = NULL;
-	struct ulist *ulist = NULL;
 	const bool simple = (quota_ctl_args->cmd == BTRFS_QUOTA_CTL_ENABLE_SIMPLE_QUOTA);
 	int ret = 0;
 	int slot;
@@ -1035,12 +1019,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (fs_info->quota_root)
 		goto out;
 
-	ulist = ulist_alloc(GFP_KERNEL);
-	if (!ulist) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	ret = btrfs_sysfs_add_qgroups(fs_info);
 	if (ret < 0)
 		goto out;
@@ -1080,9 +1058,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (fs_info->quota_root)
 		goto out;
 
-	fs_info->qgroup_ulist = ulist;
-	ulist = NULL;
-
 	/*
 	 * initially create the quota tree
 	 */
@@ -1281,17 +1256,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (ret)
 		btrfs_put_root(quota_root);
 out:
-	if (ret) {
-		ulist_free(fs_info->qgroup_ulist);
-		fs_info->qgroup_ulist = NULL;
+	if (ret)
 		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);
 	kfree(prealloc);
 	return ret;
 }
-- 
2.47.2

[PATCH v2 0/2] btrfs: qgroup race fix and cleanup

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Fix a race that leads to a use-after-free plus cleanup no longer used
qgroup_ulist.

V2: Update patch 1/2 to return -ENOTCONN if quotas are disabled when
    we try to start the rescan worker.

Filipe Manana (2):
  btrfs: qgroup: fix race between quota disable and quota rescan ioctl
  btrfs: qgroup: remove no longer used fs_info->qgroup_ulist

 fs/btrfs/disk-io.c |  1 -
 fs/btrfs/fs.h      |  6 -----
 fs/btrfs/qgroup.c  | 55 ++++++++++++++++------------------------------
 3 files changed, 19 insertions(+), 43 deletions(-)

-- 
2.47.2

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

[fdmanana]

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>
---
 fs/btrfs/qgroup.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index b83d9534adae..b24cb54e4f42 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,9 +4044,13 @@ 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;
-- 
2.47.2

[PATCH v2 2/2] btrfs: qgroup: remove no longer used fs_info->qgroup_ulist

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

It's not used anymore after commit 091344508249 ("btrfs: qgroup: use
qgroup_iterator in qgroup_convert_meta()"), so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/disk-io.c |  1 -
 fs/btrfs/fs.h      |  6 ------
 fs/btrfs/qgroup.c  | 31 +------------------------------
 3 files changed, 1 insertion(+), 37 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f6fa951c6be9..ee4911452cfd 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1947,7 +1947,6 @@ static void btrfs_init_qgroup(struct btrfs_fs_info *fs_info)
 	fs_info->qgroup_tree = RB_ROOT;
 	INIT_LIST_HEAD(&fs_info->dirty_qgroups);
 	fs_info->qgroup_seq = 1;
-	fs_info->qgroup_ulist = NULL;
 	fs_info->qgroup_rescan_running = false;
 	fs_info->qgroup_drop_subtree_thres = BTRFS_QGROUP_DROP_SUBTREE_THRES_DEFAULT;
 	mutex_init(&fs_info->qgroup_rescan_lock);
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index b239e4b8421c..a731c883687d 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -740,12 +740,6 @@ struct btrfs_fs_info {
 	struct rb_root qgroup_tree;
 	spinlock_t qgroup_lock;
 
-	/*
-	 * Used to avoid frequently calling ulist_alloc()/ulist_free()
-	 * when doing qgroup accounting, it must be protected by qgroup_lock.
-	 */
-	struct ulist *qgroup_ulist;
-
 	/*
 	 * Protect user change for quota operations. If a transaction is needed,
 	 * it must be started before locking this lock.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index b24cb54e4f42..d689afa11327 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -397,12 +397,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
 	if (!fs_info->quota_root)
 		return 0;
 
-	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
-	if (!fs_info->qgroup_ulist) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
@@ -587,8 +581,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
 		if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)
 			ret = qgroup_rescan_init(fs_info, rescan_progress, 0);
 	} else {
-		ulist_free(fs_info->qgroup_ulist);
-		fs_info->qgroup_ulist = NULL;
 		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
 		btrfs_sysfs_del_qgroups(fs_info);
 	}
@@ -660,13 +652,6 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
 	}
 	spin_unlock(&fs_info->qgroup_lock);
 
-	/*
-	 * We call btrfs_free_qgroup_config() when unmounting
-	 * filesystem and disabling quota, so we set qgroup_ulist
-	 * to be null here to avoid double free.
-	 */
-	ulist_free(fs_info->qgroup_ulist);
-	fs_info->qgroup_ulist = NULL;
 	btrfs_sysfs_del_qgroups(fs_info);
 }
 
@@ -1012,7 +997,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	struct btrfs_qgroup *qgroup = NULL;
 	struct btrfs_qgroup *prealloc = NULL;
 	struct btrfs_trans_handle *trans = NULL;
-	struct ulist *ulist = NULL;
 	const bool simple = (quota_ctl_args->cmd == BTRFS_QUOTA_CTL_ENABLE_SIMPLE_QUOTA);
 	int ret = 0;
 	int slot;
@@ -1035,12 +1019,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (fs_info->quota_root)
 		goto out;
 
-	ulist = ulist_alloc(GFP_KERNEL);
-	if (!ulist) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	ret = btrfs_sysfs_add_qgroups(fs_info);
 	if (ret < 0)
 		goto out;
@@ -1080,9 +1058,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (fs_info->quota_root)
 		goto out;
 
-	fs_info->qgroup_ulist = ulist;
-	ulist = NULL;
-
 	/*
 	 * initially create the quota tree
 	 */
@@ -1281,17 +1256,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (ret)
 		btrfs_put_root(quota_root);
 out:
-	if (ret) {
-		ulist_free(fs_info->qgroup_ulist);
-		fs_info->qgroup_ulist = NULL;
+	if (ret)
 		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);
 	kfree(prealloc);
 	return ret;
 }
-- 
2.47.2

[PATCH v3 0/2] btrfs: qgroup race fix and cleanup

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Fix a race that leads to a use-after-free plus cleanup no longer used
qgroup_ulist.

V2: Update patch 1/2 to return -ENOTCONN if quotas are disabled when
    we try to start the rescan worker.
V3: Make patch 1/2 actually return -ENOTCONN.

Filipe Manana (2):
  btrfs: qgroup: fix race between quota disable and quota rescan ioctl
  btrfs: qgroup: remove no longer used fs_info->qgroup_ulist

 fs/btrfs/disk-io.c |  1 -
 fs/btrfs/fs.h      |  6 -----
 fs/btrfs/qgroup.c  | 57 ++++++++++++++++------------------------------
 3 files changed, 20 insertions(+), 44 deletions(-)

-- 
2.47.2

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

[fdmanana]

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>
---
 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,
-- 
2.47.2

[PATCH v3 2/2] btrfs: qgroup: remove no longer used fs_info->qgroup_ulist

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

It's not used anymore after commit 091344508249 ("btrfs: qgroup: use
qgroup_iterator in qgroup_convert_meta()"), so remove it.

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/disk-io.c |  1 -
 fs/btrfs/fs.h      |  6 ------
 fs/btrfs/qgroup.c  | 31 +------------------------------
 3 files changed, 1 insertion(+), 37 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f6fa951c6be9..ee4911452cfd 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1947,7 +1947,6 @@ static void btrfs_init_qgroup(struct btrfs_fs_info *fs_info)
 	fs_info->qgroup_tree = RB_ROOT;
 	INIT_LIST_HEAD(&fs_info->dirty_qgroups);
 	fs_info->qgroup_seq = 1;
-	fs_info->qgroup_ulist = NULL;
 	fs_info->qgroup_rescan_running = false;
 	fs_info->qgroup_drop_subtree_thres = BTRFS_QGROUP_DROP_SUBTREE_THRES_DEFAULT;
 	mutex_init(&fs_info->qgroup_rescan_lock);
diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
index b239e4b8421c..a731c883687d 100644
--- a/fs/btrfs/fs.h
+++ b/fs/btrfs/fs.h
@@ -740,12 +740,6 @@ struct btrfs_fs_info {
 	struct rb_root qgroup_tree;
 	spinlock_t qgroup_lock;
 
-	/*
-	 * Used to avoid frequently calling ulist_alloc()/ulist_free()
-	 * when doing qgroup accounting, it must be protected by qgroup_lock.
-	 */
-	struct ulist *qgroup_ulist;
-
 	/*
 	 * Protect user change for quota operations. If a transaction is needed,
 	 * it must be started before locking this lock.
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 8fa874ef80b3..98a53e6edb2c 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -397,12 +397,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
 	if (!fs_info->quota_root)
 		return 0;
 
-	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
-	if (!fs_info->qgroup_ulist) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	path = btrfs_alloc_path();
 	if (!path) {
 		ret = -ENOMEM;
@@ -587,8 +581,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
 		if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)
 			ret = qgroup_rescan_init(fs_info, rescan_progress, 0);
 	} else {
-		ulist_free(fs_info->qgroup_ulist);
-		fs_info->qgroup_ulist = NULL;
 		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
 		btrfs_sysfs_del_qgroups(fs_info);
 	}
@@ -660,13 +652,6 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
 	}
 	spin_unlock(&fs_info->qgroup_lock);
 
-	/*
-	 * We call btrfs_free_qgroup_config() when unmounting
-	 * filesystem and disabling quota, so we set qgroup_ulist
-	 * to be null here to avoid double free.
-	 */
-	ulist_free(fs_info->qgroup_ulist);
-	fs_info->qgroup_ulist = NULL;
 	btrfs_sysfs_del_qgroups(fs_info);
 }
 
@@ -1012,7 +997,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	struct btrfs_qgroup *qgroup = NULL;
 	struct btrfs_qgroup *prealloc = NULL;
 	struct btrfs_trans_handle *trans = NULL;
-	struct ulist *ulist = NULL;
 	const bool simple = (quota_ctl_args->cmd == BTRFS_QUOTA_CTL_ENABLE_SIMPLE_QUOTA);
 	int ret = 0;
 	int slot;
@@ -1035,12 +1019,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (fs_info->quota_root)
 		goto out;
 
-	ulist = ulist_alloc(GFP_KERNEL);
-	if (!ulist) {
-		ret = -ENOMEM;
-		goto out;
-	}
-
 	ret = btrfs_sysfs_add_qgroups(fs_info);
 	if (ret < 0)
 		goto out;
@@ -1080,9 +1058,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (fs_info->quota_root)
 		goto out;
 
-	fs_info->qgroup_ulist = ulist;
-	ulist = NULL;
-
 	/*
 	 * initially create the quota tree
 	 */
@@ -1281,17 +1256,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
 	if (ret)
 		btrfs_put_root(quota_root);
 out:
-	if (ret) {
-		ulist_free(fs_info->qgroup_ulist);
-		fs_info->qgroup_ulist = NULL;
+	if (ret)
 		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);
 	kfree(prealloc);
 	return ret;
 }
-- 
2.47.2

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

[Boris Burkov]

On Mon, Jun 30, 2025 at 02:07:47PM +0100, fdmanana@kernel.org wrote:
> 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: Boris Burkov <boris@bur.io>

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

could this be one of the helpers like !btrfs_qgroup_enabled() or maybe
even better !btrfs_qgroup_full_accounting()?

> +		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,
> -- 
> 2.47.2
> 

Re: [PATCH v3 2/2] btrfs: qgroup: remove no longer used fs_info->qgroup_ulist

[Boris Burkov]

On Mon, Jun 30, 2025 at 02:07:48PM +0100, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> It's not used anymore after commit 091344508249 ("btrfs: qgroup: use
> qgroup_iterator in qgroup_convert_meta()"), so remove it.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Boris Burkov <boris@bur.io>

> ---
>  fs/btrfs/disk-io.c |  1 -
>  fs/btrfs/fs.h      |  6 ------
>  fs/btrfs/qgroup.c  | 31 +------------------------------
>  3 files changed, 1 insertion(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index f6fa951c6be9..ee4911452cfd 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1947,7 +1947,6 @@ static void btrfs_init_qgroup(struct btrfs_fs_info *fs_info)
>  	fs_info->qgroup_tree = RB_ROOT;
>  	INIT_LIST_HEAD(&fs_info->dirty_qgroups);
>  	fs_info->qgroup_seq = 1;
> -	fs_info->qgroup_ulist = NULL;
>  	fs_info->qgroup_rescan_running = false;
>  	fs_info->qgroup_drop_subtree_thres = BTRFS_QGROUP_DROP_SUBTREE_THRES_DEFAULT;
>  	mutex_init(&fs_info->qgroup_rescan_lock);
> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index b239e4b8421c..a731c883687d 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -740,12 +740,6 @@ struct btrfs_fs_info {
>  	struct rb_root qgroup_tree;
>  	spinlock_t qgroup_lock;
>  
> -	/*
> -	 * Used to avoid frequently calling ulist_alloc()/ulist_free()
> -	 * when doing qgroup accounting, it must be protected by qgroup_lock.
> -	 */
> -	struct ulist *qgroup_ulist;
> -
>  	/*
>  	 * Protect user change for quota operations. If a transaction is needed,
>  	 * it must be started before locking this lock.
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 8fa874ef80b3..98a53e6edb2c 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -397,12 +397,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
>  	if (!fs_info->quota_root)
>  		return 0;
>  
> -	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
> -	if (!fs_info->qgroup_ulist) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
>  	path = btrfs_alloc_path();
>  	if (!path) {
>  		ret = -ENOMEM;
> @@ -587,8 +581,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
>  		if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)
>  			ret = qgroup_rescan_init(fs_info, rescan_progress, 0);
>  	} else {
> -		ulist_free(fs_info->qgroup_ulist);
> -		fs_info->qgroup_ulist = NULL;
>  		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
>  		btrfs_sysfs_del_qgroups(fs_info);
>  	}
> @@ -660,13 +652,6 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
>  	}
>  	spin_unlock(&fs_info->qgroup_lock);
>  
> -	/*
> -	 * We call btrfs_free_qgroup_config() when unmounting
> -	 * filesystem and disabling quota, so we set qgroup_ulist
> -	 * to be null here to avoid double free.
> -	 */
> -	ulist_free(fs_info->qgroup_ulist);
> -	fs_info->qgroup_ulist = NULL;
>  	btrfs_sysfs_del_qgroups(fs_info);
>  }
>  
> @@ -1012,7 +997,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>  	struct btrfs_qgroup *qgroup = NULL;
>  	struct btrfs_qgroup *prealloc = NULL;
>  	struct btrfs_trans_handle *trans = NULL;
> -	struct ulist *ulist = NULL;
>  	const bool simple = (quota_ctl_args->cmd == BTRFS_QUOTA_CTL_ENABLE_SIMPLE_QUOTA);
>  	int ret = 0;
>  	int slot;
> @@ -1035,12 +1019,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>  	if (fs_info->quota_root)
>  		goto out;
>  
> -	ulist = ulist_alloc(GFP_KERNEL);
> -	if (!ulist) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
>  	ret = btrfs_sysfs_add_qgroups(fs_info);
>  	if (ret < 0)
>  		goto out;
> @@ -1080,9 +1058,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>  	if (fs_info->quota_root)
>  		goto out;
>  
> -	fs_info->qgroup_ulist = ulist;
> -	ulist = NULL;
> -
>  	/*
>  	 * initially create the quota tree
>  	 */
> @@ -1281,17 +1256,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>  	if (ret)
>  		btrfs_put_root(quota_root);
>  out:
> -	if (ret) {
> -		ulist_free(fs_info->qgroup_ulist);
> -		fs_info->qgroup_ulist = NULL;
> +	if (ret)
>  		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);
>  	kfree(prealloc);
>  	return ret;
>  }
> -- 
> 2.47.2
> 

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

[Filipe Manana]

On Mon, Jun 30, 2025 at 5:31 PM Boris Burkov <boris@bur.io> wrote:
>
> On Mon, Jun 30, 2025 at 02:07:47PM +0100, fdmanana@kernel.org wrote:
> > 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: Boris Burkov <boris@bur.io>
>
> > ---
> >  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)) {
>
> could this be one of the helpers like !btrfs_qgroup_enabled() or maybe
> even better !btrfs_qgroup_full_accounting()?

Yes, that's fine. I'll update to that when pushing to for-next:

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 98a53e6edb2c..5f1b4990f56f 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -4015,7 +4015,12 @@ btrfs_qgroup_rescan(struct btrfs_fs_info *fs_info)
        qgroup_rescan_zero_tracking(fs_info);

        mutex_lock(&fs_info->qgroup_rescan_lock);
-       if (test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags)) {
+       /*
+        * The rescan worker is only for full accounting qgroups, check if it's
+        * enabled as it is pointless to queue it otherwise. A concurrent quota
+        * disable may also have just cleared BTRFS_FS_QUOTA_ENABLED.
+        */
+       if (btrfs_qgroup_full_accounting(fs_info)) {
                fs_info->qgroup_rescan_running = true;
                btrfs_queue_work(fs_info->qgroup_rescan_workers,
                                 &fs_info->qgroup_rescan_work);


Thanks.

>
> > +             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,
> > --
> > 2.47.2
> >

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

[Qu Wenruo]

在 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,

Re: [PATCH v3 2/2] btrfs: qgroup: remove no longer used fs_info->qgroup_ulist

[Qu Wenruo]

在 2025/6/30 22:37, fdmanana@kernel.org 写道:
> From: Filipe Manana <fdmanana@suse.com>
> 
> It's not used anymore after commit 091344508249 ("btrfs: qgroup: use
> qgroup_iterator in qgroup_convert_meta()"), so remove it.
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Thanks a lot for catching this.

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

Thanks,
Qu

> ---
>   fs/btrfs/disk-io.c |  1 -
>   fs/btrfs/fs.h      |  6 ------
>   fs/btrfs/qgroup.c  | 31 +------------------------------
>   3 files changed, 1 insertion(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index f6fa951c6be9..ee4911452cfd 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1947,7 +1947,6 @@ static void btrfs_init_qgroup(struct btrfs_fs_info *fs_info)
>   	fs_info->qgroup_tree = RB_ROOT;
>   	INIT_LIST_HEAD(&fs_info->dirty_qgroups);
>   	fs_info->qgroup_seq = 1;
> -	fs_info->qgroup_ulist = NULL;
>   	fs_info->qgroup_rescan_running = false;
>   	fs_info->qgroup_drop_subtree_thres = BTRFS_QGROUP_DROP_SUBTREE_THRES_DEFAULT;
>   	mutex_init(&fs_info->qgroup_rescan_lock);
> diff --git a/fs/btrfs/fs.h b/fs/btrfs/fs.h
> index b239e4b8421c..a731c883687d 100644
> --- a/fs/btrfs/fs.h
> +++ b/fs/btrfs/fs.h
> @@ -740,12 +740,6 @@ struct btrfs_fs_info {
>   	struct rb_root qgroup_tree;
>   	spinlock_t qgroup_lock;
>   
> -	/*
> -	 * Used to avoid frequently calling ulist_alloc()/ulist_free()
> -	 * when doing qgroup accounting, it must be protected by qgroup_lock.
> -	 */
> -	struct ulist *qgroup_ulist;
> -
>   	/*
>   	 * Protect user change for quota operations. If a transaction is needed,
>   	 * it must be started before locking this lock.
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 8fa874ef80b3..98a53e6edb2c 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -397,12 +397,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
>   	if (!fs_info->quota_root)
>   		return 0;
>   
> -	fs_info->qgroup_ulist = ulist_alloc(GFP_KERNEL);
> -	if (!fs_info->qgroup_ulist) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
>   	path = btrfs_alloc_path();
>   	if (!path) {
>   		ret = -ENOMEM;
> @@ -587,8 +581,6 @@ int btrfs_read_qgroup_config(struct btrfs_fs_info *fs_info)
>   		if (fs_info->qgroup_flags & BTRFS_QGROUP_STATUS_FLAG_RESCAN)
>   			ret = qgroup_rescan_init(fs_info, rescan_progress, 0);
>   	} else {
> -		ulist_free(fs_info->qgroup_ulist);
> -		fs_info->qgroup_ulist = NULL;
>   		fs_info->qgroup_flags &= ~BTRFS_QGROUP_STATUS_FLAG_RESCAN;
>   		btrfs_sysfs_del_qgroups(fs_info);
>   	}
> @@ -660,13 +652,6 @@ void btrfs_free_qgroup_config(struct btrfs_fs_info *fs_info)
>   	}
>   	spin_unlock(&fs_info->qgroup_lock);
>   
> -	/*
> -	 * We call btrfs_free_qgroup_config() when unmounting
> -	 * filesystem and disabling quota, so we set qgroup_ulist
> -	 * to be null here to avoid double free.
> -	 */
> -	ulist_free(fs_info->qgroup_ulist);
> -	fs_info->qgroup_ulist = NULL;
>   	btrfs_sysfs_del_qgroups(fs_info);
>   }
>   
> @@ -1012,7 +997,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   	struct btrfs_qgroup *qgroup = NULL;
>   	struct btrfs_qgroup *prealloc = NULL;
>   	struct btrfs_trans_handle *trans = NULL;
> -	struct ulist *ulist = NULL;
>   	const bool simple = (quota_ctl_args->cmd == BTRFS_QUOTA_CTL_ENABLE_SIMPLE_QUOTA);
>   	int ret = 0;
>   	int slot;
> @@ -1035,12 +1019,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   	if (fs_info->quota_root)
>   		goto out;
>   
> -	ulist = ulist_alloc(GFP_KERNEL);
> -	if (!ulist) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
>   	ret = btrfs_sysfs_add_qgroups(fs_info);
>   	if (ret < 0)
>   		goto out;
> @@ -1080,9 +1058,6 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   	if (fs_info->quota_root)
>   		goto out;
>   
> -	fs_info->qgroup_ulist = ulist;
> -	ulist = NULL;
> -
>   	/*
>   	 * initially create the quota tree
>   	 */
> @@ -1281,17 +1256,13 @@ int btrfs_quota_enable(struct btrfs_fs_info *fs_info,
>   	if (ret)
>   		btrfs_put_root(quota_root);
>   out:
> -	if (ret) {
> -		ulist_free(fs_info->qgroup_ulist);
> -		fs_info->qgroup_ulist = NULL;
> +	if (ret)
>   		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);
>   	kfree(prealloc);
>   	return ret;
>   }