Messages in this thread Patch in this message |  | | Date | Thu, 7 May 2026 18:40:37 +0900 | | From | Sergey Senozhatsky <> | | Subject | Re: [PATCH] zram: fix use-after-free in zram_writeback_endio |
| |
On (26/05/05 09:37), Minchan Kim wrote: > > @@ -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.
->num_inflight is a ref-counter, basically. The problem is that completion is a two-step process, only one part of each is synchronized with the writeback context. I honestly don't want to have two ref-counts: one for requests pending zram completion and one for active endio contexts. Maybe we can repurpose num_inflight instead.
> Furthermore, keeping wake_up() outside the critical section minimizes > interrupt-disabled latency
So I considered that, but isn't endio already called from IRQ context? Just asking. We wakeup only one waiter (writeback task), so it's not that bad CPU-cycles wise. Do you think it's really a concern?
wake_up() under spin-lock solves the problem of a unsynchronized two-stages endio process.
> and avoids nesting spinlocks (done_lock -> done_wait.lock), reducing > the risk of future lockdep issues, just in case.
I considered lockdep as well but ruled it out as impossible scenario, nesting here is strictly uni-directional, we never call into zram from the scheduler. Just saying.
> 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.
Dunno, something like below maybe?
--- drivers/block/zram/zram_drv.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c index ce2e1c79fc75..27fe50d666d7 100644 --- a/drivers/block/zram/zram_drv.c +++ b/drivers/block/zram/zram_drv.c @@ -967,7 +967,7 @@ static int zram_writeback_complete(struct zram *zram, struct zram_wb_req *req) static void zram_writeback_endio(struct bio *bio) { struct zram_wb_req *req = container_of(bio, struct zram_wb_req, bio); - struct zram_wb_ctl *wb_ctl = bio->bi_private; + struct zram_wb_ctl *wb_ctl = READ_ONCE(bio->bi_private); unsigned long flags; spin_lock_irqsave(&wb_ctl->done_lock, flags); @@ -975,6 +975,7 @@ static void zram_writeback_endio(struct bio *bio) spin_unlock_irqrestore(&wb_ctl->done_lock, flags); wake_up(&wb_ctl->done_wait); + atomic_dec(&wb_ctl->num_inflight); } static void zram_submit_wb_request(struct zram *zram, @@ -998,7 +999,7 @@ static int zram_complete_done_reqs(struct zram *zram, unsigned long flags; int ret = 0, err; - while (atomic_read(&wb_ctl->num_inflight) > 0) { + for (;;) { spin_lock_irqsave(&wb_ctl->done_lock, flags); req = list_first_entry_or_null(&wb_ctl->done_reqs, struct zram_wb_req, entry); @@ -1006,7 +1007,6 @@ static int zram_complete_done_reqs(struct zram *zram, list_del(&req->entry); spin_unlock_irqrestore(&wb_ctl->done_lock, flags); - /* ->num_inflight > 0 doesn't mean we have done requests */ if (!req) break; @@ -1014,7 +1014,6 @@ static int zram_complete_done_reqs(struct zram *zram, if (err) ret = err; - atomic_dec(&wb_ctl->num_inflight); release_pp_slot(zram, req->pps); req->pps = NULL; @@ -1129,8 +1128,11 @@ static int zram_writeback_slots(struct zram *zram, if (req) release_wb_req(req); - while (atomic_read(&wb_ctl->num_inflight) > 0) { - wait_event(wb_ctl->done_wait, !list_empty(&wb_ctl->done_reqs)); + while (atomic_read(&wb_ctl->num_inflight) || + !list_empty(&wb_ctl->done_reqs)) { + wait_event_timeout(wb_ctl->done_wait, + !list_empty(&wb_ctl->done_reqs), + HZ); err = zram_complete_done_reqs(zram, wb_ctl); if (err) ret = err; -- 2.54.0.563.g4f69b47b94-goog
|  |