Re: [PATCH v2] bcache: fix UAF in cached_dev_free and safely flush/destroy

 Hi Coly and Kent,

Sorry for the earlier omissions. Here is a concise correction and completion.

1) Could you please point out exactly which reference is still held?
From the crash we analyzed, there isn’t a leaked struct cached_dev
reference causing the panic. The long-lived references during teardown
behave as expected:

dc->disk.cl (closure) is taken/released around detach/free paths.

dc->count (refcount) gates device lifetime.

The actual failure is a stop-ownership race on dc->writeback_thread:
one path stops the kthread and it exits (freeing the task), while
another racing path still calls kthread_stop() on a stale pointer,
triggering the refcount_t warning and subsequent fault. In short, it’s
not a missing cached_dev_put(), but a second kthread_stop() against an
already exited thread.


2) Could you please point out all the locations where writeback_wq is stopped?
If by “stopped” you mean flush/destroy of the per-device writeback
workqueue (dc->writeback_write_wq), there is only one place:

At the tail of bch_writeback_thread():

if (dc->writeback_write_wq) {
    flush_workqueue(dc->writeback_write_wq);
    destroy_workqueue(dc->writeback_write_wq);
}

Other sites stop the thread (see next paragraph), but do not
flush/destroy the workqueue.

For completeness, there are exactly three call sites that may call
kthread_stop(dc->writeback_thread):

cached_dev_detach_finish()

bch_cached_dev_attach() error path when bch_cached_dev_run() fails
(and error != -EBUSY)

cached_dev_free()

Note: bch_writeback_thread() does not call kthread_stop() on itself;
it only tears down its WQ, then cached_dev_put(dc) and
wait_for_kthread_stop().


The per-device writeback workqueue is flushed/destroyed at one place:

1、bch_writeback_thread() tail — it does not call kthread_stop(); it
only tears down dc->writeback_write_wq (flush + destroy), then
cached_dev_put(dc) and
 wait_for_kthread_stop().

3）how the panic comes in code logic

I/O errors -> bch_cache_set_error()
   -> set_bit(CACHE_SET_IO_DISABLE)
   -> conditional_stop_bcache_device()
        -> bcache_device_stop() -> cached_dev_flush()
             -> continue_at(..., cached_dev_free, system_wq)
                   -> cached_dev_free():
kthread_stop(writeback_thread) [Thread stop site #3]

Meanwhile writeback thread observes IO_DISABLE:
   bch_writeback_thread():
      exit path:
        - flush/destroy dc->writeback_write_wq (single owner)
        - cached_dev_put(dc)
        - wait_for_kthread_stop()

If last ref:
   queue detach once:
      cached_dev_detach_finish():
         - cancel writeback-rate dwork
         - kthread_stop(writeback_thread) if still present [Thread stop site #1]
         - bcache_device_detach(), list moves, clear flags, closure_put()

Attach error path:
   bch_cached_dev_attach():
      if bch_cached_dev_run() fails && err != -EBUSY:
         - kthread_stop(writeback_thread) [Thread stop site #2]
         - cancel writeback-rate dwork

I/O errors (e.g., NVMe remove/journal error)
   |
   v
bch_cache_set_error()
   |-- set_bit(CACHE_SET_IO_DISABLE, &c->flags)
   |-- conditional_stop_bcache_device()
          |
          v
       bcache_device_stop()
          |
          v
       cached_dev_flush()
          |
          v
       (system "events" WQ) -> cached_dev_free(dc)
                                |
                                +-- kthread_stop(dc->writeback_thread)   [A]
                                +-- kthread_stop(dc->status_update_thread)
                                '-- cancel writeback-rate dwork, unlink, free...

Meanwhile:
bch_writeback_thread(dc) sees IO_DISABLE or should_stop and exits:
   |
   +-- flush/destroy dc->writeback_write_wq      [sole WQ teardown site]
   +-- cached_dev_put(dc)
   '-- wait_for_kthread_stop()

Race:
- One path already ended the thread and freed its task_struct.
- A second path still calls kthread_stop(dc->writeback_thread) on the
stale pointer
  -> refcount splat / GPF (what we observe).

Thanks again, and apologies for the earlier incomplete answer.

Best regards,
cheliequan

liequan che <[email protected]> 于2025年11月13日周四 19:38写道：
>
> stop writeback thread and rate-update work exactly once across teardown paths,
> - Add STOP_THREAD_ONCE() and use it at all three places that stop
>   dc->writeback_thread: cached_dev_detach_finish(), cached_dev_free(),
>   and the bch_cached_dev_attach() error path.
> - In cached_dev_detach_finish(), also clear WB_RUNNING and cancel the
>   periodic writeback-rate delayed work to avoid a UAF window after
>   detach is initiated.
> - Keep the per-dc writeback workqueue flush/destroy in the writeback
>   thread exit tail, avoiding double-destroy.
> Signed-off-by: cheliequan <[email protected]>
> ---
>  drivers/md/bcache/bcache.h    | 11 +++++++++++
>  drivers/md/bcache/super.c     | 14 ++++++--------
>  drivers/md/bcache/writeback.c |  7 +++++--
>  3 files changed, 22 insertions(+), 10 deletions(-)
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 1d33e40d26ea..66dc5dca5c20 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -961,6 +961,17 @@ static inline void wait_for_kthread_stop(void)
>         }
>  }
>
> +/*
> + * Stop a kthread exactly once by taking ownership of the pointer.
> + * Safe against concurrent callers and against already-stopped threads.
> + */
> +#define STOP_THREAD_ONCE(dc, member)                                    \
> +       do {                                                             \
> +               struct task_struct *t__ = xchg(&(dc)->member, NULL);     \
> +               if (t__ && !IS_ERR(t__))                                 \
> +               kthread_stop(t__);                                       \
> +       } while (0)
> +
>  /* Forward declarations */
>
>  void bch_count_backing_io_errors(struct cached_dev *dc, struct bio *bio);
> diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> index 1492c8552255..b4da0a505d4a 100644
> --- a/drivers/md/bcache/super.c
> +++ b/drivers/md/bcache/super.c
> @@ -1143,8 +1143,7 @@ static void cached_dev_detach_finish(struct
> work_struct *w)
>                 cancel_writeback_rate_update_dwork(dc);
>
>         if (!IS_ERR_OR_NULL(dc->writeback_thread)) {
> -               kthread_stop(dc->writeback_thread);
> -               dc->writeback_thread = NULL;
> +               STOP_THREAD_ONCE(dc, writeback_thread);
>         }
>
>         mutex_lock(&bch_register_lock);
> @@ -1308,8 +1307,9 @@ int bch_cached_dev_attach(struct cached_dev *dc,
> struct cache_set *c,
>                  * created previously in bch_cached_dev_writeback_start()
>                  * have to be stopped manually here.
>                  */
> -               kthread_stop(dc->writeback_thread);
> -               cancel_writeback_rate_update_dwork(dc);
> +               if (test_and_clear_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags))
> +                       cancel_writeback_rate_update_dwork(dc);
> +               STOP_THREAD_ONCE(dc, writeback_thread);
>                 pr_err("Couldn't run cached device %pg\n", dc->bdev);
>                 return ret;
>         }
> @@ -1349,10 +1349,8 @@ static CLOSURE_CALLBACK(cached_dev_free)
>         if (test_and_clear_bit(BCACHE_DEV_WB_RUNNING, &dc->disk.flags))
>                 cancel_writeback_rate_update_dwork(dc);
>
> -       if (!IS_ERR_OR_NULL(dc->writeback_thread))
> -               kthread_stop(dc->writeback_thread);
> -       if (!IS_ERR_OR_NULL(dc->status_update_thread))
> -               kthread_stop(dc->status_update_thread);
> +       STOP_THREAD_ONCE(dc, writeback_thread);
> +       STOP_THREAD_ONCE(dc, status_update_thread);
>
>         mutex_lock(&bch_register_lock);
>
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index 302e75f1fc4b..50e67a784acd 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -741,6 +741,7 @@ static int bch_writeback_thread(void *arg)
>         struct cached_dev *dc = arg;
>         struct cache_set *c = dc->disk.c;
>         bool searched_full_index;
> +       struct workqueue_struct *wq = NULL;
>
>         bch_ratelimit_reset(&dc->writeback_rate);
>
> @@ -832,8 +833,10 @@ static int bch_writeback_thread(void *arg)
>                 }
>         }
>
> -       if (dc->writeback_write_wq)
> -               destroy_workqueue(dc->writeback_write_wq);
> +       wq = xchg(&dc->writeback_write_wq, NULL);
> +       if (wq) {
> +           destroy_workqueue(wq);
> +        }
>
>         cached_dev_put(dc);
>         wait_for_kthread_stop();
> --
> 2.25.1