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