Series comparison

-[PULL 0/1] Block patches
+[PULL 0/2] Block patches
-The following changes since commit 61c265f0660ee476985808c8aa7915617c44fd53:
+The following changes since commit 67c1115edd98f388ca89dd38322ea3fadf034523:
-  Merge remote-tracking branch 'remotes/dgilbert/tags/pull-migration-20200313a' into staging (2020-03-13 10:33:04 +0000)
+  Merge remote-tracking branch 'remotes/kraxel/tags/ui-20210323-pull-request' into staging (2021-03-23 23:47:30 +0000)
 are available in the Git repository at:
-  https://github.com/stefanha/qemu.git tags/block-pull-request
+  https://gitlab.com/stefanha/qemu.git tags/block-pull-request
-for you to fetch changes up to 4ab78b19189a81038e744728ed949d09aa477550:
+for you to fetch changes up to 3460fd7f3959d1fa7bcc255796844aa261c805a4:
-  block/io: fix bdrv_co_do_copy_on_readv (2020-03-16 11:46:11 +0000)
+  migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps (2021-03-24 13:41:19 +0000)
 ----------------------------------------------------------------
 Pull request
+This dirty bitmap fix solves a crash that can be triggered in the destination
+QEMU process during live migration.
 ----------------------------------------------------------------
-Vladimir Sementsov-Ogievskiy (1):
+Vladimir Sementsov-Ogievskiy (2):
-  block/io: fix bdrv_co_do_copy_on_readv
+  migration/block-dirty-bitmap: make incoming disabled bitmaps busy
   migrate-bitmaps-postcopy-test: check that we can't remove in-flight
     bitmaps
- block/io.c | 2 +-
+ migration/block-dirty-bitmap.c                         |  6 ++++++
-file changed, 1 insertion(+), 1 deletion(-)
+ tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
 files changed, 16 insertions(+)
 --
-.24.1
+.30.2

-[PULL 1/1] block/io: fix bdrv_co_do_copy_on_readv
+[PULL 1/2] migration/block-dirty-bitmap: make incoming disabled bitmaps busy
 From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
-Prior to 1143ec5ebf4 it was OK to qemu_iovec_from_buf() from aligned-up
+Incoming enabled bitmaps are busy, because we do
-buffer to original qiov, as qemu_iovec_from_buf() will stop at qiov end
+bdrv_dirty_bitmap_create_successor() for them. But disabled bitmaps
-anyway.
+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:
-But after 1143ec5ebf4 we assume that bdrv_co_do_copy_on_readv works on
+ #0  qemu_mutex_lock_impl (mutex=0x5593d88c50d1, file=0x559680554b20
-part of original qiov, defined by qiov_offset and bytes. So we must not
+   "../block/dirty-bitmap.c", line=64) at ../util/qemu-thread-posix.c:77
-touch qiov behind qiov_offset+bytes bound. Fix it.
+ #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
-Cc: qemu-stable@nongnu.org # v4.2
+Note bs pointer taken from bitmap: it's definitely bad aligned. That's
-Fixes: 1143ec5ebf4
+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>
-Reviewed-by: John Snow <jsnow@redhat.com>
-Message-id: 20200312081949.5350-1-vsementsov@virtuozzo.com
 Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+Message-Id: <20210322094906.5079-2-vsementsov@virtuozzo.com>
 ---
- block/io.c | 2 +-
+ migration/block-dirty-bitmap.c | 6 ++++++
-file changed, 1 insertion(+), 1 deletion(-)
+file changed, 6 insertions(+)
-diff --git a/block/io.c b/block/io.c
+diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
 index XXXXXXX..XXXXXXX 100644
---- a/block/io.c
+--- a/migration/block-dirty-bitmap.c
-+++ b/block/io.c
++++ b/migration/block-dirty-bitmap.c
-@@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
+@@ -XXX,XX +XXX,XX @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
-             if (!(flags & BDRV_REQ_PREFETCH)) {
+             error_report_err(local_err);
-                 qemu_iovec_from_buf(qiov, qiov_offset + progress,
+             return -EINVAL;
-                                     bounce_buffer + skip_bytes,
+         }
--                                    pnum - skip_bytes);
++    } else {
-+                                    MIN(pnum - skip_bytes, bytes - progress));
++        bdrv_dirty_bitmap_set_busy(s->bitmap, true);
-             }
+     }
-         } else if (!(flags & BDRV_REQ_PREFETCH)) {
-             /* Read directly into the destination */
+     b = g_new(LoadBitmapState, 1);
@@ -XXX,XX +XXX,XX @@ 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);
      }
@@ -XXX,XX +XXX,XX @@ 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)) {
 --
-.24.1
+.30.2

-New patch
+[PULL 2/2] migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps
+From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
+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>
+Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
+Message-Id: <20210322094906.5079-3-vsementsov@virtuozzo.com>
+---
+ tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test | 10 ++++++++++
+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 XXXXXXX..XXXXXXX 100755
+--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
++++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
+@@ -XXX,XX +XXX,XX @@ 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
+--
+.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Prior to 1143ec5ebf4 it was OK to qemu_iovec_from_buf() from aligned-up
buffer to original qiov, as qemu_iovec_from_buf() will stop at qiov end
anyway.

But after 1143ec5ebf4 we assume that bdrv_co_do_copy_on_readv works on
part of original qiov, defined by qiov_offset and bytes. So we must not
touch qiov behind qiov_offset+bytes bound. Fix it.

Cc: qemu-stable@nongnu.org # v4.2
Fixes: 1143ec5ebf4
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 20200312081949.5350-1-vsementsov@virtuozzo.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index XXXXXXX..XXXXXXX 100644
--- a/block/io.c
+++ b/block/io.c
@@ -XXX,XX +XXX,XX @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
             if (!(flags & BDRV_REQ_PREFETCH)) {
                 qemu_iovec_from_buf(qiov, qiov_offset + progress,
                                     bounce_buffer + skip_bytes,
-                                    pnum - skip_bytes);
+                                    MIN(pnum - skip_bytes, bytes - progress));
             }
         } else if (!(flags & BDRV_REQ_PREFETCH)) {
             /* Read directly into the destination */
-- 
2.24.1

The following changes since commit 67c1115edd98f388ca89dd38322ea3fadf034523:

Merge remote-tracking branch 'remotes/kraxel/tags/ui-20210323-pull-request' into staging (2021-03-23 23:47:30 +0000)

are available in the Git repository at:

https://gitlab.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 3460fd7f3959d1fa7bcc255796844aa261c805a4:

migrate-bitmaps-postcopy-test: check that we can't remove in-flight bitmaps (2021-03-24 13:41:19 +0000)

----------------------------------------------------------------
Pull request

This dirty bitmap fix solves a crash that can be triggered in the destination
QEMU process during live migration.

----------------------------------------------------------------

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.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20210322094906.5079-2-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 XXXXXXX..XXXXXXX 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -XXX,XX +XXX,XX @@ 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);
@@ -XXX,XX +XXX,XX @@ 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);
     }
@@ -XXX,XX +XXX,XX @@ 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.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

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>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-Id: <20210322094906.5079-3-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 XXXXXXX..XXXXXXX 100755
--- a/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
+++ b/tests/qemu-iotests/tests/migrate-bitmaps-postcopy-test
@@ -XXX,XX +XXX,XX @@ 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.30.2