Staging: zram: Fix access of NULL pointer

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(-)

drivers/staging/zram/zram_drv.c +0 -0

index 2c4ed52..d640a8f 100644

drivers/staging/zram/zram_drv.c +1 -3

        /* 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/

Quoted 26 lines · Weijie Yang

Quoted 23 lines · 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 <[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(-)

drivers/staging/zram/zram_drv.c +0 -0

index 2c4ed52..d640a8f 100644
@@ -648,6 +648,9 @@ static ssize_t reset_store(struct device *dev,
        zram = dev_to_zram(dev);
        bdev = bdget_disk(zram->disk, 0);
 

Quoted 6 lines

drivers/staging/zram/zram_drv.c static ssize_t reset_store(struct device *dev, · line 653

Quoted 3 lines

drivers/staging/zram/zram_drv.c +3 -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 hope I am clear.

Thanks,

--
Rashika Kheria
B.Tech CSE
IIIT Hyderabad

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(-)

drivers/staging/zram/zram_drv.c +4 -2

index 2c4ed52..d640a8f 100644
@@ -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;

Signature · 7 lines

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/

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 <[email protected]>

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 <[email protected]>

---
 drivers/staging/zram/zram_drv.c |    1 +
 1 file changed, 1 insertion(+)

drivers/staging/zram/zram_drv.c +1 -0

index 9027975..98dac15 100644
@@ -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;

Signature · 7 lines

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/

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 [email protected]

Signed-off-by Rashika Kheria <[email protected]>

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 [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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(+)

drivers/staging/zram/zram_drv.c +0 -0

index d640a8f..592e760 100644

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 [email protected]

Acked-by Minchan Kim <[email protected]>

Signed-off-by Rashika Kheria <[email protected]>

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

drivers/staging/zram/zram_drv.c +3 -6

index 592e760..bf28d56 100644
@@ -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)

Signature · 7 lines

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/

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 [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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(-)

drivers/staging/zram/zram_drv.c +0 -0

index 2c4ed52..d640a8f 100644

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 [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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(-)

drivers/staging/zram/zram_drv.c +4 -2

index 2c4ed52..d640a8f 100644
@@ -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;

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 [email protected]

Acked-by Minchan Kim <[email protected]>

Signed-off-by Rashika Kheria <[email protected]>

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

drivers/staging/zram/zram_drv.c +3 -6

index 592e760..bf28d56 100644
@@ -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)

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 <[email protected]>

Acked-by Jerome Marchand <[email protected]>

Signed-off-by Rashika Kheria <[email protected]>

---

This revision fixes the following issues of the previous revision- Removes Cc to [email protected] as it is just a Smatch warning fix

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 [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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(-)

drivers/staging/zram/zram_drv.c +0 -0

index 012ba15..0bc2835 100644

drivers/staging/zram/zram_drv.c +14 -6

        /* 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

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 [email protected]

Signed-off-by Rashika Kheria <[email protected]>

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 [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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(-)

drivers/staging/zram/zram_drv.c +14 -5

index 012ba15..a1f8b1f 100644
@@ -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)

Quoted 1 line · Rashika Kheria

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.

Quoted 3 lines

(bdget_disk()) but it does not put it (bdput()) when it's done using it. The usage count is therefore incremented but never decremented."

Quoted 4 lines

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

Cc [email protected]

Signed-off-by Rashika Kheria <[email protected]>

This version finally looks good. Thanks for your effort.

Acked-by Jerome Marchand <[email protected]>

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 <[email protected]>

Acked-by Jerome Marchand <[email protected]>

Cc [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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

Quoted 7 lines · 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.

Quoted 8 lines

Acked-by Minchan Kim <[email protected]>

Acked-by Jerome Marchand <[email protected]>

Cc [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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

Quoted 20 lines · Greg Kroah-Hartman

Quoted 7 lines · 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.

Quoted 8 lines

Acked-by Minchan Kim <[email protected]>

Acked-by Jerome Marchand <[email protected]>

Cc [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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?

Quoted 7 lines

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.

Quoted 40 lines · Rashika Kheria

Quoted 20 lines · Greg Kroah-Hartman

Quoted 7 lines · 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.

Quoted 8 lines

Acked-by Minchan Kim <[email protected]>

Acked-by Jerome Marchand <[email protected]>

Cc [email protected]

Signed-off-by Rashika Kheria <[email protected]>

---

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?

Quoted 7 lines

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.

Quoted 1 line

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