[RFC PATCH v1 0/8] small bugfixes and code improvements for zram

[RFC PATCH v1 0/8] small bugfixes and code improvements for zram

[Jiang Liu]

This patchset is to fix issues in zram found by code inspection.
There is still one more issue left: should we repalce zram_stat64_xxx()
with atomic64_xxx()?

Jiang Liu (8):
  zram: simplify and optimize zram_to_dev()
  zram: avoid invalid memory access in zram_exit()
  zram: use zram->lock to protect zram_free_page() in swap free notify
    path
  zram: destroy all devices on error recovery path in zram_init()
  zram: avoid double free in error recovery path of zram_bvec_write()
  zram: avoid access beyond the zram device
  zram: optimize memory operations with clear_page()/copy_page()
  zram: protect sysfs handler from invalid memory access

 drivers/staging/zram/zram_drv.c   | 41 +++++++++++++++++++++++++--------------
 drivers/staging/zram/zram_sysfs.c | 15 ++++----------
 2 files changed, 30 insertions(+), 26 deletions(-)

-- 
1.8.1.2

[RFC PATCH v1 1/8] zram: simplify and optimize zram_to_dev()

[Jiang Liu]

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/staging/zram/zram_sysfs.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
index e6a929d..8cb7822 100644
--- a/drivers/staging/zram/zram_sysfs.c
+++ b/drivers/staging/zram/zram_sysfs.c
@@ -30,18 +30,9 @@ static u64 zram_stat64_read(struct zram *zram, u64 *v)
 	return val;
 }
 
-static struct zram *dev_to_zram(struct device *dev)
+static inline struct zram *dev_to_zram(struct device *dev)
 {
-	int i;
-	struct zram *zram = NULL;
-
-	for (i = 0; i < zram_get_num_devices(); i++) {
-		zram = &zram_devices[i];
-		if (disk_to_dev(zram->disk) == dev)
-			break;
-	}
-
-	return zram;
+	return (struct zram *)dev_to_disk(dev)->private_data;
 }
 
 static ssize_t disksize_show(struct device *dev,
-- 
1.8.1.2

[RFC PATCH v1 2/8] 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.

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

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 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

[RFC PATCH v1 3/8] zram: use zram->lock to protect zram_free_page() in swap free notify path

[Jiang Liu]

zram_free_page() is protected by down_write(&zram->lock) when called by
zram_bvec_write(), but there's no such protection when called by
zram_slot_free_notify(), which may cause wrong states to zram object.

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

[RFC PATCH v1 4/8] 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.

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 0738f6c..3d90344 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -712,8 +712,8 @@ static int __init zram_init(void)
 	return 0;
 
 free_devices:
-	while (dev_id)
-		destroy_device(&zram_devices[--dev_id]);
+	while (dev_id >= 0)
+		destroy_device(&zram_devices[dev_id--]);
 	kfree(zram_devices);
 unregister:
 	unregister_blkdev(zram_major, "zram");
-- 
1.8.1.2

[RFC PATCH v1 5/8] zram: avoid double free in error recovery path of zram_bvec_write()

[Jiang Liu]

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 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 3d90344..66cf28a 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

[RFC PATCH v1 6/8] zram: avoid access beyond the zram device

[Jiang Liu]

Function valid_io_request() should verify the entire request doesn't
exceed the zram device, otherwise it will cause invalid memory access.

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 66cf28a..64b51b9 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -428,6 +428,10 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio)
 		return 0;
 	}
 
+	if (unlikely((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >=
+		     zram->disksize))
+		return 0;
+
 	/* I/O request is valid */
 	return 1;
 }
-- 
1.8.1.2

[RFC PATCH v1 7/8] zram: optimize memory operations with clear_page()/copy_page()

[Jiang Liu]

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 drivers/staging/zram/zram_drv.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 64b51b9..b079af5 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -128,23 +128,26 @@ static void zram_free_page(struct zram *zram, size_t index)
 	meta->table[index].size = 0;
 }
 
+static inline int is_partial_io(struct bio_vec *bvec)
+{
+	return bvec->bv_len != PAGE_SIZE;
+}
+
 static void handle_zero_page(struct bio_vec *bvec)
 {
 	struct page *page = bvec->bv_page;
 	void *user_mem;
 
 	user_mem = kmap_atomic(page);
-	memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
+	if (is_partial_io(bvec))
+		memset(user_mem + bvec->bv_offset, 0, bvec->bv_len);
+	else
+		clear_page(user_mem);
 	kunmap_atomic(user_mem);
 
 	flush_dcache_page(page);
 }
 
-static inline int is_partial_io(struct bio_vec *bvec)
-{
-	return bvec->bv_len != PAGE_SIZE;
-}
-
 static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 {
 	int ret = LZO_E_OK;
@@ -154,13 +157,13 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 	unsigned long handle = meta->table[index].handle;
 
 	if (!handle || zram_test_flag(meta, index, ZRAM_ZERO)) {
-		memset(mem, 0, PAGE_SIZE);
+		clear_page(mem);
 		return 0;
 	}
 
 	cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_RO);
 	if (meta->table[index].size == PAGE_SIZE)
-		memcpy(mem, cmem, PAGE_SIZE);
+		copy_page(mem, cmem);
 	else
 		ret = lzo1x_decompress_safe(cmem, meta->table[index].size,
 						mem, &clen);
@@ -309,11 +312,13 @@ static int zram_bvec_write(struct zram *zram, struct bio_vec *bvec, u32 index,
 	}
 	cmem = zs_map_object(meta->mem_pool, handle, ZS_MM_WO);
 
-	if ((clen == PAGE_SIZE) && !is_partial_io(bvec))
+	if ((clen == PAGE_SIZE) && !is_partial_io(bvec)) {
 		src = kmap_atomic(page);
-	memcpy(cmem, src, clen);
-	if ((clen == PAGE_SIZE) && !is_partial_io(bvec))
+		copy_page(cmem, src);
 		kunmap_atomic(src);
+	} else {
+		memcpy(cmem, src, clen);
+	}
 
 	zs_unmap_object(meta->mem_pool, handle);
 
-- 
1.8.1.2

[RFC PATCH v1 8/8] 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().

Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
 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 8cb7822..e239d94 100644
--- a/drivers/staging/zram/zram_sysfs.c
+++ b/drivers/staging/zram/zram_sysfs.c
@@ -179,8 +179,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: [RFC PATCH v1 4/8] zram: destroy all devices on error recovery path in zram_init()

[Dan Carpenter]

Everyone stop putting RFC on their bugfixes!  :P  No one wants to
pre-review patches.

On Mon, Jun 03, 2013 at 11:42:16PM +0800, Jiang Liu wrote:
> On error recovery path of zram_init(), it leaks the zram device object
> causing the failure.
> 

This is a real bug but the fix isn't right.  The object causing the
failure has only been partially set up.  And in some cases it has
been partially cleaned up, for example we could end up releasing
->queue twice.

The better way is to make create_device() clean up after itself
completely instead of only partly and sometimes.

regards,
dan carpenter

Re: [RFC PATCH v1 0/8] small bugfixes and code improvements for zram

[Minchan Kim]

Hello,

On Mon, Jun 03, 2013 at 11:42:12PM +0800, Jiang Liu wrote:
> This patchset is to fix issues in zram found by code inspection.
> There is still one more issue left: should we repalce zram_stat64_xxx()
> with atomic64_xxx()?
> 
> Jiang Liu (8):
>   zram: simplify and optimize zram_to_dev()
>   zram: avoid invalid memory access in zram_exit()
>   zram: use zram->lock to protect zram_free_page() in swap free notify
>     path
>   zram: destroy all devices on error recovery path in zram_init()
>   zram: avoid double free in error recovery path of zram_bvec_write()
>   zram: avoid access beyond the zram device
>   zram: optimize memory operations with clear_page()/copy_page()
>   zram: protect sysfs handler from invalid memory access

I reviewed your patchset roughly and I feel most of patches make sense
to me but some of that isn't not sure because you didn't write up the
possible scenario, expecially "zram: use zram->lock to protect zram_free_page()
in swap free notify path".

If your patchset fix real problem, it should go to the stable tree so
you need to write description up in detail.

So, please rewrite up description on all of patches and resend.
Thanks!
-- 
Kind regards,
Minchan Kim

Re: [RFC PATCH v1 2/8] zram: avoid invalid memory access in zram_exit()

[Minchan Kim]

On Mon, Jun 03, 2013 at 11:42:14PM +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.
> 
> Fix it by holding an extra reference to zram->disk before calling
> destroy_device(zram).
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  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);

Can't we simple reverse calling order of above two functions?

        zram_reset_device(zram);
        destroy_device(zram);

-- 
Kind regards,
Minchan Kim

Re: [RFC PATCH v1 1/8] zram: simplify and optimize zram_to_dev()

[Jerome Marchand]

Hi,

Please write a commit message, no matter how straightforward a patch may
seem to you.
Also the subject suffers from dyslexia: it's dev_to_zram, not zram_to_dev.

Thanks,
Jerome

On 06/03/2013 05:42 PM, Jiang Liu wrote:
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  drivers/staging/zram/zram_sysfs.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
> index e6a929d..8cb7822 100644
> --- a/drivers/staging/zram/zram_sysfs.c
> +++ b/drivers/staging/zram/zram_sysfs.c
> @@ -30,18 +30,9 @@ static u64 zram_stat64_read(struct zram *zram, u64 *v)
>  	return val;
>  }
>  
> -static struct zram *dev_to_zram(struct device *dev)
> +static inline struct zram *dev_to_zram(struct device *dev)
>  {
> -	int i;
> -	struct zram *zram = NULL;
> -
> -	for (i = 0; i < zram_get_num_devices(); i++) {
> -		zram = &zram_devices[i];
> -		if (disk_to_dev(zram->disk) == dev)
> -			break;
> -	}
> -
> -	return zram;
> +	return (struct zram *)dev_to_disk(dev)->private_data;
>  }
>  
>  static ssize_t disksize_show(struct device *dev,
> 

Re: [RFC PATCH v1 6/8] zram: avoid access beyond the zram device

[Jerome Marchand]

On 06/03/2013 05:42 PM, Jiang Liu wrote:
> Function valid_io_request() should verify the entire request doesn't
> exceed the zram device, otherwise it will cause invalid memory access.
> 
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  drivers/staging/zram/zram_drv.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 66cf28a..64b51b9 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -428,6 +428,10 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio)
>  		return 0;
>  	}
>  
> +	if (unlikely((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >=
> +		     zram->disksize))
> +		return 0;
> +

This test make the first line of previous test redundant. Why not just
update it like the following:

-		(bio->bi_sector >= (zram->disksize >> SECTOR_SHIFT)) ||
+		((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >=
+			zram->disksize)) ||


Jerome

>  	/* I/O request is valid */
>  	return 1;
>  }
> 

Re: [RFC PATCH v1 5/8] zram: avoid double free in error recovery path of zram_bvec_write()

[Jerome Marchand]

The patch seems right, but the title is wrong: this is not a error recovery path.
Also, the description is missing again.

Jerome

On 06/03/2013 05:42 PM, Jiang Liu wrote:
> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
> ---
>  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 3d90344..66cf28a 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;
> 

Re: [RFC PATCH v1 2/8] zram: avoid invalid memory access in zram_exit()

[Jiang Liu]

On Tue 04 Jun 2013 05:03:09 PM CST, Minchan Kim wrote:
> On Mon, Jun 03, 2013 at 11:42:14PM +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.
>>
>> Fix it by holding an extra reference to zram->disk before calling
>> destroy_device(zram).
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  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);
>
> Can't we simple reverse calling order of above two functions?
>
>         zram_reset_device(zram);
>         destroy_device(zram);
>
Hi Minchan,
     We can't solve this bug by changing the order of the two functions.
If we change the order, it will cause corner cases to zram sysfs 
handler,
which will be hard to solve too.
Regards!
Gerry

Re: [RFC PATCH v1 1/8] zram: simplify and optimize zram_to_dev()

[Jiang Liu]

On Tue 04 Jun 2013 09:09:05 PM CST, Jerome Marchand wrote:
> Hi,
>
> Please write a commit message, no matter how straightforward a patch may
> seem to you.
> Also the subject suffers from dyslexia: it's dev_to_zram, not zram_to_dev.
>
> Thanks,
> Jerome
Hi Jerome,
     Thanks for review, will fix it in next version.
Regards!
Gerry

>
> On 06/03/2013 05:42 PM, Jiang Liu wrote:
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  drivers/staging/zram/zram_sysfs.c | 13 ++-----------
>>  1 file changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/zram/zram_sysfs.c b/drivers/staging/zram/zram_sysfs.c
>> index e6a929d..8cb7822 100644
>> --- a/drivers/staging/zram/zram_sysfs.c
>> +++ b/drivers/staging/zram/zram_sysfs.c
>> @@ -30,18 +30,9 @@ static u64 zram_stat64_read(struct zram *zram, u64 *v)
>>  	return val;
>>  }
>>
>> -static struct zram *dev_to_zram(struct device *dev)
>> +static inline struct zram *dev_to_zram(struct device *dev)
>>  {
>> -	int i;
>> -	struct zram *zram = NULL;
>> -
>> -	for (i = 0; i < zram_get_num_devices(); i++) {
>> -		zram = &zram_devices[i];
>> -		if (disk_to_dev(zram->disk) == dev)
>> -			break;
>> -	}
>> -
>> -	return zram;
>> +	return (struct zram *)dev_to_disk(dev)->private_data;
>>  }
>>
>>  static ssize_t disksize_show(struct device *dev,
>>
>

Re: [RFC PATCH v1 4/8] zram: destroy all devices on error recovery path in zram_init()

[Jiang Liu]

On Tue 04 Jun 2013 04:49:16 PM CST, Dan Carpenter wrote:
> Everyone stop putting RFC on their bugfixes!  :P  No one wants to
> pre-review patches.
>
> On Mon, Jun 03, 2013 at 11:42:16PM +0800, Jiang Liu wrote:
>> On error recovery path of zram_init(), it leaks the zram device object
>> causing the failure.
>>
>
> This is a real bug but the fix isn't right.  The object causing the
> failure has only been partially set up.  And in some cases it has
> been partially cleaned up, for example we could end up releasing
> ->queue twice.
>
> The better way is to make create_device() clean up after itself
> completely instead of only partly and sometimes.
>
> regards,
> dan carpenter
>
Hi Dan,
     Thanks for your review, will fix it in next version.
Regards!
Gerry

Re: [RFC PATCH v1 6/8] zram: avoid access beyond the zram device

[Jiang Liu]

On Tue 04 Jun 2013 09:15:43 PM CST, Jerome Marchand wrote:
> On 06/03/2013 05:42 PM, Jiang Liu wrote:
>> Function valid_io_request() should verify the entire request doesn't
>> exceed the zram device, otherwise it will cause invalid memory access.
>>
>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>> ---
>>  drivers/staging/zram/zram_drv.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
>> index 66cf28a..64b51b9 100644
>> --- a/drivers/staging/zram/zram_drv.c
>> +++ b/drivers/staging/zram/zram_drv.c
>> @@ -428,6 +428,10 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio)
>>  		return 0;
>>  	}
>>
>> +	if (unlikely((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >=
>> +		     zram->disksize))
>> +		return 0;
>> +
>
> This test make the first line of previous test redundant. Why not just
> update it like the following:
>
> -		(bio->bi_sector >= (zram->disksize >> SECTOR_SHIFT)) ||
> +		((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >=
> +			zram->disksize)) ||
>
>
> Jerome
Hi Jerome,
         I think the test "bio->bi_sector >= (zram->disksize >> 
SECTOR_SHIFT)" is still
needed to protect "(bio->bi_sector << SECTOR_SHIFT) + bio->bi_size" 
from wrapping
around.
Regards!
Gerry

>
>>  	/* I/O request is valid */
>>  	return 1;
>>  }
>>
>

Re: [RFC PATCH v1 6/8] zram: avoid access beyond the zram device

[Jerome Marchand]

On 06/04/2013 05:09 PM, Jiang Liu wrote:
> On Tue 04 Jun 2013 09:15:43 PM CST, Jerome Marchand wrote:
>> On 06/03/2013 05:42 PM, Jiang Liu wrote:
>>> Function valid_io_request() should verify the entire request doesn't
>>> exceed the zram device, otherwise it will cause invalid memory access.
>>>
>>> Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
>>> ---
>>>  drivers/staging/zram/zram_drv.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
>>> index 66cf28a..64b51b9 100644
>>> --- a/drivers/staging/zram/zram_drv.c
>>> +++ b/drivers/staging/zram/zram_drv.c
>>> @@ -428,6 +428,10 @@ static inline int valid_io_request(struct zram *zram, struct bio *bio)
>>>  		return 0;
>>>  	}
>>>
>>> +	if (unlikely((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >=
>>> +		     zram->disksize))
>>> +		return 0;
>>> +
>>
>> This test make the first line of previous test redundant. Why not just
>> update it like the following:
>>
>> -		(bio->bi_sector >= (zram->disksize >> SECTOR_SHIFT)) ||
>> +		((bio->bi_sector << SECTOR_SHIFT) + bio->bi_size >=
>> +			zram->disksize)) ||
>>
>>
>> Jerome
> Hi Jerome,
>          I think the test "bio->bi_sector >= (zram->disksize >> 
> SECTOR_SHIFT)" is still
> needed to protect "(bio->bi_sector << SECTOR_SHIFT) + bio->bi_size" 
> from wrapping
> around.

Good point, but I don't see how this is going to catch all the possible
values that overflow. You still need an explicit overflow test
(bio->bi_sector << SECTOR_SHIFT) + bio->bi_size < bio->bi_size), at
which point the first test would be useless.

Jerome

> Regards!
> Gerry
> 
>>
>>>  	/* I/O request is valid */
>>>  	return 1;
>>>  }
>>>
>>
> 
>