* [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration
@ 2021-03-22 9:49 Vladimir Sementsov-Ogievskiy
2021-03-22 9:49 ` [PATCH 1/2] migration/block-dirty-bitmap: make incoming disabled bitmaps busy Vladimir Sementsov-Ogievskiy
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-03-22 9:49 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, dgilbert, quintela, fam, stefanha,
jsnow, vsementsov, eblake
Hi all! Accidentally we found on use-after-free. Normally user should
not remove bitmaps during migration.. But some wrong user actions may
simply lead to Qemu crash and that's not good.
Vladimir Sementsov-Ogievskiy (2):
migration/block-dirty-bitmap: make incoming disabled bitmaps busy
migrate-bitmaps-postcopy-test: check that we can't remove in-flight
bitmaps
migration/block-dirty-bitmap.c | 6 ++++++
tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
2 files changed, 16 insertions(+)
--
2.29.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] migration/block-dirty-bitmap: make incoming disabled bitmaps busy
2021-03-22 9:49 [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration Vladimir Sementsov-Ogievskiy
@ 2021-03-22 9:49 ` Vladimir Sementsov-Ogievskiy
2021-03-22 9:49 ` [PATCH 2/2] migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps Vladimir Sementsov-Ogievskiy
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-03-22 9:49 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, dgilbert, quintela, fam, stefanha,
jsnow, vsementsov, eblake
Incoming enabled bitmaps are busy, because we do
bdrv_dirty_bitmap_create_successor() for them. But disabled bitmaps
being migrated are not marked busy, and user can remove them during the
incoming migration. Then we may crash in cancel_incoming_locked() when
try to remove the bitmap that was already removed by user, like this:
#0 qemu_mutex_lock_impl (mutex=0x5593d88c50d1, file=0x559680554b20
"../block/dirty-bitmap.c", line=64) at ../util/qemu-thread-posix.c:77
#1 bdrv_dirty_bitmaps_lock (bs=0x5593d88c0ee9)
at ../block/dirty-bitmap.c:64
#2 bdrv_release_dirty_bitmap (bitmap=0x5596810e9570)
at ../block/dirty-bitmap.c:362
#3 cancel_incoming_locked (s=0x559680be8208 <dbm_state+40>)
at ../migration/block-dirty-bitmap.c:918
#4 dirty_bitmap_load (f=0x559681d02b10, opaque=0x559680be81e0
<dbm_state>, version_id=1) at ../migration/block-dirty-bitmap.c:1194
#5 vmstate_load (f=0x559681d02b10, se=0x559680fb5810)
at ../migration/savevm.c:908
#6 qemu_loadvm_section_part_end (f=0x559681d02b10,
mis=0x559680fb4a30) at ../migration/savevm.c:2473
#7 qemu_loadvm_state_main (f=0x559681d02b10, mis=0x559680fb4a30)
at ../migration/savevm.c:2626
#8 postcopy_ram_listen_thread (opaque=0x0)
at ../migration/savevm.c:1871
#9 qemu_thread_start (args=0x5596817ccd10)
at ../util/qemu-thread-posix.c:521
#10 start_thread () at /lib64/libpthread.so.0
#11 clone () at /lib64/libc.so.6
Note bs pointer taken from bitmap: it's definitely bad aligned. That's
because we are in use after free, bitmap is already freed.
So, let's make disabled bitmaps (being migrated) busy during incoming
migration.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
migration/block-dirty-bitmap.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 975093610a..35f5ef688d 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -839,6 +839,8 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
error_report_err(local_err);
return -EINVAL;
}
+ } else {
+ bdrv_dirty_bitmap_set_busy(s->bitmap, true);
}
b = g_new(LoadBitmapState, 1);
@@ -914,6 +916,8 @@ static void cancel_incoming_locked(DBMLoadState *s)
assert(!s->before_vm_start_handled || !b->migrated);
if (bdrv_dirty_bitmap_has_successor(b->bitmap)) {
bdrv_reclaim_dirty_bitmap(b->bitmap, &error_abort);
+ } else {
+ bdrv_dirty_bitmap_set_busy(b->bitmap, false);
}
bdrv_release_dirty_bitmap(b->bitmap);
}
@@ -951,6 +955,8 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
bdrv_reclaim_dirty_bitmap(s->bitmap, &error_abort);
+ } else {
+ bdrv_dirty_bitmap_set_busy(s->bitmap, false);
}
for (item = s->bitmaps; item; item = g_slist_next(item)) {
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps
2021-03-22 9:49 [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration Vladimir Sementsov-Ogievskiy
2021-03-22 9:49 ` [PATCH 1/2] migration/block-dirty-bitmap: make incoming disabled bitmaps busy Vladimir Sementsov-Ogievskiy
@ 2021-03-22 9:49 ` Vladimir Sementsov-Ogievskiy
2021-03-22 12:22 ` [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration Vladimir Sementsov-Ogievskiy
2021-03-22 16:56 ` Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-03-22 9:49 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, dgilbert, quintela, fam, stefanha,
jsnow, vsementsov, eblake
Check that we can't remove bitmaps being migrated on destination vm.
The new check proves that previous commit helps.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
index d046ebeb94..584062b412 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
@@ -224,6 +224,16 @@ class TestDirtyBitmapPostcopyMigration(iotests.QMPTestCase):
self.start_postcopy()
self.vm_b_events += self.vm_b.get_qmp_events()
+
+ # While being here, let's check that we can't remove in-flight bitmaps.
+ for vm in (self.vm_a, self.vm_b):
+ for i in range(0, nb_bitmaps):
+ result = vm.qmp('block-dirty-bitmap-remove', node='drive0',
+ name=f'bitmap{i}')
+ self.assert_qmp(result, 'error/desc',
+ f"Bitmap 'bitmap{i}' is currently in use by "
+ "another operation and cannot be used")
+
self.vm_b.shutdown()
# recreate vm_b, so there is no incoming option, which prevents
# loading bitmaps from disk
--
2.29.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration
2021-03-22 9:49 [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration Vladimir Sementsov-Ogievskiy
2021-03-22 9:49 ` [PATCH 1/2] migration/block-dirty-bitmap: make incoming disabled bitmaps busy Vladimir Sementsov-Ogievskiy
2021-03-22 9:49 ` [PATCH 2/2] migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps Vladimir Sementsov-Ogievskiy
@ 2021-03-22 12:22 ` Vladimir Sementsov-Ogievskiy
2021-03-22 16:56 ` Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2021-03-22 12:22 UTC (permalink / raw)
To: qemu-block
Cc: qemu-devel, mreitz, kwolf, dgilbert, quintela, fam, stefanha,
jsnow, eblake
22.03.2021 12:49, Vladimir Sementsov-Ogievskiy wrote:
> Hi all! Accidentally we found on use-after-free. Normally user should
> not remove bitmaps during migration.. But some wrong user actions may
> simply lead to Qemu crash and that's not good.
>
> Vladimir Sementsov-Ogievskiy (2):
> migration/block-dirty-bitmap: make incoming disabled bitmaps busy
> migrate-bitmaps-postcopy-test: check that we can't remove in-flight
> bitmaps
>
> migration/block-dirty-bitmap.c | 6 ++++++
> tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
> 2 files changed, 16 insertions(+)
>
Oops sorry. Actually, it's a v2 for "[PATCH 0/2] Fix crash if try to remove bitmap on target during migration" with a bit improved test, patch 1` unchanged.
Supersedes: <20210319204124.364312-1-vsementsov@virtuozzo.com>
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration
2021-03-22 9:49 [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2021-03-22 12:22 ` [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration Vladimir Sementsov-Ogievskiy
@ 2021-03-22 16:56 ` Stefan Hajnoczi
3 siblings, 0 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2021-03-22 16:56 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: kwolf, fam, qemu-block, quintela, dgilbert, qemu-devel, mreitz, jsnow
[-- Attachment #1: Type: text/plain, Size: 825 bytes --]
On Mon, Mar 22, 2021 at 12:49:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Hi all! Accidentally we found on use-after-free. Normally user should
> not remove bitmaps during migration.. But some wrong user actions may
> simply lead to Qemu crash and that's not good.
>
> Vladimir Sementsov-Ogievskiy (2):
> migration/block-dirty-bitmap: make incoming disabled bitmaps busy
> migrate-bitmaps-postcopy-test: check that we can't remove in-flight
> bitmaps
>
> migration/block-dirty-bitmap.c | 6 ++++++
> tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
> 2 files changed, 16 insertions(+)
>
> --
> 2.29.2
>
Thanks, applied to my cpuidle-haltpoll-virtqueue tree:
https://gitlab.com/stefanha/qemu/commits/cpuidle-haltpoll-virtqueue
Stefan
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-03-22 16:57 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-22 9:49 [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration Vladimir Sementsov-Ogievskiy
2021-03-22 9:49 ` [PATCH 1/2] migration/block-dirty-bitmap: make incoming disabled bitmaps busy Vladimir Sementsov-Ogievskiy
2021-03-22 9:49 ` [PATCH 2/2] migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps Vladimir Sementsov-Ogievskiy
2021-03-22 12:22 ` [PATCH for-6.0 0/2] Fix use-after-free, if remove bitmap during migration Vladimir Sementsov-Ogievskiy
2021-03-22 16:56 ` Stefan Hajnoczi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).