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 <[email protected]>
---
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
On Mon, Oct 28, 2013 at 8:21 PM, Rashika Kheria
<[email protected]> 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 <[email protected]>
> ---
>
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On Mon, Oct 28, 2013 at 10:33 PM, Rashika Kheria
<[email protected]> wrote:
>
>
>
> On Mon, Oct 28, 2013 at 7:15 PM, Weijie Yang <[email protected]>
> wrote:
>>
>> On Mon, Oct 28, 2013 at 8:21 PM, Rashika Kheria
>> <[email protected]> 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 <[email protected]>
>> > ---
>> >
>> > 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
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 <[email protected]>
> ---
>
> 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 [email protected]
> 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