loop: drop caches if offset is changed

PATCH In review Jaegeuk Kim 3 participants 6 messages Dec 14, 2018 – Jan 9, 2019 Block layer

lore

Share & tools

lore

Jaegeuk Kim

author

2018-12-14 20:32 UTC #0 lore raw

PATCH 1/1 loop: drop caches if offset is changed

If we don't drop caches used in old offset, we can get old data from new offset, which gives unexpected data to user.

Martijn found a loopback bug in the below scenario.
1) LOOP_SET_FD loads first two pages on loop file
2) LOOP_SET_STATUS64 changes the offset on the loop file
3) mount is failed due to the cached pages having wrong superblock

This patch drops caches when we change lo->offset.

Cc Jens Axboe <[email protected]>

Cc [email protected]

Reported-by Martijn Coenen <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 drivers/block/loop.c | 6 ++++++
 1 file changed, 6 insertions(+)

drivers/block/loop.c +6 -0

index cb0cc8685076..f073a3f1a7cd 100644
@@ -1154,6 +1154,12 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 
 	if (lo->lo_offset != info->lo_offset ||
 	    lo->lo_sizelimit != info->lo_sizelimit) {
+		struct block_device *bdev = lo->lo_device;
+
+		/* drop stale caches used in old offset */
+		sync_blockdev(bdev);
+		kill_bdev(bdev);
+
 		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) {
 			err = -EFBIG;
 			goto exit;

Signature · 1 line

2.19.0.605.g01d371f741-goog

Jaegeuk Kim

author

2018-12-17 19:42 UTC #1 lore raw

Re: [PATCH v2] loop: drop caches if offset or block_size are changed

If we don't drop caches used in old offset or block_size, we can get old data from new offset/block_size, which gives unexpected data to user.

For example, Martijn found a loopback bug in the below scenario.
1) LOOP_SET_FD loads first two pages on loop file
2) LOOP_SET_STATUS64 changes the offset on the loop file
3) mount is failed due to the cached pages having wrong superblock

Cc Jens Axboe <[email protected]>

Cc [email protected]

Reported-by Martijn Coenen <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---

v1 to v2:
- cover block_size change

 drivers/block/loop.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/block/loop.c +15 -0

index cb0cc8685076..382557c81674 100644
@@ -1154,6 +1154,12 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 
 	if (lo->lo_offset != info->lo_offset ||
 	    lo->lo_sizelimit != info->lo_sizelimit) {
+		struct block_device *bdev = lo->lo_device;
+
+		/* drop stale caches used in old offset */
+		sync_blockdev(bdev);
+		kill_bdev(bdev);
+
 		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) {
 			err = -EFBIG;
 			goto exit;

@@ -1388,6 +1394,15 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
 	blk_queue_io_min(lo->lo_queue, arg);
 	loop_update_dio(lo);
 
+	/* Don't change the size if it is same as current */
+	if (lo->lo_queue->limits.logical_block_size != arg) {
+		struct block_device *bdev = lo->lo_device;
+
+		/* drop stale caches likewise set_blocksize */
+		sync_blockdev(bdev);
+		kill_bdev(bdev);
+	}
+
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
 	return 0;

Signature · 1 line

2.19.0.605.g01d371f741-goog

Jens Axboe

maintainer

2018-12-18 17:48 UTC #2 lore raw

Re: [PATCH v2] loop: drop caches if offset or block_size are changed

Quoted 43 lines · Jaegeuk Kim

If we don't drop caches used in old offset or block_size, we can get old data from new offset/block_size, which gives unexpected data to user.
For example, Martijn found a loopback bug in the below scenario.
1) LOOP_SET_FD loads first two pages on loop file
2) LOOP_SET_STATUS64 changes the offset on the loop file
3) mount is failed due to the cached pages having wrong superblock
Cc Jens Axboe <[email protected]>
Cc [email protected]
Reported-by Martijn Coenen <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
v1 to v2:
- cover block_size change
 drivers/block/loop.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
drivers/block/loop.c +8 -0
index cb0cc8685076..382557c81674 100644
@@ -1154,6 +1154,12 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 
 	if (lo->lo_offset != info->lo_offset ||
 	    lo->lo_sizelimit != info->lo_sizelimit) {
+		struct block_device *bdev = lo->lo_device;
+
+		/* drop stale caches used in old offset */
+		sync_blockdev(bdev);
+		kill_bdev(bdev);
+
 		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) {
 			err = -EFBIG;
 			goto exit;
@@ -1388,6 +1394,15 @@ static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
 	blk_queue_io_min(lo->lo_queue, arg);
 	loop_update_dio(lo);
 
+	/* Don't change the size if it is same as current */
+	if (lo->lo_queue->limits.logical_block_size != arg) {

drivers/block/loop.c static int loop_set_block_size(struct loop_device *lo, unsigned long arg) · line 1408

Quoted 10 lines

drivers/block/loop.c +7 -0

+		struct block_device *bdev = lo->lo_device;
+
+		/* drop stale caches likewise set_blocksize */
+		sync_blockdev(bdev);
+		kill_bdev(bdev);
+	}
+
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
 	return 0;

Looks fine to me, my only worry would be verifying that we're fine calling sync/kill from those contexts. The queue is frozen at this point, what happens if we do need to flush out dirty data?

Signature · 1 line

Jens Axboe

Jaegeuk Kim

author

2018-12-18 22:41 UTC #3 lore raw

Re: [PATCH v3] loop: drop caches if offset or block_size are changed

If we don't drop caches used in old offset or block_size, we can get old data from new offset/block_size, which gives unexpected data to user.

Cc Jens Axboe <[email protected]>

Cc [email protected]

Reported-by Martijn Coenen <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
v2 -> v3:
- avoid to submit IOs on frozen mq

Jens, how about this?

Thanks,

 drivers/block/loop.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

drivers/block/loop.c +27 -2

index cb0cc8685076..6b03121d62aa 100644
@@ -1126,6 +1126,12 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE)
 		return -EINVAL;
 
+	if (lo->lo_offset != info->lo_offset ||
+	    lo->lo_sizelimit != info->lo_sizelimit) {
+		sync_blockdev(lo->lo_device);
+		kill_bdev(lo->lo_device);
+	}
+
 	/* I/O need to be drained during transfer transition */
 	blk_mq_freeze_queue(lo->lo_queue);

@@ -1154,6 +1160,11 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 
 	if (lo->lo_offset != info->lo_offset ||
 	    lo->lo_sizelimit != info->lo_sizelimit) {
+		/* kill_bdev should have truncated all the pages */
+		if (lo->lo_device->bd_inode->i_mapping->nrpages) {
+			err = -EAGAIN;
+			goto exit;
+		}
 		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) {
 			err = -EFBIG;
 			goto exit;

@@ -1375,22 +1386,36 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
 
 static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
 {
+	int err = 0;
+
 	if (lo->lo_state != Lo_bound)
 		return -ENXIO;
 
 	if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
 		return -EINVAL;
 
+	if (lo->lo_queue->limits.logical_block_size != arg) {
+		sync_blockdev(lo->lo_device);
+		kill_bdev(lo->lo_device);
+	}
+
 	blk_mq_freeze_queue(lo->lo_queue);
 
+	/* kill_bdev should have truncated all the pages */
+	if (lo->lo_queue->limits.logical_block_size != arg &&
+			lo->lo_device->bd_inode->i_mapping->nrpages) {
+		err = -EAGAIN;
+		goto out;
+	}
+
 	blk_queue_logical_block_size(lo->lo_queue, arg);
 	blk_queue_physical_block_size(lo->lo_queue, arg);
 	blk_queue_io_min(lo->lo_queue, arg);
 	loop_update_dio(lo);
-
+out:
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
-	return 0;
+	return err;
 }
 
 static int lo_ioctl(struct block_device *bdev, fmode_t mode,

Signature · 1 line

2.19.0.605.g01d371f741-goog

Jaegeuk Kim

author

2019-01-09 05:22 UTC #4 lore raw

Re: [PATCH v3] loop: drop caches if offset or block_size are changed

Hi Jens,

May I ask whether this patch is acceptable?

Thanks,

Quoted 93 lines · Jaegeuk Kim

If we don't drop caches used in old offset or block_size, we can get old data from new offset/block_size, which gives unexpected data to user.

Cc Jens Axboe <[email protected]>

Cc [email protected]

Reported-by Martijn Coenen <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
v2 -> v3:
- avoid to submit IOs on frozen mq

Jens, how about this?

Thanks,

 drivers/block/loop.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

drivers/block/loop.c +27 -2

index cb0cc8685076..6b03121d62aa 100644
@@ -1126,6 +1126,12 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 	if ((unsigned int) info->lo_encrypt_key_size > LO_KEY_SIZE)
 		return -EINVAL;
 
+	if (lo->lo_offset != info->lo_offset ||
+	    lo->lo_sizelimit != info->lo_sizelimit) {
+		sync_blockdev(lo->lo_device);
+		kill_bdev(lo->lo_device);
+	}
+
 	/* I/O need to be drained during transfer transition */
 	blk_mq_freeze_queue(lo->lo_queue);

@@ -1154,6 +1160,11 @@ loop_set_status(struct loop_device *lo, const struct loop_info64 *info)
 
 	if (lo->lo_offset != info->lo_offset ||
 	    lo->lo_sizelimit != info->lo_sizelimit) {
+		/* kill_bdev should have truncated all the pages */
+		if (lo->lo_device->bd_inode->i_mapping->nrpages) {
+			err = -EAGAIN;
+			goto exit;
+		}
 		if (figure_loop_size(lo, info->lo_offset, info->lo_sizelimit)) {
 			err = -EFBIG;
 			goto exit;

@@ -1375,22 +1386,36 @@ static int loop_set_dio(struct loop_device *lo, unsigned long arg)
 
 static int loop_set_block_size(struct loop_device *lo, unsigned long arg)
 {
+	int err = 0;
+
 	if (lo->lo_state != Lo_bound)
 		return -ENXIO;
 
 	if (arg < 512 || arg > PAGE_SIZE || !is_power_of_2(arg))
 		return -EINVAL;
 
+	if (lo->lo_queue->limits.logical_block_size != arg) {
+		sync_blockdev(lo->lo_device);
+		kill_bdev(lo->lo_device);
+	}
+
 	blk_mq_freeze_queue(lo->lo_queue);
 
+	/* kill_bdev should have truncated all the pages */
+	if (lo->lo_queue->limits.logical_block_size != arg &&
+			lo->lo_device->bd_inode->i_mapping->nrpages) {
+		err = -EAGAIN;
+		goto out;
+	}
+
 	blk_queue_logical_block_size(lo->lo_queue, arg);
 	blk_queue_physical_block_size(lo->lo_queue, arg);
 	blk_queue_io_min(lo->lo_queue, arg);
 	loop_update_dio(lo);
-
+out:
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
-	return 0;
+	return err;
 }
 
 static int lo_ioctl(struct block_device *bdev, fmode_t mode,

Signature · 1 line

2.19.0.605.g01d371f741-goog

Bart Van Assche

Reviewed-by

2019-01-09 20:51 UTC #5 lore raw

Re: [PATCH v3] loop: drop caches if offset or block_size are changed

In reply to Jaegeuk Kim

Quoted 1 line · Jaegeuk Kim

[... ]

Please post new versions of a patch as a new e-mail thread instead of as a reply to a previous e-mail.

Quoted 2 lines

[... ]>> if (lo->lo_offset !=info->lo_offset ||> lo->lo_sizelimit !=info->lo_sizelimit) {>+- /*kill_bdev should have truncated all the pages */
+- if (lo->lo_device->bd_inode->i_mapping->nrpages) {>+- err =-EAGAIN;>+- goto exit;>+- }

Please add a pr_info() or pr_warn() statement here such that it becomes easy for the user to figure out why EAGAIN has been returned.

Quoted 3 lines

	blk_mq_freeze_queue(lo->lo_queue);> 
+-	/*kill_bdev should have truncated all the pages */
+-	if (lo->lo_queue->limits.logical_block_size !=arg &&>+-			lo->lo_device->bd_inode->i_mapping->nrpages) {>+-		err =-EAGAIN;>+-		goto out;>+-	}

Same comment here. Additionally, please consider renaming the "out"label into "unfreeze"or so. I think that will make the use of label names more consistent with the rest of the block layer. Once these two comments are addressed, feel free to add:

Reviewed-by Bart Van Assche <[email protected]>