[Qemu-devel] [PULL 0/8] Block layer patches

[Qemu-devel] [PULL 0/8] Block layer patches

[Kevin Wolf]

The following changes since commit b55a69fe5f0a504dac6359bb7e99a72b130c3661:

  Merge remote-tracking branch 'remotes/juanquintela/tags/migration/20170607' into staging (2017-06-07 15:06:42 +0100)

are available in the git repository at:


  git://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 2cb8f869e43406ce829ebe3b16fecc78d367dc7f:

  block: fix external snapshot abort permission error (2017-06-07 18:51:51 +0200)

----------------------------------------------------------------
Block layer patches

----------------------------------------------------------------
Jeff Cody (1):
      block: fix external snapshot abort permission error

Kevin Wolf (6):
      block: Fix anonymous BBs in blk_root_inactivate()
      migration: Inactivate images after .save_live_complete_precopy()
      migration/block: Clean up BBs in block_save_complete()
      qemu-iotests: Block migration test
      commit: Fix use after free in completion
      qemu-iotests: Test automatic commit job cancel on hot unplug

Peter Maydell (1):
      block/qcow.c: Fix memory leak in qcow_create()

 block/block-backend.c      |   2 +-
 block/commit.c             |   7 +++
 block/qcow.c               |   1 +
 blockdev.c                 |   4 ++
 migration/block.c          |  22 +++++--
 migration/migration.c      |  12 ++--
 tests/qemu-iotests/040     |  35 +++++++++++-
 tests/qemu-iotests/040.out |   4 +-
 tests/qemu-iotests/183     | 140 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/183.out |  46 +++++++++++++++
 tests/qemu-iotests/group   |   1 +
 11 files changed, 259 insertions(+), 15 deletions(-)
 create mode 100755 tests/qemu-iotests/183
 create mode 100644 tests/qemu-iotests/183.out

[Qemu-devel] [PULL 1/8] block: Fix anonymous BBs in blk_root_inactivate()

[Kevin Wolf]

blk->name isn't an array, but a pointer that can be NULL. Checking for
an anonymous BB must involve a NULL check first, otherwise we get
crashes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
 block/block-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index f3a6008..7d7f369 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -168,7 +168,7 @@ static int blk_root_inactivate(BdrvChild *child)
      * this point because the VM is stopped) and unattached monitor-owned
      * BlockBackends. If there is still any other user like a block job, then
      * we simply can't inactivate the image. */
-    if (!blk->dev && !blk->name[0]) {
+    if (!blk->dev && !blk_name(blk)[0]) {
         return -EPERM;
     }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 2/8] migration: Inactivate images after .save_live_complete_precopy()

[Kevin Wolf]

Block migration may still access the image during its
.save_live_complete_precopy() implementation, so we should only
inactivate the image afterwards.

Another reason for the change is that inactivating an image fails when
there is still a non-device BlockBackend using it, which includes the
BBs used by block migration. We want to give block migration a chance to
release the BBs before trying to inactivate the image (this will be done
in another patch).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
 migration/migration.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 6f0705a..fc95acb 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1825,17 +1825,19 @@ static void migration_completion(MigrationState *s, int current_active_state,
 
         if (!ret) {
             ret = vm_stop_force_state(RUN_STATE_FINISH_MIGRATE);
+            if (ret >= 0) {
+                qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
+                qemu_savevm_state_complete_precopy(s->to_dst_file, false);
+            }
             /*
              * Don't mark the image with BDRV_O_INACTIVE flag if
              * we will go into COLO stage later.
              */
             if (ret >= 0 && !migrate_colo_enabled()) {
                 ret = bdrv_inactivate_all();
-            }
-            if (ret >= 0) {
-                qemu_file_set_rate_limit(s->to_dst_file, INT64_MAX);
-                qemu_savevm_state_complete_precopy(s->to_dst_file, false);
-                s->block_inactive = true;
+                if (ret >= 0) {
+                    s->block_inactive = true;
+                }
             }
         }
         qemu_mutex_unlock_iothread();
-- 
1.8.3.1

[Qemu-devel] [PULL 3/8] migration/block: Clean up BBs in block_save_complete()

[Kevin Wolf]

We need to release any block migrations BlockBackends on the source
before successfully completing the migration because otherwise
inactivating the images will fail (inactivation only tolerates device
BBs).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
---
 migration/block.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index 4d8c2e9..114cedb 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -674,16 +674,14 @@ static int64_t get_remaining_dirty(void)
     return dirty << BDRV_SECTOR_BITS;
 }
 
-/* Called with iothread lock taken.  */
 
-static void block_migration_cleanup(void *opaque)
+
+/* Called with iothread lock taken.  */
+static void block_migration_cleanup_bmds(void)
 {
     BlkMigDevState *bmds;
-    BlkMigBlock *blk;
     AioContext *ctx;
 
-    bdrv_drain_all();
-
     unset_dirty_tracking();
 
     while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
@@ -701,6 +699,16 @@ static void block_migration_cleanup(void *opaque)
         g_free(bmds->aio_bitmap);
         g_free(bmds);
     }
+}
+
+/* Called with iothread lock taken.  */
+static void block_migration_cleanup(void *opaque)
+{
+    BlkMigBlock *blk;
+
+    bdrv_drain_all();
+
+    block_migration_cleanup_bmds();
 
     blk_mig_lock();
     while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) {
@@ -844,6 +852,10 @@ static int block_save_complete(QEMUFile *f, void *opaque)
 
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
 
+    /* Make sure that our BlockBackends are gone, so that the block driver
+     * nodes can be inactivated. */
+    block_migration_cleanup_bmds();
+
     return 0;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 4/8] qemu-iotests: Block migration test

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
 tests/qemu-iotests/183     | 140 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/183.out |  46 +++++++++++++++
 tests/qemu-iotests/group   |   1 +
 3 files changed, 187 insertions(+)
 create mode 100755 tests/qemu-iotests/183
 create mode 100644 tests/qemu-iotests/183.out

diff --git a/tests/qemu-iotests/183 b/tests/qemu-iotests/183
new file mode 100755
index 0000000..20268ff
--- /dev/null
+++ b/tests/qemu-iotests/183
@@ -0,0 +1,140 @@
+#!/bin/bash
+#
+# Test old-style block migration (migrate -b)
+#
+# Copyright (C) 2017 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+status=1 # failure is the default!
+
+MIG_SOCKET="${TEST_DIR}/migrate"
+
+_cleanup()
+{
+    rm -f "${MIG_SOCKET}"
+    rm -f "${TEST_IMG}.dest"
+    _cleanup_test_img
+    _cleanup_qemu
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+. ./common.qemu
+
+_supported_fmt qcow2 raw qed dmg quorum
+_supported_proto file
+_supported_os Linux
+
+size=64M
+_make_test_img $size
+TEST_IMG="${TEST_IMG}.dest" _make_test_img $size
+
+echo
+echo === Starting VMs ===
+echo
+
+qemu_comm_method="qmp"
+
+_launch_qemu \
+    -drive file="${TEST_IMG}",cache=$CACHEMODE,driver=$IMGFMT,id=disk
+src=$QEMU_HANDLE
+_send_qemu_cmd $src "{ 'execute': 'qmp_capabilities' }" 'return'
+
+_launch_qemu \
+    -drive file="${TEST_IMG}.dest",cache=$CACHEMODE,driver=$IMGFMT,id=disk \
+    -incoming "unix:${MIG_SOCKET}"
+dest=$QEMU_HANDLE
+_send_qemu_cmd $dest "{ 'execute': 'qmp_capabilities' }" 'return'
+
+echo
+echo === Write something on the source ===
+echo
+
+_send_qemu_cmd $src \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io disk \"write -P 0x55 0 64k\"' } }" \
+    'return'
+_send_qemu_cmd $src \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io disk \"read -P 0x55 0 64k\"' } }" \
+    'return'
+
+echo
+echo === Do block migration to destination ===
+echo
+
+reply="$(_send_qemu_cmd $src \
+    "{ 'execute': 'migrate',
+       'arguments': { 'uri': 'unix:${MIG_SOCKET}', 'blk': true } }" \
+    'return\|error')"
+echo "$reply"
+if echo "$reply" | grep "compiled without old-style" > /dev/null; then
+    _notrun "migrate -b support not compiled in"
+fi
+
+QEMU_COMM_TIMEOUT=0.1 qemu_cmd_repeat=50 silent=yes \
+    _send_qemu_cmd $src "{ 'execute': 'query-migrate' }" '"status": "completed"'
+_send_qemu_cmd $src "{ 'execute': 'query-status' }" "return"
+
+echo
+echo === Do some I/O on the destination ===
+echo
+
+# It is important that we use the BlockBackend of the guest device here instead
+# of the node name, which would create a new BlockBackend and not test whether
+# the guest has the necessary permissions to access the image now
+silent=yes _send_qemu_cmd $dest "" "100 %"
+_send_qemu_cmd $dest "{ 'execute': 'query-status' }" "return"
+_send_qemu_cmd $dest \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io disk \"read -P 0x55 0 64k\"' } }" \
+    'return'
+_send_qemu_cmd $dest \
+    "{ 'execute': 'human-monitor-command',
+       'arguments': { 'command-line':
+                      'qemu-io disk \"write -P 0x66 1M 64k\"' } }" \
+    'return'
+
+echo
+echo === Shut down and check image ===
+echo
+
+_send_qemu_cmd $src '{"execute":"quit"}' 'return'
+_send_qemu_cmd $dest '{"execute":"quit"}' 'return'
+wait=1 _cleanup_qemu
+
+_check_test_img
+TEST_IMG="${TEST_IMG}.dest" _check_test_img
+
+$QEMU_IO -c 'write -P 0x66 1M 64k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG compare "$TEST_IMG.dest" "$TEST_IMG"
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/183.out b/tests/qemu-iotests/183.out
new file mode 100644
index 0000000..103fdc7
--- /dev/null
+++ b/tests/qemu-iotests/183.out
@@ -0,0 +1,46 @@
+QA output created by 183
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+Formatting 'TEST_DIR/t.IMGFMT.dest', fmt=IMGFMT size=67108864
+
+=== Starting VMs ===
+
+{"return": {}}
+{"return": {}}
+
+=== Write something on the source ===
+
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+
+=== Do block migration to destination ===
+
+{"return": {}}
+{"return": {"status": "postmigrate", "singlestep": false, "running": false}}
+
+=== Do some I/O on the destination ===
+
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "RESUME"}
+{"return": {"status": "running", "singlestep": false, "running": true}}
+read 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+wrote 65536/65536 bytes at offset 1048576
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+{"return": ""}
+
+=== Shut down and check image ===
+
+{"return": {}}
+{"return": {}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+{"timestamp": {"seconds":  TIMESTAMP, "microseconds":  TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false}}
+No errors were found on the image.
+No errors were found on the image.
+wrote 65536/65536 bytes at offset 1048576
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Images are identical.
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 5c8ea0f..a6acaff 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -174,3 +174,4 @@
 179 rw auto quick
 181 rw auto migration
 182 rw auto quick
+183 rw auto migration
-- 
1.8.3.1

[Qemu-devel] [PULL 5/8] commit: Fix use after free in completion

[Kevin Wolf]

The final bdrv_set_backing_hd() could be working on already freed nodes
because the commit job drops its references (through BlockBackends) to
both overlay_bs and top already a bit earlier.

One way to trigger the bug is hot unplugging a disk for which
blockdev_mark_auto_del() cancels the block job.

Fix this by taking BDS-level references while we're still using the
nodes.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block/commit.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/commit.c b/block/commit.c
index a3028b2..af6fa68 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
     int ret = data->ret;
     bool remove_commit_top_bs = false;
 
+    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
+    bdrv_ref(top);
+    bdrv_ref(overlay_bs);
+
     /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
      * the normal backing chain can be restored. */
     blk_unref(s->base);
@@ -124,6 +128,9 @@ static void commit_complete(BlockJob *job, void *opaque)
     if (remove_commit_top_bs) {
         bdrv_set_backing_hd(overlay_bs, top, &error_abort);
     }
+
+    bdrv_unref(overlay_bs);
+    bdrv_unref(top);
 }
 
 static void coroutine_fn commit_run(void *opaque)
-- 
1.8.3.1

[Qemu-devel] [PULL 6/8] qemu-iotests: Test automatic commit job cancel on hot unplug

[Kevin Wolf]

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 tests/qemu-iotests/040     | 35 +++++++++++++++++++++++++++++++++--
 tests/qemu-iotests/040.out |  4 ++--
 2 files changed, 35 insertions(+), 4 deletions(-)

diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
index 5bdaf3d..9d381d9 100755
--- a/tests/qemu-iotests/040
+++ b/tests/qemu-iotests/040
@@ -70,7 +70,9 @@ class ImageCommitTestCase(iotests.QMPTestCase):
         self.wait_for_complete()
 
 class TestSingleDrive(ImageCommitTestCase):
-    image_len = 1 * 1024 * 1024
+    # Need some space after the copied data so that throttling is effective in
+    # tests that use it rather than just completing the job immediately
+    image_len = 2 * 1024 * 1024
     test_len = 1 * 1024 * 256
 
     def setUp(self):
@@ -79,7 +81,9 @@ class TestSingleDrive(ImageCommitTestCase):
         qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % mid_img, test_img)
         qemu_io('-f', 'raw', '-c', 'write -P 0xab 0 524288', backing_img)
         qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xef 524288 524288', mid_img)
-        self.vm = iotests.VM().add_drive(test_img)
+        self.vm = iotests.VM().add_drive(test_img, interface="none")
+        self.vm.add_device("virtio-scsi-pci")
+        self.vm.add_device("scsi-hd,id=scsi0,drive=drive0")
         self.vm.launch()
 
     def tearDown(self):
@@ -131,6 +135,33 @@ class TestSingleDrive(ImageCommitTestCase):
         self.assert_qmp(result, 'error/class', 'GenericError')
         self.assert_qmp(result, 'error/desc', 'Base \'%s\' not found' % mid_img)
 
+    # When the job is running on a BB that is automatically deleted on hot
+    # unplug, the job is cancelled when the device disappears
+    def test_hot_unplug(self):
+        if self.image_len == 0:
+            return
+
+        self.assert_no_active_block_jobs()
+        result = self.vm.qmp('block-commit', device='drive0', top=mid_img,
+                             base=backing_img, speed=(self.image_len / 4))
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp('device_del', id='scsi0')
+        self.assert_qmp(result, 'return', {})
+
+        cancelled = False
+        deleted = False
+        while not cancelled or not deleted:
+            for event in self.vm.get_qmp_events(wait=True):
+                if event['event'] == 'DEVICE_DELETED':
+                    self.assert_qmp(event, 'data/device', 'scsi0')
+                    deleted = True
+                elif event['event'] == 'BLOCK_JOB_CANCELLED':
+                    self.assert_qmp(event, 'data/device', 'drive0')
+                    cancelled = True
+                else:
+                    self.fail("Unexpected event %s" % (event['event']))
+
+        self.assert_no_active_block_jobs()
 
 class TestRelativePaths(ImageCommitTestCase):
     image_len = 1 * 1024 * 1024
diff --git a/tests/qemu-iotests/040.out b/tests/qemu-iotests/040.out
index 4fd1c2d..6d9bee1 100644
--- a/tests/qemu-iotests/040.out
+++ b/tests/qemu-iotests/040.out
@@ -1,5 +1,5 @@
-.........................
+...........................
 ----------------------------------------------------------------------
-Ran 25 tests
+Ran 27 tests
 
 OK
-- 
1.8.3.1

[Qemu-devel] [PULL 7/8] block/qcow.c: Fix memory leak in qcow_create()

[Kevin Wolf]

From: Peter Maydell <peter.maydell@linaro.org>

Coverity points out that the code path in qcow_create() for
the magic "fat:" backing file name leaks the memory used to
store the filename (CID 1307771). Free the memory before
we overwrite the pointer.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/qcow.c b/block/qcow.c
index 95ab123..7bd94dc 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -852,6 +852,7 @@ static int qcow_create(const char *filename, QemuOpts *opts, Error **errp)
             header_size += backing_filename_len;
         } else {
             /* special backing file for vvfat */
+            g_free(backing_file);
             backing_file = NULL;
         }
         header.cluster_bits = 9; /* 512 byte cluster to avoid copying
-- 
1.8.3.1

[Qemu-devel] [PULL 8/8] block: fix external snapshot abort permission error

[Kevin Wolf]

From: Jeff Cody <jcody@redhat.com>

In external_snapshot_abort(), we try to undo what was done in
external_snapshot_prepare() calling bdrv_replace_node() to swap the
nodes back.  However, we receive a permissions error as writers are
blocked on the old node, which is now the new node backing file.

An easy fix (initially suggested by Kevin Wolf) is to call
bdrv_set_backing_hd() on the new node, to set the backing node to NULL.

Signed-off-by: Jeff Cody <jcody@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 blockdev.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/blockdev.c b/blockdev.c
index 892d768..6472548 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1803,7 +1803,11 @@ static void external_snapshot_abort(BlkActionState *common)
                              DO_UPCAST(ExternalSnapshotState, common, common);
     if (state->new_bs) {
         if (state->overlay_appended) {
+            bdrv_ref(state->old_bs);   /* we can't let bdrv_set_backind_hd()
+                                          close state->old_bs; we need it */
+            bdrv_set_backing_hd(state->new_bs, NULL, &error_abort);
             bdrv_replace_node(state->new_bs, state->old_bs, &error_abort);
+            bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */
         }
     }
 }
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 0/8] Block layer patches

[Peter Maydell]

On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
> The following changes since commit b55a69fe5f0a504dac6359bb7e99a72b130c3661:
>
>   Merge remote-tracking branch 'remotes/juanquintela/tags/migration/20170607' into staging (2017-06-07 15:06:42 +0100)
>
> are available in the git repository at:
>
>
>   git://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 2cb8f869e43406ce829ebe3b16fecc78d367dc7f:
>
>   block: fix external snapshot abort permission error (2017-06-07 18:51:51 +0200)
>
> ----------------------------------------------------------------
> Block layer patches
>
> ----------------------------------------------------------------
> Jeff Cody (1):
>       block: fix external snapshot abort permission error
>
> Kevin Wolf (6):
>       block: Fix anonymous BBs in blk_root_inactivate()
>       migration: Inactivate images after .save_live_complete_precopy()
>       migration/block: Clean up BBs in block_save_complete()
>       qemu-iotests: Block migration test
>       commit: Fix use after free in completion
>       qemu-iotests: Test automatic commit job cancel on hot unplug
>
> Peter Maydell (1):
>       block/qcow.c: Fix memory leak in qcow_create()

Applied, thanks.

-- PMM

Re: [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion

[Peter Maydell]

On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
> The final bdrv_set_backing_hd() could be working on already freed nodes
> because the commit job drops its references (through BlockBackends) to
> both overlay_bs and top already a bit earlier.
>
> One way to trigger the bug is hot unplugging a disk for which
> blockdev_mark_auto_del() cancels the block job.
>
> Fix this by taking BDS-level references while we're still using the
> nodes.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  block/commit.c | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/block/commit.c b/block/commit.c
> index a3028b2..af6fa68 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
>      int ret = data->ret;
>      bool remove_commit_top_bs = false;
>
> +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
> +    bdrv_ref(top);
> +    bdrv_ref(overlay_bs);
> +
>      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>       * the normal backing chain can be restored. */
>      blk_unref(s->base);

Hi -- coverity complains about this change, because bdrv_ref()
assumes that its argument is not NULL, but later on in commit_complete()
we have a check
    "if (overlay_bs && ...)"
which assumes its argument might be NULL. (CID 1376205)

Which is correct?

thanks
-- PMM

Re: [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion

[Kevin Wolf]

Am 13.06.2017 um 18:12 hat Peter Maydell geschrieben:
> On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
> > diff --git a/block/commit.c b/block/commit.c
> > index a3028b2..af6fa68 100644
> > --- a/block/commit.c
> > +++ b/block/commit.c
> > @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
> >      int ret = data->ret;
> >      bool remove_commit_top_bs = false;
> >
> > +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
> > +    bdrv_ref(top);
> > +    bdrv_ref(overlay_bs);
> > +
> >      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> >       * the normal backing chain can be restored. */
> >      blk_unref(s->base);
> 
> Hi -- coverity complains about this change, because bdrv_ref()
> assumes that its argument is not NULL, but later on in commit_complete()
> we have a check
>     "if (overlay_bs && ...)"
> which assumes its argument might be NULL. (CID 1376205)
> 
> Which is correct?

I saw the Coverity report and am looking into it. It's not completely
clear to me yet which is correct, but I suspect it can be NULL.

Kevin

Re: [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion

[Peter Maydell]

On 13 June 2017 at 17:46, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 13.06.2017 um 18:12 hat Peter Maydell geschrieben:
>> On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
>> > diff --git a/block/commit.c b/block/commit.c
>> > index a3028b2..af6fa68 100644
>> > --- a/block/commit.c
>> > +++ b/block/commit.c
>> > @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
>> >      int ret = data->ret;
>> >      bool remove_commit_top_bs = false;
>> >
>> > +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
>> > +    bdrv_ref(top);
>> > +    bdrv_ref(overlay_bs);
>> > +
>> >      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
>> >       * the normal backing chain can be restored. */
>> >      blk_unref(s->base);
>>
>> Hi -- coverity complains about this change, because bdrv_ref()
>> assumes that its argument is not NULL, but later on in commit_complete()
>> we have a check
>>     "if (overlay_bs && ...)"
>> which assumes its argument might be NULL. (CID 1376205)
>>
>> Which is correct?
>
> I saw the Coverity report and am looking into it. It's not completely
> clear to me yet which is correct, but I suspect it can be NULL.

Just a nudge on this one -- I don't think there's been a patch sent
to the list for this check-after-use ?

(It's one of just 7 coverity issues left which haven't had at least
a patch sent to the list now...)

thanks
-- PMM

Re: [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion

[Kevin Wolf]

Am 09.07.2017 um 19:09 hat Peter Maydell geschrieben:
> On 13 June 2017 at 17:46, Kevin Wolf <kwolf@redhat.com> wrote:
> > Am 13.06.2017 um 18:12 hat Peter Maydell geschrieben:
> >> On 7 June 2017 at 18:50, Kevin Wolf <kwolf@redhat.com> wrote:
> >> > diff --git a/block/commit.c b/block/commit.c
> >> > index a3028b2..af6fa68 100644
> >> > --- a/block/commit.c
> >> > +++ b/block/commit.c
> >> > @@ -89,6 +89,10 @@ static void commit_complete(BlockJob *job, void *opaque)
> >> >      int ret = data->ret;
> >> >      bool remove_commit_top_bs = false;
> >> >
> >> > +    /* Make sure overlay_bs and top stay around until bdrv_set_backing_hd() */
> >> > +    bdrv_ref(top);
> >> > +    bdrv_ref(overlay_bs);
> >> > +
> >> >      /* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
> >> >       * the normal backing chain can be restored. */
> >> >      blk_unref(s->base);
> >>
> >> Hi -- coverity complains about this change, because bdrv_ref()
> >> assumes that its argument is not NULL, but later on in commit_complete()
> >> we have a check
> >>     "if (overlay_bs && ...)"
> >> which assumes its argument might be NULL. (CID 1376205)
> >>
> >> Which is correct?
> >
> > I saw the Coverity report and am looking into it. It's not completely
> > clear to me yet which is correct, but I suspect it can be NULL.
> 
> Just a nudge on this one -- I don't think there's been a patch sent
> to the list for this check-after-use ?
> 
> (It's one of just 7 coverity issues left which haven't had at least
> a patch sent to the list now...)

As far as I can tell, this can't currently be triggered. I intended to
fix it with some work on the commit block job that I need to do anyway,
and which would potentially enable a way to trigger it. But it turned
out that this is a bit more complicated than I thought.

So maybe I'd better just post a very small patch that silences Coverity
(without making a practical difference) until I can finish the real
thing.

Kevin