[OPW,kernel,v4,1/2] Staging: zram: Fix variable dereferenced before check
diff mbox

Message ID 02167236830ac8854c71c9f87de05938e5511b05.1382448451.git.rashika.kheria@gmail.com
State Changes Requested
Headers show

Commit Message

Rashika Oct. 22, 2013, 1:30 p.m. UTC
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.kheria@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(-)

Comments

Minchan Kim Oct. 25, 2013, 10:10 a.m. UTC | #1
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.kheria@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
>
Minchan Kim Oct. 28, 2013, 6:23 a.m. UTC | #2
Hello Rashika,


On Fri, Oct 25, 2013 at 7:10 PM, Minchan Kim <minchan@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.kheria@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!
Rashika Oct. 28, 2013, 11:03 a.m. UTC | #3
On Mon, Oct 28, 2013 at 11:53 AM, Minchan Kim <minchan@kernel.org> wrote:

> Hello Rashika,
>
>
> On Fri, Oct 25, 2013 at 7:10 PM, Minchan Kim <minchan@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.kheria@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
>

Thanks for your review.
I will resend the patch with suggested changes.


Thanks,

Patch
diff mbox

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)