1. Patchew
  2. QEMU
  3. View series

[PATCH] block: Fail gracefully when blockdev-snapshot creates loops

Kevin Wolf posted 1 patch 2 years, 6 months ago
Test checkpatch passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20211018134714.48438-1-kwolf@redhat.com
Maintainers: Kevin Wolf <kwolf@redhat.com>, Hanna Reitz <hreitz@redhat.com>
block.c                    | 10 ++++++++++
tests/qemu-iotests/085     | 31 ++++++++++++++++++++++++++++++-
tests/qemu-iotests/085.out | 33 ++++++++++++++++++++++++++++++---
3 files changed, 70 insertions(+), 4 deletions(-)
[PATCH] block: Fail gracefully when blockdev-snapshot creates loops
Posted by Kevin Wolf 2 years, 6 months ago 
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
Re: [PATCH] block: Fail gracefully when blockdev-snapshot creates loops
Posted by Vladimir Sementsov-Ogievskiy 2 years, 6 months ago 
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
Re: [PATCH] block: Fail gracefully when blockdev-snapshot creates loops
Posted by Kevin Wolf 2 years, 6 months ago 
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

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.