[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 24/24] block/block-copy: use aio-task-pool API
From: |
Peter Maydell |
Subject: |
Re: [PULL 24/24] block/block-copy: use aio-task-pool API |
Date: |
Thu, 7 May 2020 16:52:27 +0100 |
On Tue, 5 May 2020 at 13:59, Max Reitz <address@hidden> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <address@hidden>
>
> Run block_copy iterations in parallel in aio tasks.
>
> Changes:
> - BlockCopyTask becomes aio task structure. Add zeroes field to pass
> it to block_copy_do_copy
> - add call state - it's a state of one call of block_copy(), shared
> between parallel tasks. For now used only to keep information about
> first error: is it read or not.
> - convert block_copy_dirty_clusters to aio-task loop.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Message-Id: <address@hidden>
> Signed-off-by: Max Reitz <address@hidden>
Hi; this patch seems to introduce a use-after-free (CID 1428756):
> @@ -484,6 +554,8 @@ static int coroutine_fn
> block_copy_dirty_clusters(BlockCopyState *s,
> int ret = 0;
> bool found_dirty = false;
> int64_t end = offset + bytes;
> + AioTaskPool *aio = NULL;
> + BlockCopyCallState call_state = {false, false};
>
> /*
> * block_copy() user is responsible for keeping source and target in same
> @@ -495,11 +567,11 @@ static int coroutine_fn
> block_copy_dirty_clusters(BlockCopyState *s,
> assert(QEMU_IS_ALIGNED(offset, s->cluster_size));
> assert(QEMU_IS_ALIGNED(bytes, s->cluster_size));
>
> - while (bytes) {
> - g_autofree BlockCopyTask *task = NULL;
> + while (bytes && aio_task_pool_status(aio) == 0) {
> + BlockCopyTask *task;
> int64_t status_bytes;
>
> - task = block_copy_task_create(s, offset, bytes);
> + task = block_copy_task_create(s, &call_state, offset, bytes);
> if (!task) {
> /* No more dirty bits in the bitmap */
> trace_block_copy_skip_range(s, offset, bytes);
> @@ -519,6 +591,7 @@ static int coroutine_fn
> block_copy_dirty_clusters(BlockCopyState *s,
> }
> if (s->skip_unallocated && !(ret & BDRV_BLOCK_ALLOCATED)) {
> block_copy_task_end(task, 0);
> + g_free(task);
This hunk frees 'task' here...
> progress_set_remaining(s->progress,
> bdrv_get_dirty_count(s->copy_bitmap) +
> s->in_flight_bytes);
...but the next lines after this one (just out of context) are:
trace_block_copy_skip_range(s, task->offset, task->bytes);
offset = task_end(task);
which now dereference 'task' after it is freed.
thanks
-- PMM
- [PULL 14/24] iotests: use python logging for iotests.log(), (continued)
- [PULL 14/24] iotests: use python logging for iotests.log(), Max Reitz, 2020/05/05
- [PULL 19/24] Fix iotest 153, Max Reitz, 2020/05/05
- [PULL 17/24] qcow2: Tweak comment about bitmaps vs. resize, Max Reitz, 2020/05/05
- [PULL 21/24] block/block-copy: alloc task on each iteration, Max Reitz, 2020/05/05
- [PULL 16/24] qcow2: Allow resize of images with internal snapshots, Max Reitz, 2020/05/05
- [PULL 20/24] block/block-copy: rename in-flight requests to tasks, Max Reitz, 2020/05/05
- [PULL 22/24] block/block-copy: add state pointer to BlockCopyTask, Max Reitz, 2020/05/05
- [PULL 15/24] block: Add blk_new_with_bs() helper, Max Reitz, 2020/05/05
- [PULL 23/24] block/block-copy: refactor task creation, Max Reitz, 2020/05/05
- [PULL 24/24] block/block-copy: use aio-task-pool API, Max Reitz, 2020/05/05
- Re: [PULL 24/24] block/block-copy: use aio-task-pool API,
Peter Maydell <=
- Re: [PULL 00/24] Block patches, Peter Maydell, 2020/05/06