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@xxxxxxxxx>
> ---
> 
> 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@xxxxxxxxxxxxxxx
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/