* [Qemu-devel] [PULL 1/8] block: Fix anonymous BBs in blk_root_inactivate()
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
@ 2017-06-07 17:50 ` Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 2/8] migration: Inactivate images after .save_live_complete_precopy() Kevin Wolf
` (7 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2017-06-07 17:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PULL 2/8] migration: Inactivate images after .save_live_complete_precopy()
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 1/8] block: Fix anonymous BBs in blk_root_inactivate() Kevin Wolf
@ 2017-06-07 17:50 ` Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 3/8] migration/block: Clean up BBs in block_save_complete() Kevin Wolf
` (6 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2017-06-07 17:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PULL 3/8] migration/block: Clean up BBs in block_save_complete()
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 1/8] block: Fix anonymous BBs in blk_root_inactivate() Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 2/8] migration: Inactivate images after .save_live_complete_precopy() Kevin Wolf
@ 2017-06-07 17:50 ` Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 4/8] qemu-iotests: Block migration test Kevin Wolf
` (5 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2017-06-07 17:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PULL 4/8] qemu-iotests: Block migration test
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
` (2 preceding siblings ...)
2017-06-07 17:50 ` [Qemu-devel] [PULL 3/8] migration/block: Clean up BBs in block_save_complete() Kevin Wolf
@ 2017-06-07 17:50 ` Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion Kevin Wolf
` (4 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2017-06-07 17:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
` (3 preceding siblings ...)
2017-06-07 17:50 ` [Qemu-devel] [PULL 4/8] qemu-iotests: Block migration test Kevin Wolf
@ 2017-06-07 17:50 ` Kevin Wolf
2017-06-13 16:12 ` Peter Maydell
2017-06-07 17:50 ` [Qemu-devel] [PULL 6/8] qemu-iotests: Test automatic commit job cancel on hot unplug Kevin Wolf
` (3 subsequent siblings)
8 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2017-06-07 17:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion
2017-06-07 17:50 ` [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion Kevin Wolf
@ 2017-06-13 16:12 ` Peter Maydell
2017-06-13 16:46 ` Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2017-06-13 16:12 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion
2017-06-13 16:12 ` Peter Maydell
@ 2017-06-13 16:46 ` Kevin Wolf
2017-07-09 17:09 ` Peter Maydell
0 siblings, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2017-06-13 16:46 UTC (permalink / raw)
To: Peter Maydell; +Cc: Qemu-block, QEMU Developers
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion
2017-06-13 16:46 ` Kevin Wolf
@ 2017-07-09 17:09 ` Peter Maydell
2017-07-10 11:40 ` Kevin Wolf
0 siblings, 1 reply; 14+ messages in thread
From: Peter Maydell @ 2017-07-09 17:09 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion
2017-07-09 17:09 ` Peter Maydell
@ 2017-07-10 11:40 ` Kevin Wolf
0 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2017-07-10 11:40 UTC (permalink / raw)
To: Peter Maydell; +Cc: Qemu-block, QEMU Developers
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PULL 6/8] qemu-iotests: Test automatic commit job cancel on hot unplug
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
` (4 preceding siblings ...)
2017-06-07 17:50 ` [Qemu-devel] [PULL 5/8] commit: Fix use after free in completion Kevin Wolf
@ 2017-06-07 17:50 ` Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 7/8] block/qcow.c: Fix memory leak in qcow_create() Kevin Wolf
` (2 subsequent siblings)
8 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2017-06-07 17:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PULL 7/8] block/qcow.c: Fix memory leak in qcow_create()
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
` (5 preceding siblings ...)
2017-06-07 17:50 ` [Qemu-devel] [PULL 6/8] qemu-iotests: Test automatic commit job cancel on hot unplug Kevin Wolf
@ 2017-06-07 17:50 ` Kevin Wolf
2017-06-07 17:50 ` [Qemu-devel] [PULL 8/8] block: fix external snapshot abort permission error Kevin Wolf
2017-06-12 10:22 ` [Qemu-devel] [PULL 0/8] Block layer patches Peter Maydell
8 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2017-06-07 17:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PULL 8/8] block: fix external snapshot abort permission error
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
` (6 preceding siblings ...)
2017-06-07 17:50 ` [Qemu-devel] [PULL 7/8] block/qcow.c: Fix memory leak in qcow_create() Kevin Wolf
@ 2017-06-07 17:50 ` Kevin Wolf
2017-06-12 10:22 ` [Qemu-devel] [PULL 0/8] Block layer patches Peter Maydell
8 siblings, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2017-06-07 17:50 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, qemu-devel
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PULL 0/8] Block layer patches
2017-06-07 17:50 [Qemu-devel] [PULL 0/8] Block layer patches Kevin Wolf
` (7 preceding siblings ...)
2017-06-07 17:50 ` [Qemu-devel] [PULL 8/8] block: fix external snapshot abort permission error Kevin Wolf
@ 2017-06-12 10:22 ` Peter Maydell
8 siblings, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2017-06-12 10:22 UTC (permalink / raw)
To: Kevin Wolf; +Cc: Qemu-block, QEMU Developers
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
^ permalink raw reply [flat|nested] 14+ messages in thread