[PATCH 0/5] Fix not white-listed copy-before-write

[PATCH 0/5] Fix not white-listed copy-before-write

[Vladimir Sementsov-Ogievskiy]

Hi all!

As reported in https://bugzilla.redhat.com/show_bug.cgi?id=2004812
backup don't work when copy-before-write is not white-listed.

Yes, we do need copy-before-write filter for backup to work (like we
always use copy-on-read filter in block-stream).
The problem is that in bdrv_insert_node() (called to insert filters
internally) we use bdrv_open(), which does a lot of things we don't need
for internal node creation and among them check the white-list.

Backup job should of course work when copy-before-write is not
white-listed. As well, block-stream should work with not-white-listed
copy-on-read. White-list is for user, not for internal implementation.

Following Kevin's suggestion fix the problem by implementing a version
of bdrv_new_open_driver() that supports QDict of options, and use it
instead of bdrv_open().

Vladimir Sementsov-Ogievskiy (5):
  block: implement bdrv_new_open_driver_opts()
  block: bdrv_insert_node(): fix and improve error handling
  block: bdrv_insert_node(): doc and style
  block: bdrv_insert_node(): don't use bdrv_open()
  iotests/image-fleecing: declare requirement of copy-before-write

 include/block/block.h                   |  4 ++
 block.c                                 | 77 ++++++++++++++++++++-----
 tests/qemu-iotests/tests/image-fleecing |  1 +
 3 files changed, 68 insertions(+), 14 deletions(-)

-- 
2.29.2

[PATCH 1/5] block: implement bdrv_new_open_driver_opts()

[Vladimir Sementsov-Ogievskiy]

Add version of bdrv_new_open_driver() that supports QDict options.
We'll use it in further commit.

Simply add one more argument to bdrv_new_open_driver() is worse, as
there are too many invocations of bdrv_new_open_driver() to update
then.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block.h |  4 ++++
 block.c               | 25 +++++++++++++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 740038a892..24b773e69c 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -383,6 +383,10 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, Error **errp);
 BlockDriverState *bdrv_open(const char *filename, const char *reference,
                             QDict *options, int flags, Error **errp);
+BlockDriverState *bdrv_new_open_driver_opts(BlockDriver *drv,
+                                            const char *node_name,
+                                            QDict *options, int flags,
+                                            Error **errp);
 BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
                                        int flags, Error **errp);
 BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue *bs_queue,
diff --git a/block.c b/block.c
index 5ce08a79fd..917fb7faca 100644
--- a/block.c
+++ b/block.c
@@ -1604,16 +1604,26 @@ open_failed:
     return ret;
 }
 
-BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
-                                       int flags, Error **errp)
+/*
+ * Create and open a block node.
+ *
+ * @options is a QDict of options to pass to the block drivers, or NULL for an
+ * empty set of options. The reference to the QDict belongs to the block layer
+ * after the call (even on failure), so if the caller intends to reuse the
+ * dictionary, it needs to use qobject_ref() before calling bdrv_open.
+ */
+BlockDriverState *bdrv_new_open_driver_opts(BlockDriver *drv,
+                                            const char *node_name,
+                                            QDict *options, int flags,
+                                            Error **errp)
 {
     BlockDriverState *bs;
     int ret;
 
     bs = bdrv_new();
     bs->open_flags = flags;
-    bs->explicit_options = qdict_new();
-    bs->options = qdict_new();
+    bs->options = options ?: qdict_new();
+    bs->explicit_options = qdict_clone_shallow(bs->options);
     bs->opaque = NULL;
 
     update_options_from_flags(bs->options, flags);
@@ -1631,6 +1641,13 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
     return bs;
 }
 
+/* Create and open a block node. */
+BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
+                                       int flags, Error **errp)
+{
+    return bdrv_new_open_driver_opts(drv, node_name, NULL, flags, errp);
+}
+
 QemuOptsList bdrv_runtime_opts = {
     .name = "bdrv_common",
     .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head),
-- 
2.29.2

[PATCH 2/5] block: bdrv_insert_node(): fix and improve error handling

[Vladimir Sementsov-Ogievskiy]

 - use ERRP_GUARD(): function calls error_prepend(), so it must use
   ERRP_GUARD(), otherwise error_prepend() would not be called when
   passed errp is error_fatal

 - drop error propagation, handle return code instead

 - for symmetry, do error_prepend() for the second failure

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 917fb7faca..5d49188073 100644
--- a/block.c
+++ b/block.c
@@ -5122,8 +5122,9 @@ static void bdrv_delete(BlockDriverState *bs)
 BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
                                    int flags, Error **errp)
 {
+    ERRP_GUARD();
+    int ret;
     BlockDriverState *new_node_bs;
-    Error *local_err = NULL;
 
     new_node_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
     if (new_node_bs == NULL) {
@@ -5132,12 +5133,12 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
     }
 
     bdrv_drained_begin(bs);
-    bdrv_replace_node(bs, new_node_bs, &local_err);
+    ret = bdrv_replace_node(bs, new_node_bs, errp);
     bdrv_drained_end(bs);
 
-    if (local_err) {
+    if (ret < 0) {
+        error_prepend(errp, "Could not replace node: ");
         bdrv_unref(new_node_bs);
-        error_propagate(errp, local_err);
         return NULL;
     }
 
-- 
2.29.2

[PATCH 3/5] block: bdrv_insert_node(): doc and style

[Vladimir Sementsov-Ogievskiy]

 - options & flags is common pair for open-like functions, let's use it
 - add a comment that specifies use of @options

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 5d49188073..3a90407b83 100644
--- a/block.c
+++ b/block.c
@@ -5119,14 +5119,23 @@ static void bdrv_delete(BlockDriverState *bs)
     g_free(bs);
 }
 
-BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options,
+
+/*
+ * Replace @bs by newly created block node.
+ *
+ * @options is a QDict of options to pass to the block drivers, or NULL for an
+ * empty set of options. The reference to the QDict belongs to the block layer
+ * after the call (even on failure), so if the caller intends to reuse the
+ * dictionary, it needs to use qobject_ref() before calling bdrv_open.
+ */
+BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options,
                                    int flags, Error **errp)
 {
     ERRP_GUARD();
     int ret;
     BlockDriverState *new_node_bs;
 
-    new_node_bs = bdrv_open(NULL, NULL, node_options, flags, errp);
+    new_node_bs = bdrv_open(NULL, NULL, options, flags, errp);
     if (new_node_bs == NULL) {
         error_prepend(errp, "Could not create node: ");
         return NULL;
-- 
2.29.2

[PATCH 4/5] block: bdrv_insert_node(): don't use bdrv_open()

[Vladimir Sementsov-Ogievskiy]

Use bdrv_new_open_driver_opts() instead of complicated bdrv_open().

Among other extra things bdrv_open() also check for white-listed
formats, which we don't want for internal node creation: currently
backup doesn't work when copy-before-write filter is not white-listed.
As well block-stream doesn't work when copy-on-read is not
white-listed.

Fixes: 751cec7a261adaf1145dc7adf6de7c9c084e5a0b
Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2004812
Reported-by: Yanan Fu
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 3a90407b83..a174801785 100644
--- a/block.c
+++ b/block.c
@@ -5134,11 +5134,29 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options,
     ERRP_GUARD();
     int ret;
     BlockDriverState *new_node_bs;
+    const char *drvname, *node_name;
+    BlockDriver *drv;
+
+    drvname = qdict_get_try_str(options, "driver");
+    if (!drvname) {
+        error_setg(errp, "driver is not specified");
+        goto fail;
+    }
+
+    drv = bdrv_find_format(drvname);
+    if (!drv) {
+        error_setg(errp, "Unknown driver: '%s'", drvname);
+        goto fail;
+    }
 
-    new_node_bs = bdrv_open(NULL, NULL, options, flags, errp);
-    if (new_node_bs == NULL) {
+    node_name = qdict_get_try_str(options, "node-name");
+
+    new_node_bs = bdrv_new_open_driver_opts(drv, node_name, options, flags,
+                                            errp);
+    options = NULL; /* bdrv_new_open_driver() eats options */
+    if (!new_node_bs) {
         error_prepend(errp, "Could not create node: ");
-        return NULL;
+        goto fail;
     }
 
     bdrv_drained_begin(bs);
@@ -5147,11 +5165,15 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options,
 
     if (ret < 0) {
         error_prepend(errp, "Could not replace node: ");
-        bdrv_unref(new_node_bs);
-        return NULL;
+        goto fail;
     }
 
     return new_node_bs;
+
+fail:
+    qobject_unref(options);
+    bdrv_unref(new_node_bs);
+    return NULL;
 }
 
 /*
-- 
2.29.2

[PATCH 5/5] iotests/image-fleecing: declare requirement of copy-before-write

[Vladimir Sementsov-Ogievskiy]

Now test fails if copy-before-write is not white-listed.
Let's skip test instead.

Fixes: c0605985696a19ef034fa25d04f53f3b3b383896
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/tests/image-fleecing | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tests/qemu-iotests/tests/image-fleecing b/tests/qemu-iotests/tests/image-fleecing
index f6318492c6..35164e9036 100755
--- a/tests/qemu-iotests/tests/image-fleecing
+++ b/tests/qemu-iotests/tests/image-fleecing
@@ -28,6 +28,7 @@ from iotests import log, qemu_img, qemu_io, qemu_io_silent
 iotests.script_initialize(
     supported_fmts=['qcow2', 'qcow', 'qed', 'vmdk', 'vhdx', 'raw'],
     supported_platforms=['linux'],
+    required_fmts=['copy-before-write'],
 )
 
 patterns = [('0x5d', '0',         '64k'),
-- 
2.29.2

Re: [PATCH 4/5] block: bdrv_insert_node(): don't use bdrv_open()

[Kevin Wolf]

Am 20.09.2021 um 13:55 hat Vladimir Sementsov-Ogievskiy geschrieben:
> Use bdrv_new_open_driver_opts() instead of complicated bdrv_open().
> 
> Among other extra things bdrv_open() also check for white-listed
> formats, which we don't want for internal node creation: currently
> backup doesn't work when copy-before-write filter is not white-listed.
> As well block-stream doesn't work when copy-on-read is not
> white-listed.
> 
> Fixes: 751cec7a261adaf1145dc7adf6de7c9c084e5a0b
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2004812
> Reported-by: Yanan Fu
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 3a90407b83..a174801785 100644
> --- a/block.c
> +++ b/block.c
> @@ -5134,11 +5134,29 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options,
>      ERRP_GUARD();
>      int ret;
>      BlockDriverState *new_node_bs;

gcc tells me that this needs to be initialised now because of the
bdrv_unref() aftert the fail: label.

Fixed this up to be new_node_bs = NULL and applied the series to the
block branch, thanks.

Kevin