| Message ID | 20211104103849.46855-6-hreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Attempt on fixing 030-reported errors | expand |
Am 04.11.2021 um 11:38 hat Hanna Reitz geschrieben: > bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially > set it to NULL. That is dangerous, because BDS parents generally assume > that their children's .bs pointer is never NULL. We therefore want to > let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer > to NULL, too. > > This patch lays the foundation for it by passing a BdrvChild ** pointer > to bdrv_replace_child_noperm() so that it can later use it to NULL the > BdrvChild pointer immediately after setting BdrvChild.bs to NULL. > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > block.c | 59 ++++++++++++++++++++++++++++++++------------------------- > 1 file changed, 33 insertions(+), 26 deletions(-) > > diff --git a/block.c b/block.c > index 6d230ad3d1..ff45447686 100644 > --- a/block.c > +++ b/block.c > @@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, > static bool bdrv_recurse_has_child(BlockDriverState *bs, > BlockDriverState *child); > > -static void bdrv_replace_child_noperm(BdrvChild *child, > +static void bdrv_replace_child_noperm(BdrvChild **child, > BlockDriverState *new_bs); > static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, > BdrvChild *child, > @@ -2254,6 +2254,7 @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm, > > typedef struct BdrvReplaceChildState { > BdrvChild *child; > + BdrvChild **childp; Would it be clearer to rename child to old_child now that it can be changed? I would also consider having childp first because this is really the object that we're modifying and potentially rolling back now. > BlockDriverState *old_bs; > } BdrvReplaceChildState; > > @@ -2269,8 +2270,8 @@ static void bdrv_replace_child_abort(void *opaque) > BdrvReplaceChildState *s = opaque; > BlockDriverState *new_bs = s->child->bs; > > - /* old_bs reference is transparently moved from @s to @s->child */ > - bdrv_replace_child_noperm(s->child, s->old_bs); > + /* old_bs reference is transparently moved from @s to *s->childp */ > + bdrv_replace_child_noperm(s->childp, s->old_bs); > bdrv_unref(new_bs); > } > > @@ -2287,21 +2288,23 @@ static TransactionActionDrv bdrv_replace_child_drv = { > * > * The function doesn't update permissions, caller is responsible for this. > */ > -static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs, > +static void bdrv_replace_child_tran(BdrvChild **childp, > + BlockDriverState *new_bs, > Transaction *tran) > { > BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1); > *s = (BdrvReplaceChildState) { > - .child = child, > - .old_bs = child->bs, > + .child = *childp, > + .childp = childp, > + .old_bs = (*childp)->bs, > }; > tran_add(tran, &bdrv_replace_child_drv, s); > > if (new_bs) { > bdrv_ref(new_bs); > } > - bdrv_replace_child_noperm(child, new_bs); > - /* old_bs reference is transparently moved from @child to @s */ > + bdrv_replace_child_noperm(childp, new_bs); > + /* old_bs reference is transparently moved from *childp to @s */ > } > > /* > @@ -2672,9 +2675,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) > return permissions[qapi_perm]; > } > > -static void bdrv_replace_child_noperm(BdrvChild *child, > +static void bdrv_replace_child_noperm(BdrvChild **childp, > BlockDriverState *new_bs) > { > + BdrvChild *child = *childp; > BlockDriverState *old_bs = child->bs; > int new_bs_quiesce_counter; > int drain_saldo; > @@ -2767,7 +2771,7 @@ static void bdrv_attach_child_common_abort(void *opaque) > BdrvChild *child = *s->child; > BlockDriverState *bs = child->bs; > > - bdrv_replace_child_noperm(child, NULL); > + bdrv_replace_child_noperm(s->child, NULL); > > if (bdrv_get_aio_context(bs) != s->old_child_ctx) { > bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); > @@ -2867,7 +2871,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, > } > > bdrv_ref(child_bs); > - bdrv_replace_child_noperm(new_child, child_bs); > + bdrv_replace_child_noperm(&new_child, child_bs); > > *child = new_child; > > @@ -2927,12 +2931,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, > return 0; > } > > -static void bdrv_detach_child(BdrvChild *child) > +static void bdrv_detach_child(BdrvChild **childp) > { > - BlockDriverState *old_bs = child->bs; > + BlockDriverState *old_bs = (*childp)->bs; > > - bdrv_replace_child_noperm(child, NULL); > - bdrv_child_free(child); > + bdrv_replace_child_noperm(childp, NULL); > + bdrv_child_free(*childp); > > if (old_bs) { > /* > @@ -3038,7 +3042,7 @@ void bdrv_root_unref_child(BdrvChild *child) > BlockDriverState *child_bs; > > child_bs = child->bs; > - bdrv_detach_child(child); > + bdrv_detach_child(&child); > bdrv_unref(child_bs); > } > > @@ -4891,30 +4895,33 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, > BdrvChild *child, > Transaction *tran) > { > + BdrvChild **childp; > BdrvRemoveFilterOrCowChild *s; > > - assert(child == bs->backing || child == bs->file); > + if (child == bs->backing) { > + childp = &bs->backing; > + } else if (child == bs->file) { > + childp = &bs->file; > + } else { > + g_assert_not_reached(); > + } > > if (!child) { > return; > } > > if (child->bs) { > - bdrv_replace_child_tran(child, NULL, tran); > + bdrv_replace_child_tran(childp, NULL, tran); > } > > s = g_new(BdrvRemoveFilterOrCowChild, 1); > *s = (BdrvRemoveFilterOrCowChild) { > .child = child, > - .is_backing = (child == bs->backing), > + .is_backing = (childp == &bs->backing), > }; > tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); > > - if (s->is_backing) { > - bs->backing = NULL; > - } else { > - bs->file = NULL; > - } > + *childp = NULL; > } Hmm... This looks a bit dangerous. Is it guaranteed that bs lives until the end of the transaction? I guess it has to because we want to be able to roll back, so probably this is okay, though I'm not sure if I would bet money on it. But... > @@ -4950,7 +4957,7 @@ static int bdrv_replace_node_noperm(BlockDriverState *from, > c->name, from->node_name); > return -EPERM; > } > - bdrv_replace_child_tran(c, to, tran); > + bdrv_replace_child_tran(&c, to, tran); > } ...here, c is a local stack variable that is even reused in a loop. bdrv_replace_child_tran() now keeps a pointer to the same variable in the transaction state for each BdrvChild in the whole parent list, and by the time the transaction is finalised, I think they are all dangling pointers anyway because they pointed to stack variables in a function that has already returned. bdrv_replace_child_tran() should probably have a comment like we already have in bdrv_attach_child_common(): * @child is saved to a new entry of @tran, so that *@child could be reverted to * NULL on abort(). So referenced variable must live at least until transaction * end. > return 0; > @@ -5099,7 +5106,7 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, > bdrv_drained_begin(old_bs); > bdrv_drained_begin(new_bs); > > - bdrv_replace_child_tran(child, new_bs, tran); > + bdrv_replace_child_tran(&child, new_bs, tran); > > found = g_hash_table_new(NULL, NULL); > refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs); Kevin
On 05.11.21 16:15, Kevin Wolf wrote: > Am 04.11.2021 um 11:38 hat Hanna Reitz geschrieben: >> bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially >> set it to NULL. That is dangerous, because BDS parents generally assume >> that their children's .bs pointer is never NULL. We therefore want to >> let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer >> to NULL, too. >> >> This patch lays the foundation for it by passing a BdrvChild ** pointer >> to bdrv_replace_child_noperm() so that it can later use it to NULL the >> BdrvChild pointer immediately after setting BdrvChild.bs to NULL. >> >> Signed-off-by: Hanna Reitz <hreitz@redhat.com> >> --- >> block.c | 59 ++++++++++++++++++++++++++++++++------------------------- >> 1 file changed, 33 insertions(+), 26 deletions(-) >> >> diff --git a/block.c b/block.c >> index 6d230ad3d1..ff45447686 100644 >> --- a/block.c >> +++ b/block.c >> @@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, >> static bool bdrv_recurse_has_child(BlockDriverState *bs, >> BlockDriverState *child); >> >> -static void bdrv_replace_child_noperm(BdrvChild *child, >> +static void bdrv_replace_child_noperm(BdrvChild **child, >> BlockDriverState *new_bs); >> static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, >> BdrvChild *child, >> @@ -2254,6 +2254,7 @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm, >> >> typedef struct BdrvReplaceChildState { >> BdrvChild *child; >> + BdrvChild **childp; > Would it be clearer to rename child to old_child now that it can be > changed? > > I would also consider having childp first because this is really the > object that we're modifying and potentially rolling back now. > >> BlockDriverState *old_bs; >> } BdrvReplaceChildState; >> >> @@ -2269,8 +2270,8 @@ static void bdrv_replace_child_abort(void *opaque) >> BdrvReplaceChildState *s = opaque; >> BlockDriverState *new_bs = s->child->bs; >> >> - /* old_bs reference is transparently moved from @s to @s->child */ >> - bdrv_replace_child_noperm(s->child, s->old_bs); >> + /* old_bs reference is transparently moved from @s to *s->childp */ >> + bdrv_replace_child_noperm(s->childp, s->old_bs); >> bdrv_unref(new_bs); >> } >> >> @@ -2287,21 +2288,23 @@ static TransactionActionDrv bdrv_replace_child_drv = { >> * >> * The function doesn't update permissions, caller is responsible for this. >> */ >> -static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs, >> +static void bdrv_replace_child_tran(BdrvChild **childp, >> + BlockDriverState *new_bs, >> Transaction *tran) >> { >> BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1); >> *s = (BdrvReplaceChildState) { >> - .child = child, >> - .old_bs = child->bs, >> + .child = *childp, >> + .childp = childp, >> + .old_bs = (*childp)->bs, >> }; >> tran_add(tran, &bdrv_replace_child_drv, s); >> >> if (new_bs) { >> bdrv_ref(new_bs); >> } >> - bdrv_replace_child_noperm(child, new_bs); >> - /* old_bs reference is transparently moved from @child to @s */ >> + bdrv_replace_child_noperm(childp, new_bs); >> + /* old_bs reference is transparently moved from *childp to @s */ >> } >> >> /* >> @@ -2672,9 +2675,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) >> return permissions[qapi_perm]; >> } >> >> -static void bdrv_replace_child_noperm(BdrvChild *child, >> +static void bdrv_replace_child_noperm(BdrvChild **childp, >> BlockDriverState *new_bs) >> { >> + BdrvChild *child = *childp; >> BlockDriverState *old_bs = child->bs; >> int new_bs_quiesce_counter; >> int drain_saldo; >> @@ -2767,7 +2771,7 @@ static void bdrv_attach_child_common_abort(void *opaque) >> BdrvChild *child = *s->child; >> BlockDriverState *bs = child->bs; >> >> - bdrv_replace_child_noperm(child, NULL); >> + bdrv_replace_child_noperm(s->child, NULL); >> >> if (bdrv_get_aio_context(bs) != s->old_child_ctx) { >> bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); >> @@ -2867,7 +2871,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, >> } >> >> bdrv_ref(child_bs); >> - bdrv_replace_child_noperm(new_child, child_bs); >> + bdrv_replace_child_noperm(&new_child, child_bs); >> >> *child = new_child; >> >> @@ -2927,12 +2931,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, >> return 0; >> } >> >> -static void bdrv_detach_child(BdrvChild *child) >> +static void bdrv_detach_child(BdrvChild **childp) >> { >> - BlockDriverState *old_bs = child->bs; >> + BlockDriverState *old_bs = (*childp)->bs; >> >> - bdrv_replace_child_noperm(child, NULL); >> - bdrv_child_free(child); >> + bdrv_replace_child_noperm(childp, NULL); >> + bdrv_child_free(*childp); >> >> if (old_bs) { >> /* >> @@ -3038,7 +3042,7 @@ void bdrv_root_unref_child(BdrvChild *child) >> BlockDriverState *child_bs; >> >> child_bs = child->bs; >> - bdrv_detach_child(child); >> + bdrv_detach_child(&child); >> bdrv_unref(child_bs); >> } >> >> @@ -4891,30 +4895,33 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, >> BdrvChild *child, >> Transaction *tran) >> { >> + BdrvChild **childp; >> BdrvRemoveFilterOrCowChild *s; >> >> - assert(child == bs->backing || child == bs->file); >> + if (child == bs->backing) { >> + childp = &bs->backing; >> + } else if (child == bs->file) { >> + childp = &bs->file; >> + } else { >> + g_assert_not_reached(); >> + } >> >> if (!child) { >> return; >> } >> >> if (child->bs) { >> - bdrv_replace_child_tran(child, NULL, tran); >> + bdrv_replace_child_tran(childp, NULL, tran); >> } >> >> s = g_new(BdrvRemoveFilterOrCowChild, 1); >> *s = (BdrvRemoveFilterOrCowChild) { >> .child = child, >> - .is_backing = (child == bs->backing), >> + .is_backing = (childp == &bs->backing), >> }; >> tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); >> >> - if (s->is_backing) { >> - bs->backing = NULL; >> - } else { >> - bs->file = NULL; >> - } >> + *childp = NULL; >> } > Hmm... This looks a bit dangerous. Is it guaranteed that bs lives until > the end of the transaction? I guess it has to because we want to be able > to roll back, so probably this is okay, though I'm not sure if I would > bet money on it. Well, if it should live long enough, a bdrv_ref()+unref() definitely shouldn’t hurt. > But... > >> @@ -4950,7 +4957,7 @@ static int bdrv_replace_node_noperm(BlockDriverState *from, >> c->name, from->node_name); >> return -EPERM; >> } >> - bdrv_replace_child_tran(c, to, tran); >> + bdrv_replace_child_tran(&c, to, tran); >> } > ...here, c is a local stack variable that is even reused in a loop. > bdrv_replace_child_tran() now keeps a pointer to the same variable in > the transaction state for each BdrvChild in the whole parent list, and > by the time the transaction is finalised, I think they are all dangling > pointers anyway because they pointed to stack variables in a function > that has already returned. Oh no. Yes, that’s a bit of a problem... > bdrv_replace_child_tran() should probably have a comment like we already > have in bdrv_attach_child_common(): > > * @child is saved to a new entry of @tran, so that *@child could be reverted to > * NULL on abort(). So referenced variable must live at least until transaction > * end. Right. Now just to figure out how to solve the concrete problem... :/ (Yes, a borrow checker would’ve helped :)) Hanna
diff --git a/block.c b/block.c index 6d230ad3d1..ff45447686 100644 --- a/block.c +++ b/block.c @@ -87,7 +87,7 @@ static BlockDriverState *bdrv_open_inherit(const char *filename, static bool bdrv_recurse_has_child(BlockDriverState *bs, BlockDriverState *child); -static void bdrv_replace_child_noperm(BdrvChild *child, +static void bdrv_replace_child_noperm(BdrvChild **child, BlockDriverState *new_bs); static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, BdrvChild *child, @@ -2254,6 +2254,7 @@ static int bdrv_drv_set_perm(BlockDriverState *bs, uint64_t perm, typedef struct BdrvReplaceChildState { BdrvChild *child; + BdrvChild **childp; BlockDriverState *old_bs; } BdrvReplaceChildState; @@ -2269,8 +2270,8 @@ static void bdrv_replace_child_abort(void *opaque) BdrvReplaceChildState *s = opaque; BlockDriverState *new_bs = s->child->bs; - /* old_bs reference is transparently moved from @s to @s->child */ - bdrv_replace_child_noperm(s->child, s->old_bs); + /* old_bs reference is transparently moved from @s to *s->childp */ + bdrv_replace_child_noperm(s->childp, s->old_bs); bdrv_unref(new_bs); } @@ -2287,21 +2288,23 @@ static TransactionActionDrv bdrv_replace_child_drv = { * * The function doesn't update permissions, caller is responsible for this. */ -static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs, +static void bdrv_replace_child_tran(BdrvChild **childp, + BlockDriverState *new_bs, Transaction *tran) { BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1); *s = (BdrvReplaceChildState) { - .child = child, - .old_bs = child->bs, + .child = *childp, + .childp = childp, + .old_bs = (*childp)->bs, }; tran_add(tran, &bdrv_replace_child_drv, s); if (new_bs) { bdrv_ref(new_bs); } - bdrv_replace_child_noperm(child, new_bs); - /* old_bs reference is transparently moved from @child to @s */ + bdrv_replace_child_noperm(childp, new_bs); + /* old_bs reference is transparently moved from *childp to @s */ } /* @@ -2672,9 +2675,10 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm) return permissions[qapi_perm]; } -static void bdrv_replace_child_noperm(BdrvChild *child, +static void bdrv_replace_child_noperm(BdrvChild **childp, BlockDriverState *new_bs) { + BdrvChild *child = *childp; BlockDriverState *old_bs = child->bs; int new_bs_quiesce_counter; int drain_saldo; @@ -2767,7 +2771,7 @@ static void bdrv_attach_child_common_abort(void *opaque) BdrvChild *child = *s->child; BlockDriverState *bs = child->bs; - bdrv_replace_child_noperm(child, NULL); + bdrv_replace_child_noperm(s->child, NULL); if (bdrv_get_aio_context(bs) != s->old_child_ctx) { bdrv_try_set_aio_context(bs, s->old_child_ctx, &error_abort); @@ -2867,7 +2871,7 @@ static int bdrv_attach_child_common(BlockDriverState *child_bs, } bdrv_ref(child_bs); - bdrv_replace_child_noperm(new_child, child_bs); + bdrv_replace_child_noperm(&new_child, child_bs); *child = new_child; @@ -2927,12 +2931,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, return 0; } -static void bdrv_detach_child(BdrvChild *child) +static void bdrv_detach_child(BdrvChild **childp) { - BlockDriverState *old_bs = child->bs; + BlockDriverState *old_bs = (*childp)->bs; - bdrv_replace_child_noperm(child, NULL); - bdrv_child_free(child); + bdrv_replace_child_noperm(childp, NULL); + bdrv_child_free(*childp); if (old_bs) { /* @@ -3038,7 +3042,7 @@ void bdrv_root_unref_child(BdrvChild *child) BlockDriverState *child_bs; child_bs = child->bs; - bdrv_detach_child(child); + bdrv_detach_child(&child); bdrv_unref(child_bs); } @@ -4891,30 +4895,33 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, BdrvChild *child, Transaction *tran) { + BdrvChild **childp; BdrvRemoveFilterOrCowChild *s; - assert(child == bs->backing || child == bs->file); + if (child == bs->backing) { + childp = &bs->backing; + } else if (child == bs->file) { + childp = &bs->file; + } else { + g_assert_not_reached(); + } if (!child) { return; } if (child->bs) { - bdrv_replace_child_tran(child, NULL, tran); + bdrv_replace_child_tran(childp, NULL, tran); } s = g_new(BdrvRemoveFilterOrCowChild, 1); *s = (BdrvRemoveFilterOrCowChild) { .child = child, - .is_backing = (child == bs->backing), + .is_backing = (childp == &bs->backing), }; tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); - if (s->is_backing) { - bs->backing = NULL; - } else { - bs->file = NULL; - } + *childp = NULL; } /* @@ -4950,7 +4957,7 @@ static int bdrv_replace_node_noperm(BlockDriverState *from, c->name, from->node_name); return -EPERM; } - bdrv_replace_child_tran(c, to, tran); + bdrv_replace_child_tran(&c, to, tran); } return 0; @@ -5099,7 +5106,7 @@ int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, bdrv_drained_begin(old_bs); bdrv_drained_begin(new_bs); - bdrv_replace_child_tran(child, new_bs, tran); + bdrv_replace_child_tran(&child, new_bs, tran); found = g_hash_table_new(NULL, NULL); refresh_list = bdrv_topological_dfs(refresh_list, found, old_bs);
bdrv_replace_child_noperm() modifies BdrvChild.bs, and can potentially set it to NULL. That is dangerous, because BDS parents generally assume that their children's .bs pointer is never NULL. We therefore want to let bdrv_replace_child_noperm() set the corresponding BdrvChild pointer to NULL, too. This patch lays the foundation for it by passing a BdrvChild ** pointer to bdrv_replace_child_noperm() so that it can later use it to NULL the BdrvChild pointer immediately after setting BdrvChild.bs to NULL. Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block.c | 59 ++++++++++++++++++++++++++++++++------------------------- 1 file changed, 33 insertions(+), 26 deletions(-)