On Tue, 5 May 2020 at 13:59, Max Reitz <[email protected]> wrote:
>
> From: Vladimir Sementsov-Ogievskiy <[email protected]>
>
> 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 <[email protected]>
> Message-Id: <[email protected]>
> Signed-off-by: Max Reitz <[email protected]>
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
