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

[Sergey Senozhatsky]

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;
 }