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 <hre...@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
