block/copy-before-write.c | 3 ++- block/reqlist.c | 2 -- 2 files changed, 2 insertions(+), 3 deletions(-)
Allow overlapping request by removing the assert that made it
impossible. There are only two callers:
1. block_copy_task_create()
It already asserts the very same condition before calling
reqlist_init_req().
2. cbw_snapshot_read_lock()
There is no need to have read requests be non-overlapping in
copy-before-write when used for snapshot-access. In fact, there was no
protection against two callers of cbw_snapshot_read_lock() calling
reqlist_init_req() with overlapping ranges and this could lead to an
assertion failure [1].
In particular, with the reproducer script below [0], two
cbw_co_snapshot_block_status() callers could race, with the second
calling reqlist_init_req() before the first one finishes and removes
its conflicting request.
[0]:
> #!/bin/bash -e
> dd if=/dev/urandom of=/tmp/disk.raw bs=1M count=1024
> ./qemu-img create /tmp/fleecing.raw -f raw 1G
> (
> ./qemu-system-x86_64 --qmp stdio \
> --blockdev raw,node-name=node0,file.driver=file,file.filename=/tmp/disk.raw \
> --blockdev raw,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.raw \
> <<EOF
> {"execute": "qmp_capabilities"}
> {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } }
> {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } }
> {"execute": "nbd-server-start", "arguments": {"addr": { "type": "unix", "data": { "path": "/tmp/nbd.socket" } } } }
> {"execute": "block-export-add", "arguments": {"id": "exp0", "node-name": "snap0", "type": "nbd", "name": "exp0"}}
> EOF
> ) &
> sleep 5
> while true; do
> ./qemu-nbd -d /dev/nbd0
> ./qemu-nbd -c /dev/nbd0 nbd:unix:/tmp/nbd.socket:exportname=exp0 -f raw -r
> nbdinfo --map 'nbd+unix:///exp0?socket=/tmp/nbd.socket'
> done
[1]:
> #5 0x000071e5f0088eb2 in __GI___assert_fail (...) at ./assert/assert.c:101
> #6 0x0000615285438017 in reqlist_init_req (...) at ../block/reqlist.c:23
> #7 0x00006152853e2d98 in cbw_snapshot_read_lock (...) at ../block/copy-before-write.c:237
> #8 0x00006152853e3068 in cbw_co_snapshot_block_status (...) at ../block/copy-before-write.c:304
> #9 0x00006152853f4d22 in bdrv_co_snapshot_block_status (...) at ../block/io.c:3726
> #10 0x000061528543a63e in snapshot_access_co_block_status (...) at ../block/snapshot-access.c:48
> #11 0x00006152853f1a0a in bdrv_co_do_block_status (...) at ../block/io.c:2474
> #12 0x00006152853f2016 in bdrv_co_common_block_status_above (...) at ../block/io.c:2652
> #13 0x00006152853f22cf in bdrv_co_block_status_above (...) at ../block/io.c:2732
> #14 0x00006152853d9a86 in blk_co_block_status_above (...) at ../block/block-backend.c:1473
> #15 0x000061528538da6c in blockstatus_to_extents (...) at ../nbd/server.c:2374
> #16 0x000061528538deb1 in nbd_co_send_block_status (...) at ../nbd/server.c:2481
> #17 0x000061528538f424 in nbd_handle_request (...) at ../nbd/server.c:2978
> #18 0x000061528538f906 in nbd_trip (...) at ../nbd/server.c:3121
> #19 0x00006152855a7caf in coroutine_trampoline (...) at ../util/coroutine-ucontext.c:175
Cc: qemu-stable@nongnu.org
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Changes in v2:
* different approach, allowing overlapping requests for
copy-before-write rather than waiting for them. block-copy already
asserts there are no conflicts before adding a request.
block/copy-before-write.c | 3 ++-
block/reqlist.c | 2 --
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 853e01a1eb..28f6a096cd 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -66,7 +66,8 @@ typedef struct BDRVCopyBeforeWriteState {
/*
* @frozen_read_reqs: current read requests for fleecing user in bs->file
- * node. These areas must not be rewritten by guest.
+ * node. These areas must not be rewritten by guest. There can be multiple
+ * overlapping read requests.
*/
BlockReqList frozen_read_reqs;
diff --git a/block/reqlist.c b/block/reqlist.c
index 08cb57cfa4..098e807378 100644
--- a/block/reqlist.c
+++ b/block/reqlist.c
@@ -20,8 +20,6 @@
void reqlist_init_req(BlockReqList *reqs, BlockReq *req, int64_t offset,
int64_t bytes)
{
- assert(!reqlist_find_conflict(reqs, offset, bytes));
-
*req = (BlockReq) {
.offset = offset,
.bytes = bytes,
--
2.39.212.07.2024 17:07, Fiona Ebner wrote:
> Allow overlapping request by removing the assert that made it
> impossible. There are only two callers:
>
> 1. block_copy_task_create()
>
> It already asserts the very same condition before calling
> reqlist_init_req().
>
> 2. cbw_snapshot_read_lock()
>
> There is no need to have read requests be non-overlapping in
> copy-before-write when used for snapshot-access. In fact, there was no
> protection against two callers of cbw_snapshot_read_lock() calling
> reqlist_init_req() with overlapping ranges and this could lead to an
> assertion failure [1].
>
> In particular, with the reproducer script below [0], two
> cbw_co_snapshot_block_status() callers could race, with the second
> calling reqlist_init_req() before the first one finishes and removes
> its conflicting request.
>
> [0]:
>
>> #!/bin/bash -e
>> dd if=/dev/urandom of=/tmp/disk.raw bs=1M count=1024
>> ./qemu-img create /tmp/fleecing.raw -f raw 1G
>> (
>> ./qemu-system-x86_64 --qmp stdio \
>> --blockdev raw,node-name=node0,file.driver=file,file.filename=/tmp/disk.raw \
>> --blockdev raw,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.raw \
>> <<EOF
>> {"execute": "qmp_capabilities"}
>> {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } }
>> {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } }
>> {"execute": "nbd-server-start", "arguments": {"addr": { "type": "unix", "data": { "path": "/tmp/nbd.socket" } } } }
>> {"execute": "block-export-add", "arguments": {"id": "exp0", "node-name": "snap0", "type": "nbd", "name": "exp0"}}
>> EOF
>> ) &
>> sleep 5
>> while true; do
>> ./qemu-nbd -d /dev/nbd0
>> ./qemu-nbd -c /dev/nbd0 nbd:unix:/tmp/nbd.socket:exportname=exp0 -f raw -r
>> nbdinfo --map 'nbd+unix:///exp0?socket=/tmp/nbd.socket'
>> done
>
> [1]:
>
>> #5 0x000071e5f0088eb2 in __GI___assert_fail (...) at ./assert/assert.c:101
>> #6 0x0000615285438017 in reqlist_init_req (...) at ../block/reqlist.c:23
>> #7 0x00006152853e2d98 in cbw_snapshot_read_lock (...) at ../block/copy-before-write.c:237
>> #8 0x00006152853e3068 in cbw_co_snapshot_block_status (...) at ../block/copy-before-write.c:304
>> #9 0x00006152853f4d22 in bdrv_co_snapshot_block_status (...) at ../block/io.c:3726
>> #10 0x000061528543a63e in snapshot_access_co_block_status (...) at ../block/snapshot-access.c:48
>> #11 0x00006152853f1a0a in bdrv_co_do_block_status (...) at ../block/io.c:2474
>> #12 0x00006152853f2016 in bdrv_co_common_block_status_above (...) at ../block/io.c:2652
>> #13 0x00006152853f22cf in bdrv_co_block_status_above (...) at ../block/io.c:2732
>> #14 0x00006152853d9a86 in blk_co_block_status_above (...) at ../block/block-backend.c:1473
>> #15 0x000061528538da6c in blockstatus_to_extents (...) at ../nbd/server.c:2374
>> #16 0x000061528538deb1 in nbd_co_send_block_status (...) at ../nbd/server.c:2481
>> #17 0x000061528538f424 in nbd_handle_request (...) at ../nbd/server.c:2978
>> #18 0x000061528538f906 in nbd_trip (...) at ../nbd/server.c:3121
>> #19 0x00006152855a7caf in coroutine_trampoline (...) at ../util/coroutine-ucontext.c:175
>
> Cc: qemu-stable@nongnu.org
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Hi!
Has this been forgotten or is it not needed for 9.1?
Thanks,
/mjt
>
> Changes in v2:
> * different approach, allowing overlapping requests for
> copy-before-write rather than waiting for them. block-copy already
> asserts there are no conflicts before adding a request.
>
> block/copy-before-write.c | 3 ++-
> block/reqlist.c | 2 --
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 853e01a1eb..28f6a096cd 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -66,7 +66,8 @@ typedef struct BDRVCopyBeforeWriteState {
>
> /*
> * @frozen_read_reqs: current read requests for fleecing user in bs->file
> - * node. These areas must not be rewritten by guest.
> + * node. These areas must not be rewritten by guest. There can be multiple
> + * overlapping read requests.
> */
> BlockReqList frozen_read_reqs;
>
> diff --git a/block/reqlist.c b/block/reqlist.c
> index 08cb57cfa4..098e807378 100644
> --- a/block/reqlist.c
> +++ b/block/reqlist.c
> @@ -20,8 +20,6 @@
> void reqlist_init_req(BlockReqList *reqs, BlockReq *req, int64_t offset,
> int64_t bytes)
> {
> - assert(!reqlist_find_conflict(reqs, offset, bytes));
> -
> *req = (BlockReq) {
> .offset = offset,
> .bytes = bytes,
--
GPG Key transition (from rsa2048 to rsa4096) since 2024-04-24.
New key: rsa4096/61AD3D98ECDF2C8E 9D8B E14E 3F2A 9DD7 9199 28F1 61AD 3D98 ECDF 2C8E
Old key: rsa2048/457CE0A0804465C5 6EE1 95D1 886E 8FFB 810D 4324 457C E0A0 8044 65C5
Transition statement: http://www.corpit.ru/mjt/gpg-transition-2024.txt
On 11.08.24 20:55, Michael Tokarev wrote:
> 12.07.2024 17:07, Fiona Ebner wrote:
>> Allow overlapping request by removing the assert that made it
>> impossible. There are only two callers:
>>
>> 1. block_copy_task_create()
>>
>> It already asserts the very same condition before calling
>> reqlist_init_req().
>>
>> 2. cbw_snapshot_read_lock()
>>
>> There is no need to have read requests be non-overlapping in
>> copy-before-write when used for snapshot-access. In fact, there was no
>> protection against two callers of cbw_snapshot_read_lock() calling
>> reqlist_init_req() with overlapping ranges and this could lead to an
>> assertion failure [1].
>>
>> In particular, with the reproducer script below [0], two
>> cbw_co_snapshot_block_status() callers could race, with the second
>> calling reqlist_init_req() before the first one finishes and removes
>> its conflicting request.
>>
>> [0]:
>>
>>> #!/bin/bash -e
>>> dd if=/dev/urandom of=/tmp/disk.raw bs=1M count=1024
>>> ./qemu-img create /tmp/fleecing.raw -f raw 1G
>>> (
>>> ./qemu-system-x86_64 --qmp stdio \
>>> --blockdev raw,node-name=node0,file.driver=file,file.filename=/tmp/disk.raw \
>>> --blockdev raw,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.raw \
>>> <<EOF
>>> {"execute": "qmp_capabilities"}
>>> {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } }
>>> {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } }
>>> {"execute": "nbd-server-start", "arguments": {"addr": { "type": "unix", "data": { "path": "/tmp/nbd.socket" } } } }
>>> {"execute": "block-export-add", "arguments": {"id": "exp0", "node-name": "snap0", "type": "nbd", "name": "exp0"}}
>>> EOF
>>> ) &
>>> sleep 5
>>> while true; do
>>> ./qemu-nbd -d /dev/nbd0
>>> ./qemu-nbd -c /dev/nbd0 nbd:unix:/tmp/nbd.socket:exportname=exp0 -f raw -r
>>> nbdinfo --map 'nbd+unix:///exp0?socket=/tmp/nbd.socket'
>>> done
>>
>> [1]:
>>
>>> #5 0x000071e5f0088eb2 in __GI___assert_fail (...) at ./assert/assert.c:101
>>> #6 0x0000615285438017 in reqlist_init_req (...) at ../block/reqlist.c:23
>>> #7 0x00006152853e2d98 in cbw_snapshot_read_lock (...) at ../block/copy-before-write.c:237
>>> #8 0x00006152853e3068 in cbw_co_snapshot_block_status (...) at ../block/copy-before-write.c:304
>>> #9 0x00006152853f4d22 in bdrv_co_snapshot_block_status (...) at ../block/io.c:3726
>>> #10 0x000061528543a63e in snapshot_access_co_block_status (...) at ../block/snapshot-access.c:48
>>> #11 0x00006152853f1a0a in bdrv_co_do_block_status (...) at ../block/io.c:2474
>>> #12 0x00006152853f2016 in bdrv_co_common_block_status_above (...) at ../block/io.c:2652
>>> #13 0x00006152853f22cf in bdrv_co_block_status_above (...) at ../block/io.c:2732
>>> #14 0x00006152853d9a86 in blk_co_block_status_above (...) at ../block/block-backend.c:1473
>>> #15 0x000061528538da6c in blockstatus_to_extents (...) at ../nbd/server.c:2374
>>> #16 0x000061528538deb1 in nbd_co_send_block_status (...) at ../nbd/server.c:2481
>>> #17 0x000061528538f424 in nbd_handle_request (...) at ../nbd/server.c:2978
>>> #18 0x000061528538f906 in nbd_trip (...) at ../nbd/server.c:3121
>>> #19 0x00006152855a7caf in coroutine_trampoline (...) at ../util/coroutine-ucontext.c:175
>>
>> Cc: qemu-stable@nongnu.org
>> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>
> Hi!
>
> Has this been forgotten or is it not needed for 9.1?
>
My apologies, this is forgotten. I think rc4 is too late, I'll send Pull request as soon as 9.2 window open.
--
Best regards,
Vladimir
On 12.07.24 17:07, Fiona Ebner wrote:
> Allow overlapping request by removing the assert that made it
> impossible. There are only two callers:
>
> 1. block_copy_task_create()
>
> It already asserts the very same condition before calling
> reqlist_init_req().
>
> 2. cbw_snapshot_read_lock()
>
> There is no need to have read requests be non-overlapping in
> copy-before-write when used for snapshot-access. In fact, there was no
> protection against two callers of cbw_snapshot_read_lock() calling
> reqlist_init_req() with overlapping ranges and this could lead to an
> assertion failure [1].
>
> In particular, with the reproducer script below [0], two
> cbw_co_snapshot_block_status() callers could race, with the second
> calling reqlist_init_req() before the first one finishes and removes
> its conflicting request.
>
> [0]:
>
>> #!/bin/bash -e
>> dd if=/dev/urandom of=/tmp/disk.raw bs=1M count=1024
>> ./qemu-img create /tmp/fleecing.raw -f raw 1G
>> (
>> ./qemu-system-x86_64 --qmp stdio \
>> --blockdev raw,node-name=node0,file.driver=file,file.filename=/tmp/disk.raw \
>> --blockdev raw,node-name=node1,file.driver=file,file.filename=/tmp/fleecing.raw \
>> <<EOF
>> {"execute": "qmp_capabilities"}
>> {"execute": "blockdev-add", "arguments": { "driver": "copy-before-write", "file": "node0", "target": "node1", "node-name": "node3" } }
>> {"execute": "blockdev-add", "arguments": { "driver": "snapshot-access", "file": "node3", "node-name": "snap0" } }
>> {"execute": "nbd-server-start", "arguments": {"addr": { "type": "unix", "data": { "path": "/tmp/nbd.socket" } } } }
>> {"execute": "block-export-add", "arguments": {"id": "exp0", "node-name": "snap0", "type": "nbd", "name": "exp0"}}
>> EOF
>> ) &
>> sleep 5
>> while true; do
>> ./qemu-nbd -d /dev/nbd0
>> ./qemu-nbd -c /dev/nbd0 nbd:unix:/tmp/nbd.socket:exportname=exp0 -f raw -r
>> nbdinfo --map 'nbd+unix:///exp0?socket=/tmp/nbd.socket'
>> done
>
> [1]:
>
>> #5 0x000071e5f0088eb2 in __GI___assert_fail (...) at ./assert/assert.c:101
>> #6 0x0000615285438017 in reqlist_init_req (...) at ../block/reqlist.c:23
>> #7 0x00006152853e2d98 in cbw_snapshot_read_lock (...) at ../block/copy-before-write.c:237
>> #8 0x00006152853e3068 in cbw_co_snapshot_block_status (...) at ../block/copy-before-write.c:304
>> #9 0x00006152853f4d22 in bdrv_co_snapshot_block_status (...) at ../block/io.c:3726
>> #10 0x000061528543a63e in snapshot_access_co_block_status (...) at ../block/snapshot-access.c:48
>> #11 0x00006152853f1a0a in bdrv_co_do_block_status (...) at ../block/io.c:2474
>> #12 0x00006152853f2016 in bdrv_co_common_block_status_above (...) at ../block/io.c:2652
>> #13 0x00006152853f22cf in bdrv_co_block_status_above (...) at ../block/io.c:2732
>> #14 0x00006152853d9a86 in blk_co_block_status_above (...) at ../block/block-backend.c:1473
>> #15 0x000061528538da6c in blockstatus_to_extents (...) at ../nbd/server.c:2374
>> #16 0x000061528538deb1 in nbd_co_send_block_status (...) at ../nbd/server.c:2481
>> #17 0x000061528538f424 in nbd_handle_request (...) at ../nbd/server.c:2978
>> #18 0x000061528538f906 in nbd_trip (...) at ../nbd/server.c:3121
>> #19 0x00006152855a7caf in coroutine_trampoline (...) at ../util/coroutine-ucontext.c:175
>
> Cc: qemu-stable@nongnu.org
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Changes in v2:
> * different approach, allowing overlapping requests for
> copy-before-write rather than waiting for them. block-copy already
> asserts there are no conflicts before adding a request.
>
> block/copy-before-write.c | 3 ++-
> block/reqlist.c | 2 --
> 2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/block/copy-before-write.c b/block/copy-before-write.c
> index 853e01a1eb..28f6a096cd 100644
> --- a/block/copy-before-write.c
> +++ b/block/copy-before-write.c
> @@ -66,7 +66,8 @@ typedef struct BDRVCopyBeforeWriteState {
>
> /*
> * @frozen_read_reqs: current read requests for fleecing user in bs->file
> - * node. These areas must not be rewritten by guest.
> + * node. These areas must not be rewritten by guest. There can be multiple
> + * overlapping read requests.
> */
> BlockReqList frozen_read_reqs;
>
> diff --git a/block/reqlist.c b/block/reqlist.c
> index 08cb57cfa4..098e807378 100644
> --- a/block/reqlist.c
> +++ b/block/reqlist.c
> @@ -20,8 +20,6 @@
> void reqlist_init_req(BlockReqList *reqs, BlockReq *req, int64_t offset,
> int64_t bytes)
> {
> - assert(!reqlist_find_conflict(reqs, offset, bytes));
> -
> *req = (BlockReq) {
> .offset = offset,
> .bytes = bytes,
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Thanks, applied to my block branch
--
Best regards,
Vladimir
© 2016 - 2024 Red Hat, Inc.