* [PATCH] block: Fail gracefully when blockdev-snapshot creates loops
@ 2021-10-18 13:47 Kevin Wolf
2021-10-18 14:22 ` Vladimir Sementsov-Ogievskiy
0 siblings, 1 reply; 3+ messages in thread
From: Kevin Wolf @ 2021-10-18 13:47 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, hreitz, vsementsov, qemu-devel
Using blockdev-snapshot to append a node as an overlay to itself, or to
any of its parents, causes crashes. Catch the condition and return an
error for these cases instead.
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1824363
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 10 ++++++++++
tests/qemu-iotests/085 | 31 ++++++++++++++++++++++++++++++-
tests/qemu-iotests/085.out | 33 ++++++++++++++++++++++++++++++---
3 files changed, 70 insertions(+), 4 deletions(-)
diff --git a/block.c b/block.c
index 45f653a88b..231dddf655 100644
--- a/block.c
+++ b/block.c
@@ -84,6 +84,9 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
BdrvChildRole child_role,
Error **errp);
+static bool bdrv_recurse_has_child(BlockDriverState *bs,
+ BlockDriverState *child);
+
static void bdrv_replace_child_noperm(BdrvChild *child,
BlockDriverState *new_bs);
static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
@@ -2673,6 +2676,7 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
int drain_saldo;
assert(!child->frozen);
+ assert(old_bs != new_bs);
if (old_bs && new_bs) {
assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));
@@ -2892,6 +2896,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
assert(parent_bs->drv);
+ if (bdrv_recurse_has_child(child_bs, parent_bs)) {
+ error_setg(errp, "Making '%s' a %s child of '%s' would create a cycle",
+ parent_bs->node_name, child_name, child_bs->node_name);
+ return -EINVAL;
+ }
+
bdrv_get_cumulative_perm(parent_bs, &perm, &shared_perm);
bdrv_child_perm(parent_bs, child_bs, NULL, child_role, NULL,
perm, shared_perm, &perm, &shared_perm);
diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
index d557522943..de74262a26 100755
--- a/tests/qemu-iotests/085
+++ b/tests/qemu-iotests/085
@@ -103,11 +103,18 @@ do_blockdev_add()
}
# ${1}: unique identifier for the snapshot filename
-add_snapshot_image()
+create_snapshot_image()
{
base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
TEST_IMG=$snapshot_file _make_test_img -u -b "${base_image}" -F $IMGFMT "$size"
+}
+
+# ${1}: unique identifier for the snapshot filename
+add_snapshot_image()
+{
+ snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
+ create_snapshot_image "$1"
do_blockdev_add "$1" "'backing': null, " "${snapshot_file}"
}
@@ -230,6 +237,28 @@ _make_test_img -b "${TEST_IMG}.base" -F $IMGFMT "$size"
do_blockdev_add ${SNAPSHOTS} "" "${TEST_IMG}"
blockdev_snapshot ${SNAPSHOTS} error
+echo
+echo === Invalid command - creating loops ===
+echo
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+add_snapshot_image ${SNAPSHOTS}
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+ 'arguments': { 'node':'snap_${SNAPSHOTS}',
+ 'overlay':'snap_${SNAPSHOTS}' }
+ }" "error"
+
+SNAPSHOTS=$((${SNAPSHOTS}+1))
+create_snapshot_image ${SNAPSHOTS}
+do_blockdev_add ${SNAPSHOTS} "'backing': 'snap_$((${SNAPSHOTS}-1))', " \
+ "${TEST_DIR}/${SNAPSHOTS}-${snapshot_virt0}"
+
+_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
+ 'arguments': { 'node':'snap_${SNAPSHOTS}',
+ 'overlay':'snap_$((${SNAPSHOTS}-1))' }
+ }" "error"
+
echo
echo === Invalid command - The node does not exist ===
echo
diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
index 1d4c565b6d..7750b3df5f 100644
--- a/tests/qemu-iotests/085.out
+++ b/tests/qemu-iotests/085.out
@@ -217,15 +217,42 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
'overlay':'snap_13' } }
{"error": {"class": "GenericError", "desc": "The overlay already has a backing image"}}
+=== Invalid command - creating loops ===
+
+Formatting 'TEST_DIR/14-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/13-snapshot-v0.IMGFMT backing_fmt=IMGFMT
+{ 'execute': 'blockdev-add', 'arguments':
+ { 'driver': 'IMGFMT', 'node-name': 'snap_14', 'backing': null,
+ 'file':
+ { 'driver': 'file', 'filename': 'TEST_DIR/14-snapshot-v0.IMGFMT',
+ 'node-name': 'file_14' } } }
+{"return": {}}
+{ 'execute': 'blockdev-snapshot',
+ 'arguments': { 'node':'snap_14',
+ 'overlay':'snap_14' }
+ }
+{"error": {"class": "GenericError", "desc": "Making 'snap_14' a backing child of 'snap_14' would create a cycle"}}
+Formatting 'TEST_DIR/15-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/14-snapshot-v0.IMGFMT backing_fmt=IMGFMT
+{ 'execute': 'blockdev-add', 'arguments':
+ { 'driver': 'IMGFMT', 'node-name': 'snap_15', 'backing': 'snap_14',
+ 'file':
+ { 'driver': 'file', 'filename': 'TEST_DIR/15-snapshot-v0.IMGFMT',
+ 'node-name': 'file_15' } } }
+{"return": {}}
+{ 'execute': 'blockdev-snapshot',
+ 'arguments': { 'node':'snap_15',
+ 'overlay':'snap_14' }
+ }
+{"error": {"class": "GenericError", "desc": "Making 'snap_14' a backing child of 'snap_15' would create a cycle"}}
+
=== Invalid command - The node does not exist ===
{ 'execute': 'blockdev-snapshot',
'arguments': { 'node': 'virtio0',
- 'overlay':'snap_14' } }
-{"error": {"class": "GenericError", "desc": "Cannot find device='snap_14' nor node-name='snap_14'"}}
+ 'overlay':'snap_16' } }
+{"error": {"class": "GenericError", "desc": "Cannot find device='snap_16' nor node-name='snap_16'"}}
{ 'execute': 'blockdev-snapshot',
'arguments': { 'node':'nodevice',
- 'overlay':'snap_13' }
+ 'overlay':'snap_15' }
}
{"error": {"class": "GenericError", "desc": "Cannot find device='nodevice' nor node-name='nodevice'"}}
*** done
--
2.31.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] block: Fail gracefully when blockdev-snapshot creates loops
2021-10-18 13:47 [PATCH] block: Fail gracefully when blockdev-snapshot creates loops Kevin Wolf
@ 2021-10-18 14:22 ` Vladimir Sementsov-Ogievskiy
2021-10-19 12:23 ` Kevin Wolf
0 siblings, 1 reply; 3+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-10-18 14:22 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: hreitz, qemu-devel
18.10.2021 16:47, Kevin Wolf wrote:
> Using blockdev-snapshot to append a node as an overlay to itself, or to
> any of its parents, causes crashes. Catch the condition and return an
> error for these cases instead.
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1824363
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block.c | 10 ++++++++++
> tests/qemu-iotests/085 | 31 ++++++++++++++++++++++++++++++-
> tests/qemu-iotests/085.out | 33 ++++++++++++++++++++++++++++++---
> 3 files changed, 70 insertions(+), 4 deletions(-)
>
> diff --git a/block.c b/block.c
> index 45f653a88b..231dddf655 100644
> --- a/block.c
> +++ b/block.c
> @@ -84,6 +84,9 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
> BdrvChildRole child_role,
> Error **errp);
>
> +static bool bdrv_recurse_has_child(BlockDriverState *bs,
> + BlockDriverState *child);
> +
> static void bdrv_replace_child_noperm(BdrvChild *child,
> BlockDriverState *new_bs);
> static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
> @@ -2673,6 +2676,7 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
> int drain_saldo;
>
> assert(!child->frozen);
> + assert(old_bs != new_bs);
>
> if (old_bs && new_bs) {
> assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));
> @@ -2892,6 +2896,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
>
> assert(parent_bs->drv);
>
> + if (bdrv_recurse_has_child(child_bs, parent_bs)) {
> + error_setg(errp, "Making '%s' a %s child of '%s' would create a cycle",
> + parent_bs->node_name, child_name, child_bs->node_name);
Seems, child_bs and parent_bs should be swapped.
with that fixed:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> + return -EINVAL;
> + }
> +
> bdrv_get_cumulative_perm(parent_bs, &perm, &shared_perm);
> bdrv_child_perm(parent_bs, child_bs, NULL, child_role, NULL,
> perm, shared_perm, &perm, &shared_perm);
> diff --git a/tests/qemu-iotests/085 b/tests/qemu-iotests/085
> index d557522943..de74262a26 100755
> --- a/tests/qemu-iotests/085
> +++ b/tests/qemu-iotests/085
> @@ -103,11 +103,18 @@ do_blockdev_add()
> }
>
> # ${1}: unique identifier for the snapshot filename
> -add_snapshot_image()
> +create_snapshot_image()
> {
> base_image="${TEST_DIR}/$((${1}-1))-${snapshot_virt0}"
> snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
> TEST_IMG=$snapshot_file _make_test_img -u -b "${base_image}" -F $IMGFMT "$size"
> +}
> +
> +# ${1}: unique identifier for the snapshot filename
> +add_snapshot_image()
> +{
> + snapshot_file="${TEST_DIR}/${1}-${snapshot_virt0}"
> + create_snapshot_image "$1"
> do_blockdev_add "$1" "'backing': null, " "${snapshot_file}"
> }
>
> @@ -230,6 +237,28 @@ _make_test_img -b "${TEST_IMG}.base" -F $IMGFMT "$size"
> do_blockdev_add ${SNAPSHOTS} "" "${TEST_IMG}"
> blockdev_snapshot ${SNAPSHOTS} error
>
> +echo
> +echo === Invalid command - creating loops ===
> +echo
> +
> +SNAPSHOTS=$((${SNAPSHOTS}+1))
> +add_snapshot_image ${SNAPSHOTS}
> +
> +_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
> + 'arguments': { 'node':'snap_${SNAPSHOTS}',
> + 'overlay':'snap_${SNAPSHOTS}' }
> + }" "error"
> +
> +SNAPSHOTS=$((${SNAPSHOTS}+1))
> +create_snapshot_image ${SNAPSHOTS}
> +do_blockdev_add ${SNAPSHOTS} "'backing': 'snap_$((${SNAPSHOTS}-1))', " \
> + "${TEST_DIR}/${SNAPSHOTS}-${snapshot_virt0}"
> +
> +_send_qemu_cmd $h "{ 'execute': 'blockdev-snapshot',
> + 'arguments': { 'node':'snap_${SNAPSHOTS}',
> + 'overlay':'snap_$((${SNAPSHOTS}-1))' }
> + }" "error"
> +
> echo
> echo === Invalid command - The node does not exist ===
> echo
> diff --git a/tests/qemu-iotests/085.out b/tests/qemu-iotests/085.out
> index 1d4c565b6d..7750b3df5f 100644
> --- a/tests/qemu-iotests/085.out
> +++ b/tests/qemu-iotests/085.out
> @@ -217,15 +217,42 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/
> 'overlay':'snap_13' } }
> {"error": {"class": "GenericError", "desc": "The overlay already has a backing image"}}
>
> +=== Invalid command - creating loops ===
> +
> +Formatting 'TEST_DIR/14-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/13-snapshot-v0.IMGFMT backing_fmt=IMGFMT
> +{ 'execute': 'blockdev-add', 'arguments':
> + { 'driver': 'IMGFMT', 'node-name': 'snap_14', 'backing': null,
> + 'file':
> + { 'driver': 'file', 'filename': 'TEST_DIR/14-snapshot-v0.IMGFMT',
> + 'node-name': 'file_14' } } }
> +{"return": {}}
> +{ 'execute': 'blockdev-snapshot',
> + 'arguments': { 'node':'snap_14',
> + 'overlay':'snap_14' }
> + }
> +{"error": {"class": "GenericError", "desc": "Making 'snap_14' a backing child of 'snap_14' would create a cycle"}}
> +Formatting 'TEST_DIR/15-snapshot-v0.IMGFMT', fmt=IMGFMT size=134217728 backing_file=TEST_DIR/14-snapshot-v0.IMGFMT backing_fmt=IMGFMT
> +{ 'execute': 'blockdev-add', 'arguments':
> + { 'driver': 'IMGFMT', 'node-name': 'snap_15', 'backing': 'snap_14',
> + 'file':
> + { 'driver': 'file', 'filename': 'TEST_DIR/15-snapshot-v0.IMGFMT',
> + 'node-name': 'file_15' } } }
> +{"return": {}}
> +{ 'execute': 'blockdev-snapshot',
> + 'arguments': { 'node':'snap_15',
> + 'overlay':'snap_14' }
> + }
> +{"error": {"class": "GenericError", "desc": "Making 'snap_14' a backing child of 'snap_15' would create a cycle"}}
> +
> === Invalid command - The node does not exist ===
>
> { 'execute': 'blockdev-snapshot',
> 'arguments': { 'node': 'virtio0',
> - 'overlay':'snap_14' } }
> -{"error": {"class": "GenericError", "desc": "Cannot find device='snap_14' nor node-name='snap_14'"}}
> + 'overlay':'snap_16' } }
> +{"error": {"class": "GenericError", "desc": "Cannot find device='snap_16' nor node-name='snap_16'"}}
> { 'execute': 'blockdev-snapshot',
> 'arguments': { 'node':'nodevice',
> - 'overlay':'snap_13' }
> + 'overlay':'snap_15' }
> }
> {"error": {"class": "GenericError", "desc": "Cannot find device='nodevice' nor node-name='nodevice'"}}
> *** done
>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] block: Fail gracefully when blockdev-snapshot creates loops
2021-10-18 14:22 ` Vladimir Sementsov-Ogievskiy
@ 2021-10-19 12:23 ` Kevin Wolf
0 siblings, 0 replies; 3+ messages in thread
From: Kevin Wolf @ 2021-10-19 12:23 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy; +Cc: hreitz, qemu-devel, qemu-block
Am 18.10.2021 um 16:22 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 18.10.2021 16:47, Kevin Wolf wrote:
> > Using blockdev-snapshot to append a node as an overlay to itself, or to
> > any of its parents, causes crashes. Catch the condition and return an
> > error for these cases instead.
> >
> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1824363
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > block.c | 10 ++++++++++
> > tests/qemu-iotests/085 | 31 ++++++++++++++++++++++++++++++-
> > tests/qemu-iotests/085.out | 33 ++++++++++++++++++++++++++++++---
> > 3 files changed, 70 insertions(+), 4 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index 45f653a88b..231dddf655 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -84,6 +84,9 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
> > BdrvChildRole child_role,
> > Error **errp);
> > +static bool bdrv_recurse_has_child(BlockDriverState *bs,
> > + BlockDriverState *child);
> > +
> > static void bdrv_replace_child_noperm(BdrvChild *child,
> > BlockDriverState *new_bs);
> > static void bdrv_remove_file_or_backing_child(BlockDriverState *bs,
> > @@ -2673,6 +2676,7 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
> > int drain_saldo;
> > assert(!child->frozen);
> > + assert(old_bs != new_bs);
> > if (old_bs && new_bs) {
> > assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));
> > @@ -2892,6 +2896,12 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,
> > assert(parent_bs->drv);
> > + if (bdrv_recurse_has_child(child_bs, parent_bs)) {
> > + error_setg(errp, "Making '%s' a %s child of '%s' would create a cycle",
> > + parent_bs->node_name, child_name, child_bs->node_name);
>
> Seems, child_bs and parent_bs should be swapped.
Oops, thanks. I'm fixing it up while applying.
Kevin
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-19 12:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-18 13:47 [PATCH] block: Fail gracefully when blockdev-snapshot creates loops Kevin Wolf
2021-10-18 14:22 ` Vladimir Sementsov-Ogievskiy
2021-10-19 12:23 ` Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.