* [PATCH] loop: drop caches if offset is changed
@ 2018-12-14 20:32 Jaegeuk Kim
2018-12-17 19:42 ` [PATCH v2] loop: drop caches if offset or block_size are changed Jaegeuk Kim
0 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2018-12-14 20:32 UTC (permalink / raw)
To: linux-kernel; +Cc: Jaegeuk Kim, Jens Axboe, linux-block
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 <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
Reported-by: Martijn Coenen <maco@google.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] loop: drop caches if offset or block_size are changed
2018-12-14 20:32 [PATCH] loop: drop caches if offset is changed Jaegeuk Kim
@ 2018-12-17 19:42 ` Jaegeuk Kim
2018-12-18 17:48 ` Jens Axboe
0 siblings, 1 reply; 6+ messages in thread
From: Jaegeuk Kim @ 2018-12-17 19:42 UTC (permalink / raw)
To: linux-kernel; +Cc: Jens Axboe, linux-block
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 <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
Reported-by: Martijn Coenen <maco@google.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] loop: drop caches if offset or block_size are changed
2018-12-17 19:42 ` [PATCH v2] loop: drop caches if offset or block_size are changed Jaegeuk Kim
@ 2018-12-18 17:48 ` Jens Axboe
2018-12-18 22:41 ` [PATCH v3] " Jaegeuk Kim
0 siblings, 1 reply; 6+ messages in thread
From: Jens Axboe @ 2018-12-18 17:48 UTC (permalink / raw)
To: Jaegeuk Kim, linux-kernel; +Cc: linux-block
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 <axboe@kernel.dk>
> Cc: linux-block@vger.kernel.org
> Reported-by: Martijn Coenen <maco@google.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] loop: drop caches if offset or block_size are changed
2018-12-18 17:48 ` Jens Axboe
@ 2018-12-18 22:41 ` Jaegeuk Kim
2019-01-09 5:22 ` Jaegeuk Kim
2019-01-09 20:51 ` Bart Van Assche
0 siblings, 2 replies; 6+ messages in thread
From: Jaegeuk Kim @ 2018-12-18 22:41 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-kernel, linux-block
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 <axboe@kernel.dk>
Cc: linux-block@vger.kernel.org
Reported-by: Martijn Coenen <maco@google.com>
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3] loop: drop caches if offset or block_size are changed
2018-12-18 22:41 ` [PATCH v3] " Jaegeuk Kim
@ 2019-01-09 5:22 ` Jaegeuk Kim
2019-01-09 20:51 ` Bart Van Assche
1 sibling, 0 replies; 6+ messages in thread
From: Jaegeuk Kim @ 2019-01-09 5:22 UTC (permalink / raw)
To: Jens Axboe; +Cc: linux-kernel, linux-block
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 <axboe@kernel.dk>
> Cc: linux-block@vger.kernel.org
> Reported-by: Martijn Coenen <maco@google.com>
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] loop: drop caches if offset or block_size are changed
2018-12-18 22:41 ` [PATCH v3] " Jaegeuk Kim
2019-01-09 5:22 ` Jaegeuk Kim
@ 2019-01-09 20:51 ` Bart Van Assche
1 sibling, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2019-01-09 20:51 UTC (permalink / raw)
To: Jaegeuk Kim, Jens Axboe; +Cc: linux-kernel, linux-block
On Tue, 2018-12-18 at 14:41 -0800, Jaegeuk Kim wrote:
> [ ... ]
Please post new versions of a patch as a new e-mail thread instead of
as a reply to a previous e-mail.
> [ ... ]
>
> 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.
> 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 <bvanassche@acm.org>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-09 20:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-14 20:32 [PATCH] loop: drop caches if offset is changed Jaegeuk Kim
2018-12-17 19:42 ` [PATCH v2] loop: drop caches if offset or block_size are changed Jaegeuk Kim
2018-12-18 17:48 ` Jens Axboe
2018-12-18 22:41 ` [PATCH v3] " Jaegeuk Kim
2019-01-09 5:22 ` Jaegeuk Kim
2019-01-09 20:51 ` Bart Van Assche
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).