[PULL 0/2] Migration 20241113 patches

[PULL 0/2] Migration 20241113 patches

[Peter Xu]

The following changes since commit f0cfd067867668870931c9411d96cd518564b7a8:

  Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging (2024-11-09 12:34:01 +0000)

are available in the Git repository at:

  https://gitlab.com/peterx/qemu.git tags/migration-20241113-pull-request

for you to fetch changes up to 0926c002c7c71749a781de13f28b0481e029d323:

  migration: fix-possible-int-overflow (2024-11-13 13:02:46 -0500)

----------------------------------------------------------------
Migration pull request for 9.2-rc1

- Dmitry's small patch to quiesce a warning in possible uint32_t overflow
- Peter's fix on a recent regression to iotests (and potentially elsewhere)

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

Dmitry Frolov (1):
  migration: fix-possible-int-overflow

Peter Xu (1):
  migration: Check current_migration in migration_is_running()

 migration/migration.c | 4 ++++
 migration/multifd.c   | 2 +-
 2 files changed, 5 insertions(+), 1 deletion(-)

-- 
2.45.0

[PULL 1/2] migration: Check current_migration in migration_is_running()

[Peter Xu]

Report shows that commit 34a8892dec broke iotest 055:

https://lore.kernel.org/r/b8806360-a2b6-4608-83a3-db67e264c733@linaro.org

Denis Rastyogin reported more such issue:

https://lore.kernel.org/r/20241107114256.106831-1-gerben@altlinux.org

  In this merge, the migration_is_idle() function was replaced with
  migrate_is_running().  However, the null pointer check for `s` was
  removed, leading to a dereference of `s` when using qemu-system-x86_64
  -hda *.vdi.

When replacing migration_is_idle() with "!migration_is_running()", it was
overlooked that the idle helper also checks for current_migration being
available first.  Sample stack dump:

 migration_is_running
 is_busy
 migrate_add_blocker_modes
 migrate_add_blocker_normal
 vmdk_open
 bdrv_open_driver
 bdrv_open_common
 bdrv_open_inherit
 bdrv_open
 blk_new_open
 blockdev_init
 drive_new
 drive_init_func
 qemu_opts_foreach
 configure_blockdev
 qemu_create_early_backends
 qemu_init
 main

The check would be there if the whole series was applied, but since the
last patches in the previous series rely on some other patches to land
first, we need to recover the behavior of migration_is_idle() first before
that whole set will be merged.

I left migration_is_active / migration_is_device alone, as I don't think
it's possible for them to hit uninitialized current_migration. Also they're
prone to removal soon from VFIO side.

Cc: Peter Maydell <peter.maydell@linaro.org>
Fixes: 34a8892dec ("migration: Drop migration_is_idle()")
Reported-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reported-by: Denis Rastyogin <gerben@altlinux.org>
Tested-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Link: https://lore.kernel.org/r/20241105182725.2393425-1-peterx@redhat.com
[peterx: enhance commit msg]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index aedf7f0751..8c5bd0a75c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1117,6 +1117,10 @@ bool migration_is_running(void)
 {
     MigrationState *s = current_migration;
 
+    if (!s) {
+        return false;
+    }
+
     switch (s->state) {
     case MIGRATION_STATUS_ACTIVE:
     case MIGRATION_STATUS_POSTCOPY_ACTIVE:
-- 
2.45.0

[PULL 2/2] migration: fix-possible-int-overflow

[Peter Xu]

From: Dmitry Frolov <frolov@swemel.ru>

stat64_add() takes uint64_t as 2nd argument, but both
"p->next_packet_size" and "p->packet_len" are uint32_t.
Thus, theyr sum may overflow uint32_t.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
Link: https://lore.kernel.org/r/20241113140509.325732-2-frolov@swemel.ru
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/multifd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/migration/multifd.c b/migration/multifd.c
index 4374e14a96..498e71fd10 100644
--- a/migration/multifd.c
+++ b/migration/multifd.c
@@ -623,7 +623,7 @@ static void *multifd_send_thread(void *opaque)
             }
 
             stat64_add(&mig_stats.multifd_bytes,
-                       p->next_packet_size + p->packet_len);
+                       (uint64_t)p->next_packet_size + p->packet_len);
 
             p->next_packet_size = 0;
             multifd_set_payload_type(p->data, MULTIFD_PAYLOAD_NONE);
-- 
2.45.0

Re: [PULL 2/2] migration: fix-possible-int-overflow

[Philippe Mathieu-Daudé]

Hi,

On 13/11/24 20:16, Peter Xu wrote:
> From: Dmitry Frolov <frolov@swemel.ru>
> 
> stat64_add() takes uint64_t as 2nd argument, but both
> "p->next_packet_size" and "p->packet_len" are uint32_t.
> Thus, theyr sum may overflow uint32_t.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> Link: https://lore.kernel.org/r/20241113140509.325732-2-frolov@swemel.ru
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   migration/multifd.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/migration/multifd.c b/migration/multifd.c
> index 4374e14a96..498e71fd10 100644
> --- a/migration/multifd.c
> +++ b/migration/multifd.c
> @@ -623,7 +623,7 @@ static void *multifd_send_thread(void *opaque)
>               }
>   
>               stat64_add(&mig_stats.multifd_bytes,
> -                       p->next_packet_size + p->packet_len);
> +                       (uint64_t)p->next_packet_size + p->packet_len);

I am not familiar with this area, but quickly looking I can't
find a code path accepting 4GiB payload, so IMHO this hypothetical
case is not unreachable. My 2 cents (I'm not objecting on this
"silence this warning" patch).

>   
>               p->next_packet_size = 0;
>               multifd_set_payload_type(p->data, MULTIFD_PAYLOAD_NONE);

Re: [PULL 2/2] migration: fix-possible-int-overflow

[Peter Xu]

On Wed, Nov 13, 2024 at 09:40:50PM +0100, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> On 13/11/24 20:16, Peter Xu wrote:
> > From: Dmitry Frolov <frolov@swemel.ru>
> > 
> > stat64_add() takes uint64_t as 2nd argument, but both
> > "p->next_packet_size" and "p->packet_len" are uint32_t.
> > Thus, theyr sum may overflow uint32_t.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > 
> > Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> > Link: https://lore.kernel.org/r/20241113140509.325732-2-frolov@swemel.ru
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   migration/multifd.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index 4374e14a96..498e71fd10 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -623,7 +623,7 @@ static void *multifd_send_thread(void *opaque)
> >               }
> >               stat64_add(&mig_stats.multifd_bytes,
> > -                       p->next_packet_size + p->packet_len);
> > +                       (uint64_t)p->next_packet_size + p->packet_len);
> 
> I am not familiar with this area, but quickly looking I can't
> find a code path accepting 4GiB payload, so IMHO this hypothetical
> case is not unreachable. My 2 cents (I'm not objecting on this
> "silence this warning" patch).

Thanks Phil, for taking an extra eye.

Yes, the solo goal is probably to silent it, if that helps anyone at all.
I left similar comment when replying to Dmitry when queuing this.

If it could overflow, we have more troubles, e.g., we have plenty of places
caching these values in 32bits.  When this overflow could happen, we should
simply switch everything to 64bit..

> 
> >               p->next_packet_size = 0;
> >               multifd_set_payload_type(p->data, MULTIFD_PAYLOAD_NONE);
> 

-- 
Peter Xu

Re: [PULL 0/2] Migration 20241113 patches

[Peter Maydell]

On Wed, 13 Nov 2024 at 20:16, Peter Xu <peterx@redhat.com> wrote:
>
> The following changes since commit f0cfd067867668870931c9411d96cd518564b7a8:
>
>   Merge tag 'for-upstream' of https://gitlab.com/bonzini/qemu into staging (2024-11-09 12:34:01 +0000)
>
> are available in the Git repository at:
>
>   https://gitlab.com/peterx/qemu.git tags/migration-20241113-pull-request
>
> for you to fetch changes up to 0926c002c7c71749a781de13f28b0481e029d323:
>
>   migration: fix-possible-int-overflow (2024-11-13 13:02:46 -0500)
>
> ----------------------------------------------------------------
> Migration pull request for 9.2-rc1
>
> - Dmitry's small patch to quiesce a warning in possible uint32_t overflow
> - Peter's fix on a recent regression to iotests (and potentially elsewhere)
>
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/9.2
for any user-visible changes.

-- PMM