Message ID | 20190910110725.141014-1-slp@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | blockjob: update nodes head while removing all bdrv | expand |
On 10.09.19 13:07, Sergio Lopez wrote: > 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> > --- > blockjob.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/blockjob.c b/blockjob.c > index 6e32d1a0c0..7b1551d981 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -192,6 +192,12 @@ void block_job_remove_all_bdrv(BlockJob *job) > BdrvChild *c = l->data; > bdrv_op_unblock_all(c->bs, job->blocker); > bdrv_root_unref_child(c); > + /* > + * The call above may reach child_job_[can_]set_aio_ctx(), which will > + * also traverse job->nodes, so update the head here to make sure it > + * doesn't attempt to process an already freed BdrvChild. > + */ > + job->nodes = l->next; > } > g_slist_free(job->nodes); But this will leak the whole list. Max > job->nodes = NULL; >
Max Reitz <mreitz@redhat.com> writes: > On 10.09.19 13:07, Sergio Lopez wrote: >> 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> >> --- >> blockjob.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/blockjob.c b/blockjob.c >> index 6e32d1a0c0..7b1551d981 100644 >> --- a/blockjob.c >> +++ b/blockjob.c >> @@ -192,6 +192,12 @@ void block_job_remove_all_bdrv(BlockJob *job) >> BdrvChild *c = l->data; >> bdrv_op_unblock_all(c->bs, job->blocker); >> bdrv_root_unref_child(c); >> + /* >> + * The call above may reach child_job_[can_]set_aio_ctx(), which will >> + * also traverse job->nodes, so update the head here to make sure it >> + * doesn't attempt to process an already freed BdrvChild. >> + */ >> + job->nodes = l->next; >> } >> g_slist_free(job->nodes); > > But this will leak the whole list. Ouch. This is what happens when you rush up things. I'll send a v2 ASAP. Thanks, Sergio.
diff --git a/blockjob.c b/blockjob.c index 6e32d1a0c0..7b1551d981 100644 --- a/blockjob.c +++ b/blockjob.c @@ -192,6 +192,12 @@ void block_job_remove_all_bdrv(BlockJob *job) BdrvChild *c = l->data; bdrv_op_unblock_all(c->bs, job->blocker); bdrv_root_unref_child(c); + /* + * The call above may reach child_job_[can_]set_aio_ctx(), which will + * also traverse job->nodes, so update the head here to make sure it + * doesn't attempt to process an already freed BdrvChild. + */ + job->nodes = l->next; } g_slist_free(job->nodes); job->nodes = NULL;
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> --- blockjob.c | 6 ++++++ 1 file changed, 6 insertions(+)