Skip to content

Commit

Permalink
block/reqlist: allow adding overlapping requests
Browse files Browse the repository at this point in the history
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
> qemu#10 0x000061528543a63e in snapshot_access_co_block_status (...) at ../block/snapshot-access.c:48
> qemu#11 0x00006152853f1a0a in bdrv_co_do_block_status (...) at ../block/io.c:2474
> qemu#12 0x00006152853f2016 in bdrv_co_common_block_status_above (...) at ../block/io.c:2652
> qemu#13 0x00006152853f22cf in bdrv_co_block_status_above (...) at ../block/io.c:2732
> qemu#14 0x00006152853d9a86 in blk_co_block_status_above (...) at ../block/block-backend.c:1473
> qemu#15 0x000061528538da6c in blockstatus_to_extents (...) at ../nbd/server.c:2374
> qemu#16 0x000061528538deb1 in nbd_co_send_block_status (...) at ../nbd/server.c:2481
> qemu#17 0x000061528538f424 in nbd_handle_request (...) at ../nbd/server.c:2978
> qemu#18 0x000061528538f906 in nbd_trip (...) at ../nbd/server.c:3121
> qemu#19 0x00006152855a7caf in coroutine_trampoline (...) at ../util/coroutine-ucontext.c:175

Cc: [email protected]
Suggested-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Signed-off-by: Fiona Ebner <[email protected]>
Message-Id: <[email protected]>
Reviewed-by: Vladimir Sementsov-Ogievskiy <[email protected]>
Signed-off-by: Vladimir Sementsov-Ogievskiy <[email protected]>
  • Loading branch information
foxmox authored and Vladimir Sementsov-Ogievskiy committed Sep 30, 2024
1 parent 6252deb commit 6475155
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 3 deletions.
3 changes: 2 additions & 1 deletion block/copy-before-write.c
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 0 additions & 2 deletions block/reqlist.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down

0 comments on commit 6475155

Please sign in to comment.