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
Signed-off-by: Jiang Liu <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
Signed-off-by: Jiang Liu <[email protected]>
---
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
Signed-off-by: Jiang Liu <[email protected]>
---
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
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 <[email protected]>
---
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
On error recovery path of zram_init(), it leaks the zram device object
causing the failure.
Signed-off-by: Jiang Liu <[email protected]>
---
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
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
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> ---
> 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,
>
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 <[email protected]>
> ---
> 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;
> }
>
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 <[email protected]>
> ---
> 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;
>
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> ---
>> 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,
>>
>
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
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 <[email protected]>
>> ---
>> 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;
>> }
>>
>
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 <[email protected]>
>>> ---
>>> 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;
>>> }
>>>
>>
>
>