Re: [PATCH v4 1/2] Staging: zram: Fix variable dereferenced before check

2013-10-28 Thread Minchan Kim 
Hello Rashika,


On Fri, Oct 25, 2013 at 7:10 PM, Minchan Kim minc...@kernel.org wrote:
 Hello Rashika,

 First of all, thanks for looking this!

 On Tue, Oct 22, 2013 at 07:00:57PM +0530, Rashika Kheria wrote:
 This patch fixes the following Smatch warning in zram_drv.c-
 drivers/staging/zram/zram_drv.c:663
 reset_store() warn: variable dereferenced before check 'bdev' (see line 652)
 drivers/staging/zram/zram_drv.c:899
 destroy_device() warn: variable dereferenced before check 'zram-disk' (see 
 line 896)

 Signed-off-by: Rashika Kheria rashika.khe...@gmail.com
 ---

 This revision fixes the following issues of the previous revision:
 Remove unnecessary checks

  drivers/staging/zram/zram_drv.c |   16 +++-
  1 file changed, 7 insertions(+), 9 deletions(-)

 diff --git a/drivers/staging/zram/zram_drv.c 
 b/drivers/staging/zram/zram_drv.c
 index 2c4ed52..8679a06 100644
 --- a/drivers/staging/zram/zram_drv.c
 +++ b/drivers/staging/zram/zram_drv.c
 @@ -648,6 +648,9 @@ static ssize_t reset_store(struct device *dev,
   zram = dev_to_zram(dev);
   bdev = bdget_disk(zram-disk, 0);

 + if (!bdev)
 + return -EBUSY;
 +

 I'd like to look into that when bdget_disk could fail but I don't have a time
 so sorry. I will review it when I return back to the office in next week.


I reviewed that part and realized bdget_disk could fail when memory is
tight so you're right.

But please separate it with two stuffs.

1. fix reset_store bug caused by accessing NULL ptr
2. fix destroy_device by smatch warning.

1 is real bug fix which NULL ptr access will happen when memory is
tight although it's very rare so that IMO, it should go stable tree
while 2 is just smatch warning which will never happen in real
practice.

Thanks for looking this!

-- 
Kind regards,
Minchan Kim
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH v4 1/2] Staging: zram: Fix variable dereferenced before check

2013-10-25 Thread Minchan Kim 
Hello Rashika,

First of all, thanks for looking this!

On Tue, Oct 22, 2013 at 07:00:57PM +0530, Rashika Kheria wrote:
 This patch fixes the following Smatch warning in zram_drv.c-
 drivers/staging/zram/zram_drv.c:663
 reset_store() warn: variable dereferenced before check 'bdev' (see line 652)
 drivers/staging/zram/zram_drv.c:899
 destroy_device() warn: variable dereferenced before check 'zram-disk' (see 
 line 896)
 
 Signed-off-by: Rashika Kheria rashika.khe...@gmail.com
 ---
 
 This revision fixes the following issues of the previous revision:
 Remove unnecessary checks
 
  drivers/staging/zram/zram_drv.c |   16 +++-
  1 file changed, 7 insertions(+), 9 deletions(-)
 
 diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
 index 2c4ed52..8679a06 100644
 --- a/drivers/staging/zram/zram_drv.c
 +++ b/drivers/staging/zram/zram_drv.c
 @@ -648,6 +648,9 @@ static ssize_t reset_store(struct device *dev,
   zram = dev_to_zram(dev);
   bdev = bdget_disk(zram-disk, 0);
  
 + if (!bdev)
 + return -EBUSY;
 +

I'd like to look into that when bdget_disk could fail but I don't have a time
so sorry. I will review it when I return back to the office in next week.

Thanks!

   /* Do not reset an active device! */
   if (bdev-bd_holders)
   return -EBUSY;
 @@ -660,8 +663,7 @@ static ssize_t reset_store(struct device *dev,
   return -EINVAL;
  
   /* Make sure all pending I/O is finished */
 - if (bdev)
 - fsync_bdev(bdev);
 + fsync_bdev(bdev);
  
   zram_reset_device(zram, true);
   return len;
 @@ -895,14 +897,10 @@ static void destroy_device(struct zram *zram)
  {
   sysfs_remove_group(disk_to_dev(zram-disk)-kobj,
   zram_disk_attr_group);
 + del_gendisk(zram-disk);
 + put_disk(zram-disk);
  
 - if (zram-disk) {
 - del_gendisk(zram-disk);
 - put_disk(zram-disk);
 - }
 -
 - if (zram-queue)
 - blk_cleanup_queue(zram-queue);
 + blk_cleanup_queue(zram-queue);
  }
  
  static int __init zram_init(void)
 -- 
 1.7.9.5
 

-- 
Kind regards,
Minchan Kim
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH v4 1/2] Staging: zram: Fix variable dereferenced before check

2013-10-22 Thread Rashika Kheria 
This patch fixes the following Smatch warning in zram_drv.c-
drivers/staging/zram/zram_drv.c:663
reset_store() warn: variable dereferenced before check 'bdev' (see line 652)
drivers/staging/zram/zram_drv.c:899
destroy_device() warn: variable dereferenced before check 'zram-disk' (see 
line 896)

Signed-off-by: Rashika Kheria rashika.khe...@gmail.com
---

This revision fixes the following issues of the previous revision:
Remove unnecessary checks

 drivers/staging/zram/zram_drv.c |   16 +++-
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 2c4ed52..8679a06 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -648,6 +648,9 @@ static ssize_t reset_store(struct device *dev,
zram = dev_to_zram(dev);
bdev = bdget_disk(zram-disk, 0);
 
+   if (!bdev)
+   return -EBUSY;
+
/* Do not reset an active device! */
if (bdev-bd_holders)
return -EBUSY;
@@ -660,8 +663,7 @@ static ssize_t reset_store(struct device *dev,
return -EINVAL;
 
/* Make sure all pending I/O is finished */
-   if (bdev)
-   fsync_bdev(bdev);
+   fsync_bdev(bdev);
 
zram_reset_device(zram, true);
return len;
@@ -895,14 +897,10 @@ static void destroy_device(struct zram *zram)
 {
sysfs_remove_group(disk_to_dev(zram-disk)-kobj,
zram_disk_attr_group);
+   del_gendisk(zram-disk);
+   put_disk(zram-disk);
 
-   if (zram-disk) {
-   del_gendisk(zram-disk);
-   put_disk(zram-disk);
-   }
-
-   if (zram-queue)
-   blk_cleanup_queue(zram-queue);
+   blk_cleanup_queue(zram-queue);
 }
 
 static int __init zram_init(void)
-- 
1.7.9.5

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel