On 10/22/2013 02:30 PM, Rashika Kheria wrote: > > > > On Mon, Oct 21, 2013 at 8:01 PM, Minchan Kim <minchan@xxxxxxxxxx <mailto:minchan@xxxxxxxxxx>> wrote: > > Hello, > > On Mon, Oct 21, 2013 at 02:52:41PM +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@xxxxxxxxx <mailto:rashika.kheria@xxxxxxxxx>> > > --- > > > > This revision fixes the following issues of the previous revision- > > Not included null check > > > > drivers/staging/zram/zram_drv.c | 11 ++++++----- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/staging/zram/zram_drv.c b/drivers/staging/zram/zram_drv.c > > index 2c4ed52..5594d5b 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'm not an expert on sysfs and block so it's hard to understand > when we could see NULL bdev in reset handler. > I hope others could answer it. > > Another thing, when I review the code, I found it has a bug. > reset_store doesn't put refcount by getting one by bdget_disk. > It should be fixed, I think. > > > /* 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; > > @@ -893,10 +895,9 @@ out: > > > > static void destroy_device(struct zram *zram) > > { > > - sysfs_remove_group(&disk_to_dev(zram->disk)->kobj, > > - &zram_disk_attr_group); > > - > > if (zram->disk) { > > + sysfs_remove_group(&disk_to_dev(zram->disk)->kobj, > > + &zram_disk_attr_group); > > Is it really necessary to check zram->disk and zram->queue in this function? > As I see code roughly, it seems to be not necessary but need double check. > If so, please remove the check code. > > > > > del_gendisk(zram->disk); > > put_disk(zram->disk); > > } > > -- > > 1.7.9.5 > > > > -- > Kind regards, > Minchan Kim > > > Hi Minchan, > > Thanks for the review. > > I am not able to understand the bug that you are pointing to. I would be grateful if you can explain me the problem. 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. > > Thanks, > > -- > Rashika Kheria > B.Tech CSE > IIIT Hyderabad _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
