LinuxLists.cc - [PATCH] loop: drop caches if offset is changed

2018-12-14 20:34:47

Subject: [PATCH] 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(+)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index cb0cc8685076..f073a3f1a7cd 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -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;
--
2.19.0.605.g01d371f741-goog

2018-12-17 20:41:01

by Jaegeuk Kim

[permalink] [raw]

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

2018-12-18 18:37:45

by Jens Axboe

[permalink] [raw]

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

On 12/17/18 12:42 PM, Jaegeuk Kim wrote:
> 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(+)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index cb0cc8685076..382557c81674 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -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;

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?

--
Jens Axboe

2018-12-18 23:37:24

by Jaegeuk Kim

[permalink] [raw]

Subject: 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.

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

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index cb0cc8685076..6b03121d62aa 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -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,
--
2.19.0.605.g01d371f741-goog

2019-01-09 05:25:17

by Jaegeuk Kim

[permalink] [raw]

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

Hi Jens,

May I ask whether this patch is acceptable?

Thanks,

On 12/18, Jaegeuk Kim wrote:
> 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]>
> ---
> 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(-)
>
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index cb0cc8685076..6b03121d62aa 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -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,
> --
> 2.19.0.605.g01d371f741-goog

2019-01-09 21:22:55

by Bart Van Assche

[permalink] [raw]

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

On Tue, 2018-12-18 at 14:41 -0800, Jaegeuk Kim wrote:
+AD4 +AFs ... +AF0

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

+AD4 +AFs ... +AF0
+AD4
+AD4 if (lo-+AD4-lo+AF8-offset +ACEAPQ info-+AD4-lo+AF8-offset +AHwAfA
+AD4 lo-+AD4-lo+AF8-sizelimit +ACEAPQ info-+AD4-lo+AF8-sizelimit) +AHs
+AD4 +- /+ACo kill+AF8-bdev should have truncated all the pages +ACo-/
+AD4 +- if (lo-+AD4-lo+AF8-device-+AD4-bd+AF8-inode-+AD4-i+AF8-mapping-+AD4-nrpages) +AHs
+AD4 +- err +AD0 -EAGAIN+ADs
+AD4 +- goto exit+ADs
+AD4 +- +AH0

Please add a pr+AF8-info() or pr+AF8-warn() statement here such that it becomes
easy for the user to figure out why EAGAIN has been returned.

+AD4 blk+AF8-mq+AF8-freeze+AF8-queue(lo-+AD4-lo+AF8-queue)+ADs
+AD4
+AD4 +- /+ACo kill+AF8-bdev should have truncated all the pages +ACo-/
+AD4 +- if (lo-+AD4-lo+AF8-queue-+AD4-limits.logical+AF8-block+AF8-size +ACEAPQ arg +ACYAJg
+AD4 +- lo-+AD4-lo+AF8-device-+AD4-bd+AF8-inode-+AD4-i+AF8-mapping-+AD4-nrpages) +AHs
+AD4 +- err +AD0 -EAGAIN+ADs
+AD4 +- goto out+ADs
+AD4 +- +AH0

Same comment here. Additionally, please consider renaming the +ACI-out+ACI label
into +ACI-unfreeze+ACI 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 +ADw-bvanassche+AEA-acm.org+AD4