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

[Hanna Reitz <hreitz@redhat.com>]

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