Re: [PATCH 5/7] block: Pass BdrvChild ** to replace_child_noperm

[Kevin Wolf]

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