Messages in this thread Patch in this message |  | | Date | Fri, 8 May 2026 11:40:03 +0900 | | From | Sergey Senozhatsky <> | | Subject | Re: [PATCH] zram: fix use-after-free in zram_writeback_endio |
| |
On (26/05/07 15:56), Minchan Kim wrote: > > - 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; > > I understand why you used a timeout here, but I still don't think it's a good > idea since the user could wait for up to a second unnecessarily during the > race.
Well, sure, it doesn't have to be a full HZ, we only need to wait for propagation of atomic_dec() from another CPU. That's very fast, orders of magniter faster than a full second. Just saying.
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c > index a324ede6206d..28ab4a24e77f 100644 > --- a/drivers/block/zram/zram_drv.c > +++ b/drivers/block/zram/zram_drv.c > @@ -33,6 +33,7 @@ > #include <linux/cpuhotplug.h> > #include <linux/part_stat.h> > #include <linux/kernel_read_file.h> > +#include <linux/kref.h> > > #include "zram_drv.h" > > @@ -504,6 +505,7 @@ struct zram_wb_ctl { > wait_queue_head_t done_wait; > spinlock_t done_lock; > atomic_t num_inflight; > + struct kref kref; > };
Yeah okay, it overlaps with ->num_inflight, but we can live with that. Maybe can get rod of ->num_inflight in future patches.
[..] > @@ -864,6 +875,7 @@ static struct zram_wb_ctl *init_wb_ctl(struct zram *zram) > atomic_set(&wb_ctl->num_inflight, 0); > init_waitqueue_head(&wb_ctl->done_wait); > spin_lock_init(&wb_ctl->done_lock); > + kref_init(&wb_ctl->kref); > > for (i = 0; i < zram->wb_batch_size; i++) { > struct zram_wb_req *req; > @@ -985,6 +997,7 @@ static void zram_writeback_endio(struct bio *bio) > spin_unlock_irqrestore(&wb_ctl->done_lock, flags); > > wake_up(&wb_ctl->done_wait); > + kref_put(&wb_ctl->kref, release_wb_ctl_kref); > } > > > static void zram_submit_wb_request(struct zram *zram, > @@ -996,6 +1009,7 @@ static void zram_submit_wb_request(struct zram *zram, > * so that we don't over-submit. > */ > zram_account_writeback_submit(zram); > + kref_get(&wb_ctl->kref); > atomic_inc(&wb_ctl->num_inflight); > req->bio.bi_private = wb_ctl; > submit_bio(&req->bio); > @@ -1276,8 +1290,8 @@ static ssize_t writeback_store(struct device *dev, > > wb_ctl = init_wb_ctl(zram); > if (!wb_ctl) { > - ret = -ENOMEM; > - goto out; > + release_pp_ctl(zram, pp_ctl); > + return -ENOMEM; > } > > args = skip_spaces(buf);
So I think we also need to do kref_put(&wb_ctl->kref, release_wb_ctl_kref) at the end of writeback_store(), because otherwise it just kfree() wb_ctl and we have the same race condition:
@@ -1330,7 +1340,7 @@ static ssize_t writeback_store(struct device *dev,
out: release_pp_ctl(zram, pp_ctl); - release_wb_ctl(wb_ctl); + kref_put(&wb_ctl->kref, release_wb_ctl_kref);
return ret; }
And indirect release in init_wb_ctl() as well:
@@ -895,7 +903,7 @@ static struct zram_wb_ctl *init_wb_ctl(struct zram *zram) return wb_ctl; release_wb_ctl: - release_wb_ctl(wb_ctl); + kref_put(&wb_ctl->kref, release_wb_ctl_kref); return NULL; }
|  |