Re: [PATCH] zram: fix use-after-free in zram_writeback_endio

[Minchan Kim <minchan@kernel.org>]

On Mon, May 04, 2026 at 12:32:30PM +0000, Richard Chang wrote:
> A crash was observed in zram_writeback_endio due to a NULL pointer
> dereference in wake_up. The root cause is a race condition between the
> bio completion handler (zram_writeback_endio) and the writeback task.
> 
> In zram_writeback_endio, wake_up() is called on &wb_ctl->done_wait after
> releasing wb_ctl->done_lock. This creates a race window where the
> writeback task can see num_inflight become 0, return, and free wb_ctl
> before zram_writeback_endio calls wake_up().
> 
> CPU 0 (zram_writeback_endio)       CPU 1 (zram_complete_done_reqs)
> ============================       ============================
> spin_lock(&wb_ctl->done_lock);
> list_add(&req->entry, &wb_ctl->done_reqs);
> spin_unlock(&wb_ctl->done_lock);
>                                    while (&wb_ctl->num_inflight) > 0)
>                                    spin_lock(&wb_ctl->done_lock);
>                                    list_del(&req->entry);
>                                    spin_unlock(&wb_ctl->done_lock);
> 				   // num_inflight becomes 0
>                                    atomic_dec(&wb_ctl->num_inflight);
>                                    returns to writeback_store();
> 				   // frees wb_ctl
>                                    release_wb_ctl(wb_ctl);
> 
> // UAF crash!
> wake_up(&wb_ctl->done_wait);
> 
> Fix this by moving wake_up() inside the done_lock critical section.
> This ensures that zram_complete_done_reqs cannot consume the request
> and decrement num_inflight until zram_writeback_endio has finished
> calling wake_up() and released the lock.
> 
> Fixes: f405066a1f0d ("zram: introduce writeback bio batching")
> Signed-off-by: Richard Chang <richardycc@google.com>
> ---
>  drivers/block/zram/zram_drv.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index aebc710f0d6a..a457fdf564f8 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -966,9 +966,8 @@ static void zram_writeback_endio(struct bio *bio)
>  
>  	spin_lock_irqsave(&wb_ctl->done_lock, flags);
>  	list_add(&req->entry, &wb_ctl->done_reqs);
> -	spin_unlock_irqrestore(&wb_ctl->done_lock, flags);
> -
>  	wake_up(&wb_ctl->done_wait);
> +	spin_unlock_irqrestore(&wb_ctl->done_lock, flags);
>  }
>  

I agree this will fix the issue, but using a lock to extend the lifetime of
an object to avoid a UAF is not a good pattern. Object lifetime shared between
process and interrupt contexts should be managed explicitly using refcount.

Furthermore, keeping wake_up() outside the critical section minimizes
interrupt-disabled latency and avoids nesting spinlocks
(done_lock -> done_wait.lock), reducing the risk of future lockdep
issues, just in case.

It definitely will add more overhead for the submission/completion paths to deal
with the refcount, but I think we should go that way at the cost of runtime.