* [Qemu-devel] [PATCH v3] blockjob: update nodes head while removing all bdrv
@ 2019-09-11 10:03 Max Reitz
2019-09-11 12:40 ` Sergio Lopez
2019-09-13 11:24 ` Max Reitz
0 siblings, 2 replies; 3+ messages in thread
From: Max Reitz @ 2019-09-11 10:03 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-stable, qemu-devel, Sergio Lopez, Max Reitz
From: Sergio Lopez <slp@redhat.com>
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 <slp@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
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)
--
2.21.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v3] blockjob: update nodes head while removing all bdrv
2019-09-11 10:03 [Qemu-devel] [PATCH v3] blockjob: update nodes head while removing all bdrv Max Reitz
@ 2019-09-11 12:40 ` Sergio Lopez
2019-09-13 11:24 ` Max Reitz
1 sibling, 0 replies; 3+ messages in thread
From: Sergio Lopez @ 2019-09-11 12:40 UTC (permalink / raw)
To: Max Reitz; +Cc: Kevin Wolf, qemu-devel, qemu-block, qemu-stable
[-- Attachment #1: Type: text/plain, Size: 2079 bytes --]
Max Reitz <mreitz@redhat.com> writes:
> From: Sergio Lopez <slp@redhat.com>
>
> 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 <slp@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> 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 <slp@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH v3] blockjob: update nodes head while removing all bdrv
2019-09-11 10:03 [Qemu-devel] [PATCH v3] blockjob: update nodes head while removing all bdrv Max Reitz
2019-09-11 12:40 ` Sergio Lopez
@ 2019-09-13 11:24 ` Max Reitz
1 sibling, 0 replies; 3+ messages in thread
From: Max Reitz @ 2019-09-13 11:24 UTC (permalink / raw)
To: qemu-block; +Cc: Kevin Wolf, qemu-devel, Sergio Lopez, qemu-stable
[-- Attachment #1.1: Type: text/plain, Size: 1202 bytes --]
On 11.09.19 12:03, Max Reitz wrote:
> From: Sergio Lopez <slp@redhat.com>
>
> 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 <slp@redhat.com>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> 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(-)
Thanks Sergio for tracking down the bug’s cause, the original patch, and
the review; I’ve applied the patch to my block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-13 11:25 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-11 10:03 [Qemu-devel] [PATCH v3] blockjob: update nodes head while removing all bdrv Max Reitz
2019-09-11 12:40 ` Sergio Lopez
2019-09-13 11:24 ` Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.