[PATCH v3 01/10] zram: avoid invalid memory access in zram_exit()

[PATCH v3 01/10] zram: avoid invalid memory access in zram_exit()

[Jiang Liu]

Memory for zram->disk object may have already been freed after returning
from destroy_device(zram), then it's unsafe for zram_reset_device(zram)
to access zram->disk again.

We can't solve this bug by flipping the order of destroy_device(zram)
and zram_reset_device(zram), that will cause deadlock issues to the
zram sysfs handler.

So fix it by holding an extra reference to zram->disk before calling
destroy_device(zram).

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: stable@vger.kernel.org
---
 drivers/staging/zram/zram_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index e34e3fe..ee6b67d 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -727,8 +727,10 @@ static void __exit zram_exit(void)
 	for (i = 0; i < num_devices; i++) {
 		zram = &zram_devices[i];
 
+		get_disk(zram->disk);
 		destroy_device(zram);
 		zram_reset_device(zram);
+		put_disk(zram->disk);
 	}
 
 	unregister_blkdev(zram_major, "zram");
-- 
1.8.1.2

Re: [PATCH v3 01/10] zram: avoid invalid memory access in zram_exit()

[Minchan Kim]

Hello Jiang,

On Fri, Jun 07, 2013 at 12:07:22AM +0800, Jiang Liu wrote:
> Memory for zram->disk object may have already been freed after returning
> from destroy_device(zram), then it's unsafe for zram_reset_device(zram)
> to access zram->disk again.
> 
> We can't solve this bug by flipping the order of destroy_device(zram)
> and zram_reset_device(zram), that will cause deadlock issues to the
> zram sysfs handler.

Sorry for bothering you with description nitpick.

I agree your approach is so simple that I'd like to give Ack
but your description is not clear.

If you really want to say deadlock issue with flipping approach,
please add enough explain how the deadlock happens.(But not sure
it is worth that we should keep the problem of deadlock issue of
flipping approach in changelog)

Otherwise, it's enough with first paragraph because this bug is
very simple and plain. I prefer latter because I want that 
other developers don't waste their time to understand a deadlock issue
of flipping approach)

> 
> So fix it by holding an extra reference to zram->disk before calling
> destroy_device(zram).
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Cc: stable@vger.kernel.org

Acked-by: Minchan Kim <minchan@kernel.org>

But please rewrite the description.

-- 
Kind regards,
Minchan Kim

Re: [PATCH v3 01/10] zram: avoid invalid memory access in zram_exit()

[Jerome Marchand]

On 06/06/2013 06:07 PM, Jiang Liu wrote:
> Memory for zram->disk object may have already been freed after returning
> from destroy_device(zram), then it's unsafe for zram_reset_device(zram)
> to access zram->disk again.
> 
> We can't solve this bug by flipping the order of destroy_device(zram)
> and zram_reset_device(zram), that will cause deadlock issues to the
> zram sysfs handler.
> 
> So fix it by holding an extra reference to zram->disk before calling
> destroy_device(zram).
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Cc: stable@vger.kernel.org

Acked-by: Jerome Marchand <jmarchan@redhat.com>

[PATCH v3 02/10] zram: use zram->lock to protect zram_free_page() in swap free notify path

[Jiang Liu]

zram_slot_free_notify() is free-running without any protection from
concurrent operations. So there are race conditions between
zram_bvec_read()/zram_bvec_write() and zram_slot_free_notify(),
and possible consequences include:
1) Trigger BUG_ON(!handle) on zram_bvec_write() side.
2) Access to freed pages on zram_bvec_read() side.
3) Break some fields (bad_compress, good_compress, pages_stored)
   in zram->stats if the swap layer makes concurrently call to
   zram_slot_free_notify().

So enhance zram_slot_free_notify() to acquire writer lock on zram->lock
before calling zram_free_page().

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: stable@vger.kernel.org
---
 drivers/staging/zram/zram_drv.c | 2 ++
 drivers/staging/zram/zram_drv.h | 5 +++--
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index ee6b67d..0738f6c 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -582,7 +582,9 @@ static void zram_slot_free_notify(struct block_device *bdev,
 	struct zram *zram;
 
 	zram = bdev->bd_disk->private_data;
+	down_write(&zram->lock);
 	zram_free_page(zram, index);
+	up_write(&zram->lock);
 	zram_stat64_inc(zram, &zram->stats.notify_free);
 }
 
diff --git a/drivers/staging/zram/zram_drv.h b/drivers/staging/zram/zram_drv.h
index 2d1a3f1..d542eee 100644
--- a/drivers/staging/zram/zram_drv.h
+++ b/drivers/staging/zram/zram_drv.h
@@ -93,8 +93,9 @@ struct zram_meta {
 struct zram {
 	struct zram_meta *meta;
 	spinlock_t stat64_lock;	/* protect 64-bit stats */
-	struct rw_semaphore lock; /* protect compression buffers and table
-				   * against concurrent read and writes */
+	struct rw_semaphore lock; /* protect compression buffers, table,
+				   * 32bit stat counters against concurrent
+				   * notifications, reads and writes */
 	struct request_queue *queue;
 	struct gendisk *disk;
 	int init_done;
-- 
1.8.1.2

Re: [PATCH v3 02/10] zram: use zram->lock to protect zram_free_page() in swap free notify path

[Minchan Kim]

On Fri, Jun 07, 2013 at 12:07:23AM +0800, Jiang Liu wrote:
> zram_slot_free_notify() is free-running without any protection from
> concurrent operations. So there are race conditions between
> zram_bvec_read()/zram_bvec_write() and zram_slot_free_notify(),
> and possible consequences include:
> 1) Trigger BUG_ON(!handle) on zram_bvec_write() side.
> 2) Access to freed pages on zram_bvec_read() side.
> 3) Break some fields (bad_compress, good_compress, pages_stored)
>    in zram->stats if the swap layer makes concurrently call to
>    zram_slot_free_notify().
> 
> So enhance zram_slot_free_notify() to acquire writer lock on zram->lock
> before calling zram_free_page().
> 

If someone try to read/write *active* swap device via opening
block device file(it's not sane but we couldn't prevent it),
the race between zram_slot_free_notify and zram_bvec_[read|write] can happen.
In such case, following problem for example can happen.

1. xxx
2. xxx
3. xxx

So this patch closes the race with zram->lock write-side lock.

> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Cc: stable@vger.kernel.org

Acked-by: Minchan Kim <minchan@kernel.org>

But please rewrite the description.

-- 
Kind regards,
Minchan Kim

Re: [PATCH v3 02/10] zram: use zram->lock to protect zram_free_page() in swap free notify path

[Jerome Marchand]

On 06/06/2013 06:07 PM, Jiang Liu wrote:
> zram_slot_free_notify() is free-running without any protection from
> concurrent operations. So there are race conditions between
> zram_bvec_read()/zram_bvec_write() and zram_slot_free_notify(),
> and possible consequences include:
> 1) Trigger BUG_ON(!handle) on zram_bvec_write() side.
> 2) Access to freed pages on zram_bvec_read() side.
> 3) Break some fields (bad_compress, good_compress, pages_stored)
>    in zram->stats if the swap layer makes concurrently call to
>    zram_slot_free_notify().
> 
> So enhance zram_slot_free_notify() to acquire writer lock on zram->lock
> before calling zram_free_page().
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Cc: stable@vger.kernel.org

Acked-by: Jerome Marchand <jmarchand@redhat.com>

[PATCH v3 03/10] zram: destroy all devices on error recovery path in zram_init()

[Jiang Liu]

On error recovery path of zram_init(), it leaks the zram device object
causing the failure. So change create_device() to free allocated
resources on error path.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Acked-by: Minchan Kim <minchan@kernel.org>
Acked-by: Jerome Marchand <jmarchan@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/staging/zram/zram_drv.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 0738f6c..2ca6dc9 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -595,7 +595,7 @@ static const struct block_device_operations zram_devops = {
 
 static int create_device(struct zram *zram, int device_id)
 {
-	int ret = 0;
+	int ret = -ENOMEM;
 
 	init_rwsem(&zram->lock);
 	init_rwsem(&zram->init_lock);
@@ -605,7 +605,6 @@ static int create_device(struct zram *zram, int device_id)
 	if (!zram->queue) {
 		pr_err("Error allocating disk queue for device %d\n",
 			device_id);
-		ret = -ENOMEM;
 		goto out;
 	}
 
@@ -615,11 +614,9 @@ static int create_device(struct zram *zram, int device_id)
 	 /* gendisk structure */
 	zram->disk = alloc_disk(1);
 	if (!zram->disk) {
-		blk_cleanup_queue(zram->queue);
 		pr_warn("Error allocating disk structure for device %d\n",
 			device_id);
-		ret = -ENOMEM;
-		goto out;
+		goto out_free_queue;
 	}
 
 	zram->disk->major = zram_major;
@@ -648,11 +645,17 @@ static int create_device(struct zram *zram, int device_id)
 				&zram_disk_attr_group);
 	if (ret < 0) {
 		pr_warn("Error creating sysfs group");
-		goto out;
+		goto out_free_disk;
 	}
 
 	zram->init_done = 0;
+	return 0;
 
+out_free_disk:
+	del_gendisk(zram->disk);
+	put_disk(zram->disk);
+out_free_queue:
+	blk_cleanup_queue(zram->queue);
 out:
 	return ret;
 }
-- 
1.8.1.2

[PATCH v3 04/10] zram: avoid double free in function zram_bvec_write()

[Jiang Liu]

When doing a patial write and the whole page is filled with zero,
zram_bvec_write() will free uncmem twice.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Acked-by: Minchan Kim <minchan@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/staging/zram/zram_drv.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 2ca6dc9..27ab824 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -272,8 +272,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 
 	if (page_zero_filled(uncmem)) {
 		kunmap_atomic(user_mem);
-		if (is_partial_io(bvec))
-			kfree(uncmem);
 		zram->stats.pages_zero++;
 		zram_set_flag(meta, index, ZRAM_ZERO);
 		ret = 0;
-- 
1.8.1.2

Re: [PATCH v3 04/10] zram: avoid double free in function zram_bvec_write()

[Minchan Kim]

On Fri, Jun 07, 2013 at 12:07:25AM +0800, Jiang Liu wrote:
> When doing a patial write and the whole page is filled with zero,
               ^
               partial

> zram_bvec_write() will free uncmem twice.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Acked-by: Minchan Kim <minchan@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/staging/zram/zram_drv.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 2ca6dc9..27ab824 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -272,8 +272,6 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
>  
>  	if (page_zero_filled(uncmem)) {
>  		kunmap_atomic(user_mem);
> -		if (is_partial_io(bvec))
> -			kfree(uncmem);
>  		zram->stats.pages_zero++;
>  		zram_set_flag(meta, index, ZRAM_ZERO);
>  		ret = 0;
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Kind regards,
Minchan Kim

[PATCH v3 05/10] zram: avoid access beyond the zram device

[Jiang Liu]

Function valid_io_request() should verify the entire request are within
the zram device address range. Otherwise it may cause invalid memory
access when accessing/modifying zram->meta->table[index] because the
'index' is out of range. Then it may access non-exist memory, randomly
modify memory belong to other subsystems, which is hard to track down.

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Cc: stable@vger.kernel.org
---
 drivers/staging/zram/zram_drv.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 27ab824..9289217 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -420,13 +420,20 @@ out:
  */
 static inline int valid_io_request(struct zram *zram, struct bio *bio)
 {
-	if (unlikely(
-		(bio->bi_sector >= (zram->disksize >> SECTOR_SHIFT)) ||
-		(bio->bi_sector & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)) ||
-		(bio->bi_size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))) {
+	u64 start, end, bound;
+	
+	/* unaligned request */
+	if (unlikely(bio->bi_sector & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
+		return 0;
+	if (unlikely(bio->bi_size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
+		return 0;
 
+	start = bio->bi_sector;
+	end = start + (bio->bi_size >> SECTOR_SHIFT);
+	bound = zram->disksize >> SECTOR_SHIFT;
+	/* out of range range */
+	if (unlikely(start >= bound || end >= bound || start > end))
 		return 0;
-	}
 
 	/* I/O request is valid */
 	return 1;
-- 
1.8.1.2

Re: [PATCH v3 05/10] zram: avoid access beyond the zram device

[Minchan Kim]

On Fri, Jun 07, 2013 at 12:07:26AM +0800, Jiang Liu wrote:
> Function valid_io_request() should verify the entire request are within
> the zram device address range. Otherwise it may cause invalid memory
> access when accessing/modifying zram->meta->table[index] because the
> 'index' is out of range. Then it may access non-exist memory, randomly
> modify memory belong to other subsystems, which is hard to track down.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/staging/zram/zram_drv.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 27ab824..9289217 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -420,13 +420,20 @@ out:
>   */
>  static inline int valid_io_request(struct zram *zram, struct bio *bio)
>  {
> -	if (unlikely(
> -		(bio->bi_sector >= (zram->disksize >> SECTOR_SHIFT)) ||
> -		(bio->bi_sector & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)) ||
> -		(bio->bi_size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))) {
> +	u64 start, end, bound;
> +	
> +	/* unaligned request */
> +	if (unlikely(bio->bi_sector & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
> +		return 0;
> +	if (unlikely(bio->bi_size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
> +		return 0;
>  
> +	start = bio->bi_sector;
> +	end = start + (bio->bi_size >> SECTOR_SHIFT);
> +	bound = zram->disksize >> SECTOR_SHIFT;
> +	/* out of range range */
> +	if (unlikely(start >= bound || end >= bound || start > end))

        if (end >= bound || start > end) isn't enough?

>  		return 0;
> -	}
>  
>  	/* I/O request is valid */
>  	return 1;
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Kind regards,
Minchan Kim

Re: [PATCH v3 05/10] zram: avoid access beyond the zram device

[Jerome Marchand]

On 06/07/2013 10:09 AM, Minchan Kim wrote:
> On Fri, Jun 07, 2013 at 12:07:26AM +0800, Jiang Liu wrote:
>> Function valid_io_request() should verify the entire request are within
>> the zram device address range. Otherwise it may cause invalid memory
>> access when accessing/modifying zram->meta->table[index] because the
>> 'index' is out of range. Then it may access non-exist memory, randomly
>> modify memory belong to other subsystems, which is hard to track down.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> Cc: stable@vger.kernel.org
>> ---
>>  drivers/staging/zram/zram_drv.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
>> index 27ab824..9289217 100644
>> --- a/drivers/staging/zram/zram_drv.c
>> +++ b/drivers/staging/zram/zram_drv.c
>> @@ -420,13 +420,20 @@ out:
>>   */
>>  static inline int valid_io_request(struct zram *zram, struct bio *bio)
>>  {
>> -	if (unlikely(
>> -		(bio->bi_sector >= (zram->disksize >> SECTOR_SHIFT)) ||
>> -		(bio->bi_sector & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)) ||
>> -		(bio->bi_size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))) {
>> +	u64 start, end, bound;
>> +	
>> +	/* unaligned request */
>> +	if (unlikely(bio->bi_sector & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
>> +		return 0;
>> +	if (unlikely(bio->bi_size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
>> +		return 0;
>>  
>> +	start = bio->bi_sector;
>> +	end = start + (bio->bi_size >> SECTOR_SHIFT);
>> +	bound = zram->disksize >> SECTOR_SHIFT;
>> +	/* out of range range */
>> +	if (unlikely(start >= bound || end >= bound || start > end))
> 
>         if (end >= bound || start > end) isn't enough?

I shall think so.

Jerome

> 
>>  		return 0;
>> -	}
>>  
>>  	/* I/O request is valid */
>>  	return 1;
>> -- 
>> 1.8.1.2
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH v3 05/10] zram: avoid access beyond the zram device

[Jiang Liu]

On 06/07/2013 05:40 PM, Jerome Marchand wrote:
> On 06/07/2013 10:09 AM, Minchan Kim wrote:
>> On Fri, Jun 07, 2013 at 12:07:26AM +0800, Jiang Liu wrote:
>>> Function valid_io_request() should verify the entire request are within
...
>>>  
>>> +	start = bio->bi_sector;
>>> +	end = start + (bio->bi_size >> SECTOR_SHIFT);
>>> +	bound = zram->disksize >> SECTOR_SHIFT;
>>> +	/* out of range range */
>>> +	if (unlikely(start >= bound || end >= bound || start > end))
>>
>>         if (end >= bound || start > end) isn't enough?
> 
> I shall think so.
> 
> Jerome
Me too! But I realized this just after sending out the patchset
last night.

> 
>>
>>>  		return 0;
>>> -	}
>>>  
>>>  	/* I/O request is valid */
>>>  	return 1;
>>> -- 
>>> 1.8.1.2
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>
> 

[PATCH v3 06/10] zram: protect sysfs handler from invalid memory access

[Jiang Liu]

Use zram->init_lock to protect access to zram->meta, otherwise it
may cause invalid memory access if zram->meta has been freed by
zram_reset_device().

This issue may be triggered by:
Thread 1:
while true; do cat mem_used_total; done
Thread 2:
while true; do echo 8M > disksize; echo 1 > reset; done

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
Acked-by: Minchan Kim <minchan@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/staging/zram/zram_sysfs.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
index e6a929d..dc76a3d 100644
--- a/drivers/staging/zram/zram_sysfs.c
+++ b/drivers/staging/zram/zram_sysfs.c
@@ -188,8 +188,10 @@ static ssize_t mem_used_total_show(struct device *dev,
 	struct zram *zram = dev_to_zram(dev);
 	struct zram_meta *meta = zram->meta;
 
+	down_read(&zram->init_lock);
 	if (zram->init_done)
 		val = zs_get_total_size_bytes(meta->mem_pool);
+	up_read(&zram->init_lock);
 
 	return sprintf(buf, "%llu\n", val);
 }
-- 
1.8.1.2

Re: [PATCH v3 06/10] zram: protect sysfs handler from invalid memory access

[Jerome Marchand]

On 06/06/2013 06:07 PM, Jiang Liu wrote:
> Use zram->init_lock to protect access to zram->meta, otherwise it
> may cause invalid memory access if zram->meta has been freed by
> zram_reset_device().
> 
> This issue may be triggered by:
> Thread 1:
> while true; do cat mem_used_total; done
> Thread 2:
> while true; do echo 8M > disksize; echo 1 > reset; done
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> Acked-by: Minchan Kim <minchan@kernel.org>
> Cc: stable@vger.kernel.org

Acked-by: Jerome Marchand <jmarchan@redhat.com>