Re: [PULL 24/24] block/block-copy: use aio-task-pool API

[Peter Maydell]

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