[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-stable] [PATCH v3] blockjob: update nodes head while removing
From: |
Sergio Lopez |
Subject: |
Re: [Qemu-stable] [PATCH v3] blockjob: update nodes head while removing all bdrv |
Date: |
Wed, 11 Sep 2019 14:40:05 +0200 |
User-agent: |
mu4e 1.2.0; emacs 26.2 |
Max Reitz <address@hidden> writes:
> From: Sergio Lopez <address@hidden>
>
> block_job_remove_all_bdrv() iterates through job->nodes, calling
> bdrv_root_unref_child() for each entry. The call to the latter may
> reach child_job_[can_]set_aio_ctx(), which will also attempt to
> traverse job->nodes, potentially finding entries that where freed
> on previous iterations.
>
> To avoid this situation, update job->nodes head on each iteration to
> ensure that already freed entries are no longer linked to the list.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1746631
> Signed-off-by: Sergio Lopez <address@hidden>
> Cc: address@hidden
> Signed-off-by: Max Reitz <address@hidden>
> ---
> v3:
> - Rewrote the loop to make the whole function a bit simpler
> (Also, remove the node from the job->nodes list before unref'ing it,
> just to be extra-safe)
> ---
> blockjob.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/blockjob.c b/blockjob.c
> index 2abed0f551..c6e20e2fcd 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -175,14 +175,23 @@ static const BdrvChildRole child_job = {
>
> void block_job_remove_all_bdrv(BlockJob *job)
> {
> - GSList *l;
> - for (l = job->nodes; l; l = l->next) {
> + /*
> + * bdrv_root_unref_child() may reach child_job_[can_]set_aio_ctx(),
> + * which will also traverse job->nodes, so consume the list one by
> + * one to make sure that such a concurrent access does not attempt
> + * to process an already freed BdrvChild.
> + */
> + while (job->nodes) {
> + GSList *l = job->nodes;
> BdrvChild *c = l->data;
> +
> + job->nodes = l->next;
> +
> bdrv_op_unblock_all(c->bs, job->blocker);
> bdrv_root_unref_child(c);
> +
> + g_slist_free_1(l);
> }
> - g_slist_free(job->nodes);
> - job->nodes = NULL;
> }
>
> bool block_job_has_bdrv(BlockJob *job, BlockDriverState *bs)
Reviewed-by: Sergio Lopez <address@hidden>
signature.asc
Description: PGP signature