blockjob.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
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>
---
Changelog
v2:
- Avoid leaking job->nodes (thanks Max Reitz)
---
blockjob.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
---
diff --git a/blockjob.c b/blockjob.c
index 6e32d1a0c0..ffda6dd1e4 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -187,13 +187,21 @@ static const BdrvChildRole child_job = {
void block_job_remove_all_bdrv(BlockJob *job)
{
- GSList *l;
+ GSList *l, *orig_nodes;
+
+ orig_nodes = job->nodes;
for (l = job->nodes; l; l = l->next) {
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);
+ g_slist_free(orig_nodes);
job->nodes = NULL;
}
--
2.21.0
On 10.09.19 15:36, 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> > --- > Changelog > > v2: > - Avoid leaking job->nodes (thanks Max Reitz) > --- > blockjob.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > --- > diff --git a/blockjob.c b/blockjob.c > index 6e32d1a0c0..ffda6dd1e4 100644 > --- a/blockjob.c > +++ b/blockjob.c > @@ -187,13 +187,21 @@ static const BdrvChildRole child_job = { > > void block_job_remove_all_bdrv(BlockJob *job) > { > - GSList *l; > + GSList *l, *orig_nodes; > + > + orig_nodes = job->nodes; > for (l = job->nodes; l; l = l->next) { > 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); > + g_slist_free(orig_nodes); > job->nodes = NULL; Hm, this assignment is now a no-op. I think I’d just rewrite the whole function in the following fashion: orig_nodes = job->nodes; while (job->nodes) { BdrvChild *c = job->nodes->data; [...] job->nodes = job->nodes->next; } g_slist_free(orig_nodes); What do you think? > } > >
Max Reitz <mreitz@redhat.com> writes: > On 10.09.19 15:36, 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> >> --- >> Changelog >> >> v2: >> - Avoid leaking job->nodes (thanks Max Reitz) >> --- >> blockjob.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> --- >> diff --git a/blockjob.c b/blockjob.c >> index 6e32d1a0c0..ffda6dd1e4 100644 >> --- a/blockjob.c >> +++ b/blockjob.c >> @@ -187,13 +187,21 @@ static const BdrvChildRole child_job = { >> >> void block_job_remove_all_bdrv(BlockJob *job) >> { >> - GSList *l; >> + GSList *l, *orig_nodes; >> + >> + orig_nodes = job->nodes; >> for (l = job->nodes; l; l = l->next) { >> 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); >> + g_slist_free(orig_nodes); >> job->nodes = NULL; > > Hm, this assignment is now a no-op. > > I think I’d just rewrite the whole function in the following fashion: > > orig_nodes = job->nodes; > while (job->nodes) { > BdrvChild *c = job->nodes->data; > [...] > job->nodes = job->nodes->next; > } > g_slist_free(orig_nodes); > > What do you think? > As this is the first time I was touching this code, I was trying to keep the changes minimal, but I definitely prefer to rewrite the function as you suggest. Should I send a v3, or do you want to send a patch yourself? I don't really mind either, just want to get this fixed ASAP :-) Thanks Max, Sergio.
© 2016 - 2024 Red Hat, Inc.