Re: [PATCH for-5.0 v2 15/23] mirror: Prevent loops

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 9083 bytes --]

On 02.12.19 13:12, Vladimir Sementsov-Ogievskiy wrote:
> 11.11.2019 19:02, Max Reitz wrote:
>> While bdrv_replace_node() will not follow through with it, a specific
>> @replaces asks the mirror job to create a loop.
>>
>> For example, say both the source and the target share a child where the
>> source is a filter; by letting @replaces point to the common child, you
>> ask for a loop.
>>
>> Or if you use @replaces in drive-mirror with sync=none and
>> mode=absolute-paths, you generally ask for a loop (@replaces must point
>> to a child of the source, and sync=none makes the source the backing
>> file of the target after the job).
>>
>> bdrv_replace_node() will not create those loops, but by doing so, it
>> ignores the user-requested configuration, which is not ideally either.
>> (In the first example above, the target's child will remain what it was,
>> which may still be reasonable.  But in the second example, the target
>> will just not become a child of the source, which is precisely what was
>> requested with @replaces.)
>>
>> So prevent such configurations, both before the job, and before it
>> actually completes.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   block.c                   | 30 ++++++++++++++++++++++++
>>   block/mirror.c            | 19 +++++++++++++++-
>>   blockdev.c                | 48 ++++++++++++++++++++++++++++++++++++++-
>>   include/block/block_int.h |  3 +++
>>   4 files changed, 98 insertions(+), 2 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 0159f8e510..e3922a0474 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -6259,6 +6259,36 @@ out:
>>       return to_replace_bs;
>>   }
>>   
>> +/*
>> + * Return true iff @child is a (recursive) child of @parent, with at
>> + * least @min_level edges between them.
>> + *
>> + * (If @min_level == 0, return true if @child == @parent.  For
>> + * @min_level == 1, @child needs to be at least a real child; for
>> + * @min_level == 2, it needs to be at least a grand-child; and so on.)
>> + */
>> +bool bdrv_is_child_of(BlockDriverState *child, BlockDriverState *parent,
>> +                      int min_level)
>> +{
>> +    BdrvChild *c;
>> +
>> +    if (child == parent && min_level <= 0) {
>> +        return true;
>> +    }
>> +
>> +    if (!parent) {
>> +        return false;
>> +    }
>> +
>> +    QLIST_FOREACH(c, &parent->children, next) {
>> +        if (bdrv_is_child_of(child, c->bs, min_level - 1)) {
>> +            return true;
>> +        }
>> +    }
>> +
>> +    return false;
>> +}
>> +
>>   /**
>>    * Iterates through the list of runtime option keys that are said to
>>    * be "strong" for a BDS.  An option is called "strong" if it changes
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 68a4404666..b258c7e98b 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -701,7 +701,24 @@ static int mirror_exit_common(Job *job)
>>            * there.
>>            */
>>           if (bdrv_recurse_can_replace(src, to_replace)) {
>> -            bdrv_replace_node(to_replace, target_bs, &local_err);
>> +            /*
>> +             * It is OK for @to_replace to be an immediate child of
>> +             * @target_bs, because that is what happens with
>> +             * drive-mirror sync=none mode=absolute-paths: target_bs's
>> +             * backing file will be the source node, which is also
>> +             * to_replace (by default).
>> +             * bdrv_replace_node() handles this case by not letting
>> +             * target_bs->backing point to itself, but to the source
>> +             * still.
>> +             */
>> +            if (!bdrv_is_child_of(to_replace, target_bs, 2)) {
>> +                bdrv_replace_node(to_replace, target_bs, &local_err);
>> +            } else {
>> +                error_setg(&local_err, "Can no longer replace '%s' by '%s', "
>> +                           "because the former is now a child of the latter, "
>> +                           "and doing so would thus create a loop",
>> +                           to_replace->node_name, target_bs->node_name);
>> +            }
> 
> you may swap if and else branch, dropping "!" mark..

Yes, but I just personally prefer to have the error case in the else branch.

>>           } else {
>>               error_setg(&local_err, "Can no longer replace '%s' by '%s', "
>>                          "because it can no longer be guaranteed that doing so "
>> diff --git a/blockdev.c b/blockdev.c
>> index 9dc2238bf3..d29f147f72 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -3824,7 +3824,7 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>>       }
>>   
>>       if (has_replaces) {
>> -        BlockDriverState *to_replace_bs;
>> +        BlockDriverState *to_replace_bs, *target_backing_bs;
>>           AioContext *replace_aio_context;
>>           int64_t bs_size, replace_size;
>>   
>> @@ -3839,6 +3839,52 @@ static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
>>               return;
>>           }
>>   
>> +        if (bdrv_is_child_of(to_replace_bs, target, 1)) {
>> +            error_setg(errp, "Replacing %s by %s would result in a loop, "
>> +                       "because the former is a child of the latter",
>> +                       to_replace_bs->node_name, target->node_name);
>> +            return;
>> +        }
> 
> here min_level=1, so we don't handle the case, described in mirror_exit_common..
> I don't see why.. blockdev_mirror_common is called from qmp_drive_mirror,
> including the case with MIRROR_SYNC_MODE_NONE and NEW_IMAGE_MODE_ABSOLUTE_PATHS..
> 
> What I'm missing?

Hmm.  Well.

If it broke drive-mirror sync=none, I suppose I would have noticed by
running the iotests.  But I didn’t, and that’s because this code here is
reached only if the user actually specified @replaces.  (As opposed to
the mirror_exit_common code, where @to_replace may simply be @src if not
overridden by the user.)

The only reason why I allow it in mirror_exit_common is because we have
to.  But if the user manually specifies this configuration, we can’t
guarantee it’s safe.

OTOH, well, if we allow it for drive-mirror sync=none, why not allow it
when manually specified with blockdev-mirror?

What’s your opinion?

>> +
>> +        if (backing_mode == MIRROR_SOURCE_BACKING_CHAIN ||
>> +            backing_mode == MIRROR_OPEN_BACKING_CHAIN)
>> +        {
>> +            /*
>> +             * While we do not quite know what OPEN_BACKING_CHAIN
>> +             * (used for mode=existing) will yield, it is probably
>> +             * best to restrict it exactly like SOURCE_BACKING_CHAIN,
>> +             * because that is our best guess.
>> +             */
>> +            switch (sync) {
>> +            case MIRROR_SYNC_MODE_FULL:
>> +                target_backing_bs = NULL;
>> +                break;
>> +
>> +            case MIRROR_SYNC_MODE_TOP:
>> +                target_backing_bs = backing_bs(bs);
>> +                break;
>> +
>> +            case MIRROR_SYNC_MODE_NONE:
>> +                target_backing_bs = bs;
>> +                break;
>> +
>> +            default:
>> +                abort();
>> +            }
>> +        } else {
>> +            assert(backing_mode == MIRROR_LEAVE_BACKING_CHAIN);
>> +            target_backing_bs = backing_bs(target);
>> +        }
>> +
>> +        if (bdrv_is_child_of(to_replace_bs, target_backing_bs, 0)) {
>> +            error_setg(errp, "Replacing '%s' by '%s' with this sync mode would "
>> +                       "result in a loop, because the former would be a child "
>> +                       "of the latter's backing file ('%s') after the mirror "
>> +                       "job", to_replace_bs->node_name, target->node_name,
>> +                       target_backing_bs->node_name);
>> +            return;
>> +        }
> 
> hmm.. so for MODE_NONE we disallow to_replace == src?

I suppose that’s basically the same as above.  Should we allow this case
when specified explicitly by the user?

Max

>> +
>>           replace_aio_context = bdrv_get_aio_context(to_replace_bs);
>>           aio_context_acquire(replace_aio_context);
>>           replace_size = bdrv_getlength(to_replace_bs);
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 589a797fab..7064a1a4fa 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -1266,6 +1266,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, BdrvChild *c,
>>   bool bdrv_recurse_can_replace(BlockDriverState *bs,
>>                                 BlockDriverState *to_replace);
>>   
>> +bool bdrv_is_child_of(BlockDriverState *child, BlockDriverState *parent,
>> +                      int min_level);
>> +
>>   /*
>>    * Default implementation for drivers to pass bdrv_co_block_status() to
>>    * their file.
>>
> 
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]