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
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;
--
2.19.0.605.g01d371f741-goog
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
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
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
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