[PATCH 1/1] bcache: fix use-after-free when waiting for dirty-init threads

[Chen Fan <fan.chen@easystack.cn>]

We use multiple threads to count dirty sectors in parallel (read-only).
bch_sectors_dirty_init() waits for the last dirty-init thread to set
state.started to 0.

However, after the last thread sets state.started to 0 but before it calls
wake_up(&state->wait), bch_sectors_dirty_init() may observe started == 0,
return from wait_event() directly, and free local variable state.
The subsequent wake_up(&state->wait) then accesses freed memory, because
bch_sectors_dirty_init() has already exited.

Fix this by using a completion to wait for all dirty-init threads, so that
no dirty-init thread can still access state after the wait completes.

This issue was triggered in our environment with the following stack:
------------[ cut here ]------------
[30433.391386] Internal error: Oops: 0000000096000047 [#1] SMP
[30433.585804] CPU: 54 PID: 48242 Comm: bch_dirtcnt[1] Kdump: loaded Tainted: G  6.6.0-98.0.0.90.aarch64 #1
[30433.718972] Call trace:
[30433.722722]  native_queued_spin_lock_slowpath+0x33c/0x420
[30433.729401]  __wake_up_common_lock+0xbc/0x118
[30433.735050]  __wake_up+0x20/0x38
[30433.739575]  bch_dirty_init_thread+0x1c0/0x1f8 [bcache]
[30433.746169]  kthread+0xf0/0x108
[30433.750583]  ret_from_fork+0x10/0x20
[30433.755425] Code: 91002020 8b040284 f862d8a5 aa0403e2 (f8256841)
      ---[ end trace ]---

Signed-off-by: Chen Fan <fan.chen@easystack.cn>
---
 drivers/md/bcache/writeback.c | 31 ++++++++++++-------------------
 drivers/md/bcache/writeback.h |  4 +---
 2 files changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 4b237074f453..e74457a8c952 100644
--- a/drivers/md/bcache/writeback.c
+++ b/drivers/md/bcache/writeback.c
@@ -16,6 +16,7 @@
 #include <linux/kthread.h>
 #include <linux/sched/clock.h>
 #include <trace/events/bcache.h>
+#include <linux/completion.h>
 
 static void update_gc_after_writeback(struct cache_set *c)
 {
@@ -953,12 +954,7 @@ static int bch_dirty_init_thread(void *arg)
 	}
 
 out:
-	/* In order to wake up state->wait in time */
-	smp_mb__before_atomic();
-	if (atomic_dec_and_test(&state->started))
-		wake_up(&state->wait);
-
-	return 0;
+	kthread_complete_and_exit(&state->done, 0);
 }
 
 static int bch_btre_dirty_init_thread_nr(void)
@@ -975,8 +971,9 @@ static int bch_btre_dirty_init_thread_nr(void)
 
 void bch_sectors_dirty_init(struct bcache_device *d)
 {
-	int i;
+	int i, launched = 0;
 	struct btree *b = NULL;
+	struct task_struct *thread;
 	struct bkey *k = NULL;
 	struct btree_iter_stack iter;
 	struct sectors_dirty_init op;
@@ -1014,9 +1011,8 @@ void bch_sectors_dirty_init(struct bcache_device *d)
 	state.total_threads = bch_btre_dirty_init_thread_nr();
 	state.key_idx = 0;
 	spin_lock_init(&state.idx_lock);
-	atomic_set(&state.started, 0);
 	atomic_set(&state.enough, 0);
-	init_waitqueue_head(&state.wait);
+	init_completion(&state.done);
 
 	for (i = 0; i < state.total_threads; i++) {
 		/* Fetch latest state.enough earlier */
@@ -1024,23 +1020,20 @@ void bch_sectors_dirty_init(struct bcache_device *d)
 		if (atomic_read(&state.enough))
 			break;
 
-		atomic_inc(&state.started);
 		state.infos[i].state = &state;
-		state.infos[i].thread =
+		thread =
 			kthread_run(bch_dirty_init_thread, &state.infos[i],
 				    "bch_dirtcnt[%d]", i);
-		if (IS_ERR(state.infos[i].thread)) {
+		if (IS_ERR(thread)) {
 			pr_err("fails to run thread bch_dirty_init[%d]\n", i);
-			atomic_dec(&state.started);
-			for (--i; i >= 0; i--)
-				kthread_stop(state.infos[i].thread);
-			goto out;
+			break;
 		}
+		launched++;
 	}
 
-out:
-	/* Must wait for all threads to stop. */
-	wait_event(state.wait, atomic_read(&state.started) == 0);
+	/* Wait for all threads to finish accessing state. */
+	for (i = 0; i < launched; i++)
+		wait_for_completion(&state.done);
 	rw_unlock(0, b);
 }
 
diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
index 31df716951f6..e6c74be20fc6 100644
--- a/drivers/md/bcache/writeback.h
+++ b/drivers/md/bcache/writeback.h
@@ -31,7 +31,6 @@
 struct bch_dirty_init_state;
 struct dirty_init_thrd_info {
 	struct bch_dirty_init_state	*state;
-	struct task_struct		*thread;
 };
 
 struct bch_dirty_init_state {
@@ -40,9 +39,8 @@ struct bch_dirty_init_state {
 	int				total_threads;
 	int				key_idx;
 	spinlock_t			idx_lock;
-	atomic_t			started;
 	atomic_t			enough;
-	wait_queue_head_t		wait;
+	struct completion		done;
 	struct dirty_init_thrd_info	infos[BCH_DIRTY_INIT_THRD_MAX];
 };
 
-- 
2.43.0