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

[Sergey Senozhatsky <senozhatsky@chromium.org>]

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