[PATCH v5 1/3] Staging: zram: Fix access of NULL pointer

[PATCH v5 1/3] Staging: zram: Fix access of NULL pointer

[Rashika Kheria]

This patch fixes the bug in reset_store caused by accessing NULL pointer.
Hence, It introduces a check for bdev. It also removes unnecessary check
of bdev for fsync_bdev().

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This revision fixes the following issues of the previous revision-
Seperate patch into Bug Fix and Smatch Fix

 drivers/staging/zram/zram_drv.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 2c4ed52..d640a8f 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;
-- 
1.7.9.5

[PATCH v5 3/3] Staging: zram: Fix decrement of variable by calling bdput()

[Rashika Kheria]

As suggested by Jerome Marchand "The code in reset_store get the block device
(bdget_disk()) but it does not put it (bdput()) when it's done using it.
The usage count is therefor incremented but never decremented."

Hence, this patch introduces a call to bdput() to decrement the variable after usage. 

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---
 drivers/staging/zram/zram_drv.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 9027975..98dac15 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -664,6 +664,7 @@ static ssize_t reset_store(struct device *dev,
 
 	/* Make sure all pending I/O is finished */
 	fsync_bdev(bdev);
+	bdput(bdev);
 
 	zram_reset_device(zram, true);
 	return len;
-- 
1.7.9.5

[PATCH v5 2/3] Staging: zram: Fix variable dereferenced before check

[Rashika Kheria]

This patch fixes the following Smatch warning in zram_drv.c-
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-
Seperating patches into Bug fix and Smatch fix

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index d640a8f..9027975 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -898,13 +898,10 @@ static void destroy_device(struct zram *zram)
 	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
 			&zram_disk_attr_group);
 
-	if (zram->disk) {
-		del_gendisk(zram->disk);
-		put_disk(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

[PATCH v6 1/3] Staging: zram: Fix access of NULL pointer

[Rashika Kheria]

This patch fixes the bug in reset_store caused by accessing NULL pointer.
Hence, It introduces a check for bdev. It also removes unnecessary check
of bdev for fsync_bdev().

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This revision fixes the following issues of the previous revision-
Seperating patches into Bug fix and Smatch fix 

 drivers/staging/zram/zram_drv.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 2c4ed52..d640a8f 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;
-- 
1.7.9.5

[PATCH v6 2/3] Staging: zram: Fix variable dereferenced before check

[Rashika Kheria]

This patch fixes the following Smatch warning in zram_drv.c-
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-
Seperating patches into Bug fix and Smatch fix 

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index d640a8f..9027975 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -898,13 +898,10 @@ static void destroy_device(struct zram *zram)
 	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
 			&zram_disk_attr_group);
 
-	if (zram->disk) {
-		del_gendisk(zram->disk);
-		put_disk(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

Re: [PATCH v6 2/3] Staging: zram: Fix variable dereferenced before check

[Minchan Kim]

On Mon, Oct 28, 2013 at 05:53:40PM +0530, Rashika Kheria wrote:
> This patch fixes the following Smatch warning in zram_drv.c-
> 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>
Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

[PATCH v6 3/3] Staging: zram: Fix decrement of variable by calling bdput()

[Rashika Kheria]

As suggested by Jerome Marchand "The code in reset_store get the block device
(bdget_disk()) but it does not put it (bdput()) when it's done using it.
The usage count is therefor incremented but never decremented."

Hence, this patch introduces a call to bdput() to decrement the variable after usage.

Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---
 drivers/staging/zram/zram_drv.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 9027975..98dac15 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -664,6 +664,7 @@ static ssize_t reset_store(struct device *dev,
 
 	/* Make sure all pending I/O is finished */
 	fsync_bdev(bdev);
+	bdput(bdev);
 
 	zram_reset_device(zram, true);
 	return len;
-- 
1.7.9.5

Re: [PATCH v6 3/3] Staging: zram: Fix decrement of variable by calling bdput()

[Minchan Kim]

On Mon, Oct 28, 2013 at 05:54:39PM +0530, Rashika Kheria wrote:
> As suggested by Jerome Marchand "The code in reset_store get the block device
> (bdget_disk()) but it does not put it (bdput()) when it's done using it.
> The usage count is therefor incremented but never decremented."
> 
> Hence, this patch introduces a call to bdput() to decrement the variable after usage.
> 
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>

I think this patch is for stable, too.
So, please read my previous reply.

And please relocate this with [2/3] and do bdput in all of error cases
in reset_store.

> ---
>  drivers/staging/zram/zram_drv.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 9027975..98dac15 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -664,6 +664,7 @@ static ssize_t reset_store(struct device *dev,
>  
>  	/* Make sure all pending I/O is finished */
>  	fsync_bdev(bdev);
> +	bdput(bdev);
>  
>  	zram_reset_device(zram, true);
>  	return len;
> -- 
> 1.7.9.5
> 
> --
> 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 v6 1/3] Staging: zram: Fix access of NULL pointer

[Weijie Yang]

On Mon, Oct 28, 2013 at 8:21 PM, Rashika Kheria
<rashika.kheria@gmail.com> wrote:
> This patch fixes the bug in reset_store caused by accessing NULL pointer.
> Hence, It introduces a check for bdev. It also removes unnecessary check
> of bdev for fsync_bdev().
>
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
>
> This revision fixes the following issues of the previous revision-
> Seperating patches into Bug fix and Smatch fix
>
>  drivers/staging/zram/zram_drv.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 2c4ed52..d640a8f 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 am not sure how does it happen. Would you please make it clear to me?

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;
> --
> 1.7.9.5
>
> --
> 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 v6 1/3] Staging: zram: Fix access of NULL pointer

[Weijie Yang]

On Mon, Oct 28, 2013 at 10:33 PM, Rashika Kheria
<rashika.kheria@gmail.com> wrote:
>
>
>
> On Mon, Oct 28, 2013 at 7:15 PM, Weijie Yang <weijie.yang.kh@gmail.com>
> wrote:
>>
>> On Mon, Oct 28, 2013 at 8:21 PM, Rashika Kheria
>> <rashika.kheria@gmail.com> wrote:
>> > This patch fixes the bug in reset_store caused by accessing NULL
>> > pointer.
>> > Hence, It introduces a check for bdev. It also removes unnecessary check
>> > of bdev for fsync_bdev().
>> >
>> > Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
>> > ---
>> >
>> > This revision fixes the following issues of the previous revision-
>> > Seperating patches into Bug fix and Smatch fix
>> >
>> >  drivers/staging/zram/zram_drv.c |    6 ++++--
>> >  1 file changed, 4 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/staging/zram/zram_drv.c
>> > b/drivers/staging/zram/zram_drv.c
>> > index 2c4ed52..d640a8f 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 am not sure how does it happen. Would you please make it clear to me?
>>
>
> Hi Weijie,
>
> The bdev gets its value from bdget_disk() which could fail when memory is
> less available and hence bdev can be NULL.
>
> If this check is not introduced then we might reference NULL pointer in the
> later part of the code.

I see. Thanks!

> I hope I am clear.
>
> Thanks,
>
> --
> Rashika Kheria
> B.Tech CSE
> IIIT Hyderabad

Re: [PATCH v6 1/3] Staging: zram: Fix access of NULL pointer

[Minchan Kim]

Hello Rashika,

On Mon, Oct 28, 2013 at 05:51:38PM +0530, Rashika Kheria wrote:
> This patch fixes the bug in reset_store caused by accessing NULL pointer.
> Hence, It introduces a check for bdev. It also removes unnecessary check
> of bdev for fsync_bdev().

It's better than old but I still want it more better.
First of all, I hope patch description is more clear in case of bugfix patch
because other maintainers from stable/distro or other people could understand
well so they could judge once again whether they need to backport or not.
If maintainer judge that it's not severe, even they couldn't backport.

Patch description includes followin as, at least.

1. When this bug happens?
2. What's the result of user POV?
3. How to fix it.

You were good for 2 and 3 but 1

I can help.

If memory pressure is severe, inode could be reclaimed.
In this case, bdget_disk can return NULL because allocation of inode
in bdget could fail.

I hope you could do better massage.

In addition, this bug have been for a long time so it deserves backporting to
stable tree. In such case, you could Cc stable tree with stable mark.
Please refer Documentation/stable_kernel_rules.txt and others patches
in LKML.

After this year kernel summit, it seems to change something rule for
marking stable but I thinks it's not solid so old rule still would be
valid.

Thanks!

> 
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
> 
> This revision fixes the following issues of the previous revision-
> Seperating patches into Bug fix and Smatch fix 
> 
>  drivers/staging/zram/zram_drv.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 2c4ed52..d640a8f 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;
> -- 
> 1.7.9.5
> 
> --
> 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 v7 1/3] Staging: zram: Fix access of NULL pointer

[Rashika Kheria]

This patch fixes the bug in reset_store caused by accessing NULL pointer.

The bdev gets its value from bdget_disk() which could fail when memory
pressure is severe and hence can return NULL because allocation of
inode in bdget could fail.

Hence, this patch introduces a check for bdev to prevent reference to a
NULL pointer in the later part of the code. It also removes unnecessary
check of bdev for fsync_bdev().

Cc: stable@vger.kernel.org
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This patch fixes the following issues of the previous revision-
Better Description

 drivers/staging/zram/zram_drv.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 2c4ed52..d640a8f 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;
-- 
1.7.9.5

[PATCH v7 2/3] Staging: zram: Fix decrement of variable by calling bdput()

[Rashika Kheria]

As suggested by Jerome Marchand "The code in reset_store get the block device
(bdget_disk()) but it does not put it (bdput()) when it's done using it.
The usage count is therefor incremented but never decremented."

Hence, this patch introduces a call to bdput() to decrement the variable after usage.

Cc: stable@vger.kernel.org
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This revision fixes the following issues of the previous revision-
Handle more error cases

 drivers/staging/zram/zram_drv.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index d640a8f..592e760 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -664,6 +664,9 @@ static ssize_t reset_store(struct device *dev,
 
 	/* Make sure all pending I/O is finished */
 	fsync_bdev(bdev);
+	bdput(bdev);
+	bdput(bdev->bd_holders);
+	bdput(do_reset);
 
 	zram_reset_device(zram, true);
 	return len;
-- 
1.7.9.5

Re: [PATCH v7 2/3] Staging: zram: Fix decrement of variable by calling bdput()

[Minchan Kim]

On Wed, Oct 30, 2013 at 04:42:56AM +0530, Rashika Kheria wrote:
> As suggested by Jerome Marchand "The code in reset_store get the block device
> (bdget_disk()) but it does not put it (bdput()) when it's done using it.
> The usage count is therefor incremented but never decremented."
> 
> Hence, this patch introduces a call to bdput() to decrement the variable after usage.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
> 
> This revision fixes the following issues of the previous revision-
> Handle more error cases
> 
>  drivers/staging/zram/zram_drv.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index d640a8f..592e760 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -664,6 +664,9 @@ static ssize_t reset_store(struct device *dev,
>  
>  	/* Make sure all pending I/O is finished */
>  	fsync_bdev(bdev);
> +	bdput(bdev);
> +	bdput(bdev->bd_holders);
> +	bdput(do_reset);

I meant it.

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index e77fb6e..32f9a44 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -621,22 +621,30 @@ static ssize_t reset_store(struct device *dev,
 	bdev = bdget_disk(zram->disk, 0);
 
 	/* Do not reset an active device! */
-	if (bdev->bd_holders)
-		return -EBUSY;
+	if (bdev->bd_holders) {
+		ret = -EBUSY;
+		goto out;
+	}
 
 	ret = kstrtou16(buf, 10, &do_reset);
 	if (ret)
-		return ret;
+		goto out;
 
-	if (!do_reset)
-		return -EINVAL;
+	if (!do_reset) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/* Make sure all pending I/O is finished */
-	if (bdev)
-		fsync_bdev(bdev);
+	fsync_bdev(bdev);
+	bdput(bdev);
 
 	zram_reset_device(zram);
 	return len;
+
+out:
+	bdput(bdev);
+	return ret;
 }
 
 static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)

-- 
Kind regards,
Minchan Kim

[PATCH v7 3/3] Staging: zram: Fix variable dereferenced before check

[Rashika Kheria]

This patch fixes the following Smatch warning in zram_drv.c-
drivers/staging/zram/zram_drv.c:899
destroy_device() warn: variable dereferenced before check 'zram->disk' (see line 896)

Cc: stable@vger.kernel.org
Acked-by: Minchan Kim <minchan@kernel.org>
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---
 drivers/staging/zram/zram_drv.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 592e760..bf28d56 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -901,13 +901,10 @@ static void destroy_device(struct zram *zram)
 	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
 			&zram_disk_attr_group);
 
-	if (zram->disk) {
-		del_gendisk(zram->disk);
-		put_disk(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

Re: [PATCH v7 3/3] Staging: zram: Fix variable dereferenced before check

[Minchan Kim]

On Wed, Oct 30, 2013 at 04:45:24AM +0530, Rashika Kheria wrote:
> This patch fixes the following Smatch warning in zram_drv.c-
> drivers/staging/zram/zram_drv.c:899
> destroy_device() warn: variable dereferenced before check 'zram->disk' (see line 896)
> 
> Cc: stable@vger.kernel.org

It shouldn't be a stable stuff because it's just warning of Smatch
but there is no bug in real practice.

> Acked-by: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
>  drivers/staging/zram/zram_drv.c |    9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 592e760..bf28d56 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -901,13 +901,10 @@ static void destroy_device(struct zram *zram)
>  	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
>  			&zram_disk_attr_group);
>  
> -	if (zram->disk) {
> -		del_gendisk(zram->disk);
> -		put_disk(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
> 
> --
> 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 v7 3/3] Staging: zram: Fix variable dereferenced before check

[Jerome Marchand]

On 10/30/2013 12:15 AM, Rashika Kheria wrote:
> This patch fixes the following Smatch warning in zram_drv.c-
> drivers/staging/zram/zram_drv.c:899
> destroy_device() warn: variable dereferenced before check 'zram->disk' (see line 896)
> 
> Cc: stable@vger.kernel.org
> Acked-by: Minchan Kim <minchan@kernel.org>
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Acked-by: Jerome Marchand <jmarchan@redhat.com>

> ---
>  drivers/staging/zram/zram_drv.c |    9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 592e760..bf28d56 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -901,13 +901,10 @@ static void destroy_device(struct zram *zram)
>  	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
>  			&zram_disk_attr_group);
>  
> -	if (zram->disk) {
> -		del_gendisk(zram->disk);
> -		put_disk(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)
> 

Re: [PATCH v7 1/3] Staging: zram: Fix access of NULL pointer

[Minchan Kim]

On Wed, Oct 30, 2013 at 04:40:54AM +0530, Rashika Kheria wrote:
> This patch fixes the bug in reset_store caused by accessing NULL pointer.
> 
> The bdev gets its value from bdget_disk() which could fail when memory
> pressure is severe and hence can return NULL because allocation of
> inode in bdget could fail.
> 
> Hence, this patch introduces a check for bdev to prevent reference to a
> NULL pointer in the later part of the code. It also removes unnecessary
> check of bdev for fsync_bdev().
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Acked-by: Minchan Kim <minchan@kernel.org>

-- 
Kind regards,
Minchan Kim

Re: [PATCH v7 1/3] Staging: zram: Fix access of NULL pointer

[Minchan Kim]

On Wed, Oct 30, 2013 at 04:40:54AM +0530, Rashika Kheria wrote:
> This patch fixes the bug in reset_store caused by accessing NULL pointer.
> 
> The bdev gets its value from bdget_disk() which could fail when memory
> pressure is severe and hence can return NULL because allocation of
> inode in bdget could fail.
> 
> Hence, this patch introduces a check for bdev to prevent reference to a
> NULL pointer in the later part of the code. It also removes unnecessary
> check of bdev for fsync_bdev().
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
> 
> This patch fixes the following issues of the previous revision-
> Better Description
> 
>  drivers/staging/zram/zram_drv.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 2c4ed52..d640a8f 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;
> +

Oops, Sorry.
I don't think EBUSY is right. How about ENOMEM?

-- 
Kind regards,
Minchan Kim

Re: [PATCH v7 1/3] Staging: zram: Fix access of NULL pointer

[Jerome Marchand]

On 10/30/2013 12:10 AM, Rashika Kheria wrote:
> This patch fixes the bug in reset_store caused by accessing NULL pointer.
> 
> The bdev gets its value from bdget_disk() which could fail when memory
> pressure is severe and hence can return NULL because allocation of
> inode in bdget could fail.
> 
> Hence, this patch introduces a check for bdev to prevent reference to a
> NULL pointer in the later part of the code. It also removes unnecessary
> check of bdev for fsync_bdev().
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
Acked-by: Jerome Marchand <jmarchan@redhat.com>

> ---
> 
> This patch fixes the following issues of the previous revision-
> Better Description
> 
>  drivers/staging/zram/zram_drv.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 2c4ed52..d640a8f 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;
> 

[PATCH v8 1/3] Staging: zram: Fix access of NULL pointer

[Rashika Kheria]

This patch fixes the bug in reset_store caused by accessing NULL pointer.

The bdev gets its value from bdget_disk() which could fail when memory
pressure is severe and hence can return NULL because allocation of
inode in bdget could fail.

Hence, this patch introduces a check for bdev to prevent reference to a
NULL pointer in the later part of the code. It also removes unnecessary
check of bdev for fsync_bdev().

Cc: stable@vger.kernel.org
Acked-by: Jerome Marchand <jmarchan@redhat.com>
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

The previous revision fixes the following issues of the previous revision-
Change return error code

 drivers/staging/zram/zram_drv.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 2c4ed52..012ba15 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 -ENOMEM;
+
 	/* 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;
-- 
1.7.9.5

[PATCH v8 2/3] Staging: zram: Fix decrement of variable by calling bdput()

[Rashika Kheria]

As suggested by Jerome Marchand "The code in reset_store get the block device
(bdget_disk()) but it does not put it (bdput()) when it's done using it.
The usage count is therefore incremented but never decremented."

This patch also puts bdput() for all error cases.

Cc: stable@vger.kernel.org
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This revision fixes the following issues of the previous revision-
Proper error handling

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 012ba15..0bc2835 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -648,25 +648,36 @@ static ssize_t reset_store(struct device *dev,
 	zram = dev_to_zram(dev);
 	bdev = bdget_disk(zram->disk, 0);
 
-	if (!bdev)
-		return -ENOMEM;
+	if (!bdev) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	/* Do not reset an active device! */
-	if (bdev->bd_holders)
-		return -EBUSY;
+	if (bdev->bd_holders) {
+		ret = -EBUSY;
+		goto out;
+	}
 
 	ret = kstrtou16(buf, 10, &do_reset);
 	if (ret)
-		return ret;
+		goto out;
 
-	if (!do_reset)
-		return -EINVAL;
+	if (!do_reset) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/* Make sure all pending I/O is finished */
 	fsync_bdev(bdev);
+	bdput(bdev);
 
 	zram_reset_device(zram, true);
 	return len;
+
+out:	
+	bdput(bdev);
+	return ret;
 }
 
 static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
-- 
1.7.9.5

Re: [PATCH v8 2/3] Staging: zram: Fix decrement of variable by calling bdput()

[Weijie Yang]

Hello, Rashika

On Wed, Oct 30, 2013 at 9:10 PM, Rashika Kheria
<rashika.kheria@gmail.com> wrote:
> As suggested by Jerome Marchand "The code in reset_store get the block device
> (bdget_disk()) but it does not put it (bdput()) when it's done using it.
> The usage count is therefore incremented but never decremented."
>
> This patch also puts bdput() for all error cases.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
>
> This revision fixes the following issues of the previous revision-
> Proper error handling
>
>  drivers/staging/zram/zram_drv.c |   25 ++++++++++++++++++-------
>  1 file changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 012ba15..0bc2835 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -648,25 +648,36 @@ static ssize_t reset_store(struct device *dev,
>         zram = dev_to_zram(dev);
>         bdev = bdget_disk(zram->disk, 0);
>
> -       if (!bdev)
> -               return -ENOMEM;
> +       if (!bdev) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }

If bdev is NULL, just return -ENOMEM; DO NOT goto out;
or you will get a NULL point reference in bdput(bdev);

>         /* Do not reset an active device! */
> -       if (bdev->bd_holders)
> -               return -EBUSY;
> +       if (bdev->bd_holders) {
> +               ret = -EBUSY;
> +               goto out;
> +       }
>
>         ret = kstrtou16(buf, 10, &do_reset);
>         if (ret)
> -               return ret;
> +               goto out;
>
> -       if (!do_reset)
> -               return -EINVAL;
> +       if (!do_reset) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
>
>         /* Make sure all pending I/O is finished */
>         fsync_bdev(bdev);
> +       bdput(bdev);
>
>         zram_reset_device(zram, true);
>         return len;
> +
> +out:
> +       bdput(bdev);
> +       return ret;
>  }
>
>  static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
> --
> 1.7.9.5
>
> --
> 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 v8 3/3] Staging: zram: Fix variable dereferenced before check

[Rashika Kheria]

This patch fixes the following Smatch warning in zram_drv.c-
drivers/staging/zram/zram_drv.c:899
destroy_device() warn: variable dereferenced before check 'zram->disk' (see line 896)

Acked-by: Minchan Kim <minchan@kernel.org>
Acked-by: Jerome Marchand <jmarchan@redhat.com>
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This revision fixes the following issues of the previous revision-
Removes Cc to stable@vger.kernel.org as it is just a Smatch warning fix

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

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 0bc2835..cfa1d2e 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -909,13 +909,10 @@ static void destroy_device(struct zram *zram)
 	sysfs_remove_group(&disk_to_dev(zram->disk)->kobj,
 			&zram_disk_attr_group);
 
-	if (zram->disk) {
-		del_gendisk(zram->disk);
-		put_disk(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

Re: [PATCH v8 3/3] Staging: zram: Fix variable dereferenced before check

[Greg Kroah-Hartman]

On Wed, Oct 30, 2013 at 06:43:32PM +0530, Rashika Kheria wrote:
> This patch fixes the following Smatch warning in zram_drv.c-
> drivers/staging/zram/zram_drv.c:899
> destroy_device() warn: variable dereferenced before check 'zram->disk' (see line 896)
> 
> Acked-by: Minchan Kim <minchan@kernel.org>
> Acked-by: Jerome Marchand <jmarchan@redhat.com>
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
> 
> This revision fixes the following issues of the previous revision-
> Removes Cc to stable@vger.kernel.org as it is just a Smatch warning fix

Applied, thanks.

greg k-h

[PATCH v9 1/2] Staging: zram: Fix access of NULL pointer

[Rashika Kheria]

This patch fixes the bug in reset_store caused by accessing NULL pointer.

The bdev gets its value from bdget_disk() which could fail when memory
pressure is severe and hence can return NULL because allocation of
inode in bdget could fail.

Hence, this patch introduces a check for bdev to prevent reference to a
NULL pointer in the later part of the code. It also removes unnecessary
check of bdev for fsync_bdev().

Cc: stable@vger.kernel.org
Acked-by: Jerome Marchand <jmarchan@redhat.com> 
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---
 drivers/staging/zram/zram_drv.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 2c4ed52..012ba15 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 -ENOMEM;
+
 	/* 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;
-- 
1.7.9.5

[PATCH v9 2/2] Staging: zram: Fix decrement of variable by calling bdput()

[Rashika Kheria]

As suggested by Jerome Marchand "The code in reset_store get the block device
(bdget_disk()) but it does not put it (bdput()) when it's done using it.
The usage count is therefore incremented but never decremented."

This patch also puts bdput() for all error cases.

Cc: stable@vger.kernel.org 
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This revision fixes the following issues of the previous revision-
Proper error handling

 drivers/staging/zram/zram_drv.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 012ba15..a1f8b1f 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -652,21 +652,30 @@ static ssize_t reset_store(struct device *dev,
 		return -ENOMEM;
 
 	/* Do not reset an active device! */
-	if (bdev->bd_holders)
-		return -EBUSY;
+	if (bdev->bd_holders) {
+		ret = -EBUSY;
+		goto out;
+	}
 
 	ret = kstrtou16(buf, 10, &do_reset);
 	if (ret)
-		return ret;
+		goto out;
 
-	if (!do_reset)
-		return -EINVAL;
+	if (!do_reset) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/* Make sure all pending I/O is finished */
 	fsync_bdev(bdev);
+	bdput(bdev);
 
 	zram_reset_device(zram, true);
 	return len;
+
+out:	
+	bdput(bdev);
+	return ret;
 }
 
 static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
-- 
1.7.9.5

Re: [PATCH v9 2/2] Staging: zram: Fix decrement of variable by calling bdput()

[Minchan Kim]

I hope subject should be "Fix memory leak by refcount mismatch"

On Thu, Oct 31, 2013 at 05:28:18PM +0530, Rashika Kheria wrote:
> As suggested by Jerome Marchand "The code in reset_store get the block device
> (bdget_disk()) but it does not put it (bdput()) when it's done using it.
> The usage count is therefore incremented but never decremented."
> 
> This patch also puts bdput() for all error cases.
> 
> Cc: stable@vger.kernel.org 
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>

Other than that,
Acked-by: Minchan Kim <minchan@kernel.org>


Rashika, Thanks!

-- 
Kind regards,
Minchan Kim

Re: [PATCH v9 2/2] Staging: zram: Fix decrement of variable by calling bdput()

[Jerome Marchand]

On 10/31/2013 12:58 PM, Rashika Kheria wrote:
> As suggested by Jerome Marchand "The code in reset_store get the block device
The credit should actually go to Minchan. He found the bug. I merely
explained it.

> (bdget_disk()) but it does not put it (bdput()) when it's done using it.
> The usage count is therefore incremented but never decremented."
> 
> This patch also puts bdput() for all error cases.
> 
> Cc: stable@vger.kernel.org 
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>

This version finally looks good. Thanks for your effort.

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

> ---
> 
> This revision fixes the following issues of the previous revision-
> Proper error handling
> 
>  drivers/staging/zram/zram_drv.c |   19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
> index 012ba15..a1f8b1f 100644
> --- a/drivers/staging/zram/zram_drv.c
> +++ b/drivers/staging/zram/zram_drv.c
> @@ -652,21 +652,30 @@ static ssize_t reset_store(struct device *dev,
>  		return -ENOMEM;
>  
>  	/* Do not reset an active device! */
> -	if (bdev->bd_holders)
> -		return -EBUSY;
> +	if (bdev->bd_holders) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
>  
>  	ret = kstrtou16(buf, 10, &do_reset);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
> -	if (!do_reset)
> -		return -EINVAL;
> +	if (!do_reset) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>  
>  	/* Make sure all pending I/O is finished */
>  	fsync_bdev(bdev);
> +	bdput(bdev);
>  
>  	zram_reset_device(zram, true);
>  	return len;
> +
> +out:	
> +	bdput(bdev);
> +	return ret;
>  }
>  
>  static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
> 

Re: [PATCH v9 2/2] Staging: zram: Fix decrement of variable by calling bdput()

[Rashika Kheria]

On Fri, Nov 1, 2013 at 2:24 PM, Jerome Marchand <jmarchan@redhat.com> wrote:
>
> On 10/31/2013 12:58 PM, Rashika Kheria wrote:
> > As suggested by Jerome Marchand "The code in reset_store get the block device
> The credit should actually go to Minchan. He found the bug. I merely
> explained it.
>
> > (bdget_disk()) but it does not put it (bdput()) when it's done using it.
> > The usage count is therefore incremented but never decremented."
> >
> > This patch also puts bdput() for all error cases.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
>
> This version finally looks good. Thanks for your effort.
>
> Acked-by: Jerome Marchand <jmarchan@redhat.com>
>
>

Hi,

Thanks Minchan and Jerome for all you support and patience.
Your guidance helped me learn more about zram and kernel coding style
in general. :)

Thanks,
-- 
Rashika Kheria
B.Tech CSE
IIIT Hyderabad

[PATCH v10] Staging: zram: Fix memory leak by refcount mismatch

[Rashika Kheria]

As suggested by Minchan Kim and Jerome Marchand "The code in reset_store
get the block device (bdget_disk()) but it does not put it (bdput()) when
it's done using it. The usage count is therefore incremented but never
decremented."

This patch also puts bdput() for all error cases.

Acked-by: Minchan Kim <minchan@kernel.org>
Acked-by: Jerome Marchand <jmarchan@redhat.com>
Cc: stable@vger.kernel.org
Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
---

This revision fixes the following issue of the previous revision-
Incorrect Subject Line

 drivers/staging/zram/zram_drv.c |   19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c
index 012ba15..a1f8b1f 100644
--- a/drivers/staging/zram/zram_drv.c
+++ b/drivers/staging/zram/zram_drv.c
@@ -652,21 +652,30 @@ static ssize_t reset_store(struct device *dev,
 		return -ENOMEM;
 
 	/* Do not reset an active device! */
-	if (bdev->bd_holders)
-		return -EBUSY;
+	if (bdev->bd_holders) {
+		ret = -EBUSY;
+		goto out;
+	}
 
 	ret = kstrtou16(buf, 10, &do_reset);
 	if (ret)
-		return ret;
+		goto out;
 
-	if (!do_reset)
-		return -EINVAL;
+	if (!do_reset) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	/* Make sure all pending I/O is finished */
 	fsync_bdev(bdev);
+	bdput(bdev);
 
 	zram_reset_device(zram, true);
 	return len;
+
+out:	
+	bdput(bdev);
+	return ret;
 }
 
 static void __zram_make_request(struct zram *zram, struct bio *bio, int rw)
-- 
1.7.9.5

Re: [PATCH v10] Staging: zram: Fix memory leak by refcount mismatch

[Greg Kroah-Hartman]

On Fri, Nov 01, 2013 at 07:36:09PM +0530, Rashika Kheria wrote:
> As suggested by Minchan Kim and Jerome Marchand "The code in reset_store
> get the block device (bdget_disk()) but it does not put it (bdput()) when
> it's done using it. The usage count is therefore incremented but never
> decremented."
> 
> This patch also puts bdput() for all error cases.
> 
> Acked-by: Minchan Kim <minchan@kernel.org>
> Acked-by: Jerome Marchand <jmarchan@redhat.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> ---
> 
> This revision fixes the following issue of the previous revision-
> Incorrect Subject Line

I'm a bit confused now, I see three different zram patches from you,
with different subjects, are they all now just in one patch, this one?

Can you just send me the outstanding zram patches that you have gotten
acks from that you want applied, as I'm lost here.

thanks,

greg "easily confused" k-h

Re: [OPW kernel] Re: [PATCH v10] Staging: zram: Fix memory leak by refcount mismatch

[Rashika Kheria]

On Sun, Nov 10, 2013 at 9:15 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Fri, Nov 01, 2013 at 07:36:09PM +0530, Rashika Kheria wrote:
>> As suggested by Minchan Kim and Jerome Marchand "The code in reset_store
>> get the block device (bdget_disk()) but it does not put it (bdput()) when
>> it's done using it. The usage count is therefore incremented but never
>> decremented."
>>
>> This patch also puts bdput() for all error cases.
>>
>> Acked-by: Minchan Kim <minchan@kernel.org>
>> Acked-by: Jerome Marchand <jmarchan@redhat.com>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
>> ---
>>
>> This revision fixes the following issue of the previous revision-
>> Incorrect Subject Line
>
> I'm a bit confused now, I see three different zram patches from you,
> with different subjects, are they all now just in one patch, this one?
>
> Can you just send me the outstanding zram patches that you have gotten
> acks from that you want applied, as I'm lost here.
>
> thanks,
>
> greg "easily confused" k-h
>

Hi Greg,

You have already applied the rest two patches for this driver. This is
the only patch which is left.

But I think you might have problem applying this because there have
been changes in previous patches later i.e you applied v8 of this
series, while, maintainers later suggested to change more and hence v9
was also introduced.

Therefore, v9's patch 1 and v10 patch is correct to be applied in the tree.

Thanks,
-- 
Rashika Kheria
B.Tech CSE
IIIT Hyderabad

Re: [OPW kernel] Re: [PATCH v10] Staging: zram: Fix memory leak by refcount mismatch

[Greg Kroah-Hartman]

On Sun, Nov 10, 2013 at 09:26:15PM +0530, Rashika Kheria wrote:
> On Sun, Nov 10, 2013 at 9:15 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Fri, Nov 01, 2013 at 07:36:09PM +0530, Rashika Kheria wrote:
> >> As suggested by Minchan Kim and Jerome Marchand "The code in reset_store
> >> get the block device (bdget_disk()) but it does not put it (bdput()) when
> >> it's done using it. The usage count is therefore incremented but never
> >> decremented."
> >>
> >> This patch also puts bdput() for all error cases.
> >>
> >> Acked-by: Minchan Kim <minchan@kernel.org>
> >> Acked-by: Jerome Marchand <jmarchan@redhat.com>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
> >> ---
> >>
> >> This revision fixes the following issue of the previous revision-
> >> Incorrect Subject Line
> >
> > I'm a bit confused now, I see three different zram patches from you,
> > with different subjects, are they all now just in one patch, this one?
> >
> > Can you just send me the outstanding zram patches that you have gotten
> > acks from that you want applied, as I'm lost here.
> >
> > thanks,
> >
> > greg "easily confused" k-h
> >
> 
> Hi Greg,
> 
> You have already applied the rest two patches for this driver. This is
> the only patch which is left.
> 
> But I think you might have problem applying this because there have
> been changes in previous patches later i.e you applied v8 of this
> series, while, maintainers later suggested to change more and hence v9
> was also introduced.
> 
> Therefore, v9's patch 1 and v10 patch is correct to be applied in the tree.

Hm, can you please resend them, as I no longer have them in my queue.

thanks,

greg k-h

Re: [OPW kernel] Re: [PATCH v10] Staging: zram: Fix memory leak by refcount mismatch

[Rashika Kheria]

On Sun, Nov 10, 2013 at 9:40 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Sun, Nov 10, 2013 at 09:26:15PM +0530, Rashika Kheria wrote:
>> On Sun, Nov 10, 2013 at 9:15 PM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Fri, Nov 01, 2013 at 07:36:09PM +0530, Rashika Kheria wrote:
>> >> As suggested by Minchan Kim and Jerome Marchand "The code in reset_store
>> >> get the block device (bdget_disk()) but it does not put it (bdput()) when
>> >> it's done using it. The usage count is therefore incremented but never
>> >> decremented."
>> >>
>> >> This patch also puts bdput() for all error cases.
>> >>
>> >> Acked-by: Minchan Kim <minchan@kernel.org>
>> >> Acked-by: Jerome Marchand <jmarchan@redhat.com>
>> >> Cc: stable@vger.kernel.org
>> >> Signed-off-by: Rashika Kheria <rashika.kheria@gmail.com>
>> >> ---
>> >>
>> >> This revision fixes the following issue of the previous revision-
>> >> Incorrect Subject Line
>> >
>> > I'm a bit confused now, I see three different zram patches from you,
>> > with different subjects, are they all now just in one patch, this one?
>> >
>> > Can you just send me the outstanding zram patches that you have gotten
>> > acks from that you want applied, as I'm lost here.
>> >
>> > thanks,
>> >
>> > greg "easily confused" k-h
>> >
>>
>> Hi Greg,
>>
>> You have already applied the rest two patches for this driver. This is
>> the only patch which is left.
>>
>> But I think you might have problem applying this because there have
>> been changes in previous patches later i.e you applied v8 of this
>> series, while, maintainers later suggested to change more and hence v9
>> was also introduced.
>>
>> Therefore, v9's patch 1 and v10 patch is correct to be applied in the tree.
>
> Hm, can you please resend them, as I no longer have them in my queue.
>
> thanks,
>
> greg k-h
>

Resent.

Thanks,
-- 
Rashika Kheria
B.Tech CSE
IIIT Hyderabad