Re: [PATCH v5 3/5] migration: process_incoming_migration_co(): fix reporting s->error

[Peter Xu]

On Mon, Apr 29, 2024 at 10:14:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> It's bad idea to leave critical section with error object freed, but
> s->error still set, this theoretically may lead to use-after-free
> crash. Let's avoid it.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  migration/migration.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 0d26db47f7..58fd5819bc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -732,9 +732,19 @@ static void process_incoming_migration_bh(void *opaque)
>      migration_incoming_state_destroy();
>  }
>  
> +static void migrate_error_free(MigrationState *s)
> +{
> +    QEMU_LOCK_GUARD(&s->error_mutex);
> +    if (s->error) {
> +        error_free(s->error);
> +        s->error = NULL;
> +    }
> +}
> +
>  static void coroutine_fn
>  process_incoming_migration_co(void *opaque)
>  {
> +    MigrationState *s = migrate_get_current();
>      MigrationIncomingState *mis = migration_incoming_get_current();
>      PostcopyState ps;
>      int ret;
> @@ -779,11 +789,9 @@ process_incoming_migration_co(void *opaque)
>      }
>  
>      if (ret < 0) {
> -        MigrationState *s = migrate_get_current();
> -
>          if (migrate_has_error(s)) {
>              WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
> -                error_report_err(s->error);
> +                error_report_err(error_copy(s->error));

This looks like a bugfix, agreed.

>              }
>          }
>          error_report("load of migration failed: %s", strerror(-ret));
> @@ -801,6 +809,7 @@ fail:
>                        MIGRATION_STATUS_FAILED);
>      migration_incoming_state_destroy();
>  
> +    migrate_error_free(s);

Would migration_incoming_state_destroy() be a better place?

One thing weird is we actually reuses MigrationState*'s error for incoming
too, but so far it looks ok as long as QEMU can't be both src & dst.  Then
calling migrate_error_free even in incoming_state_destroy() looks ok.

This patch still looks like containing two changes.  Better split them (or
just fix the bug only)?

Thanks,

>      exit(EXIT_FAILURE);
>  }
>  
> @@ -1433,15 +1442,6 @@ bool migrate_has_error(MigrationState *s)
>      return qatomic_read(&s->error);
>  }
>  
> -static void migrate_error_free(MigrationState *s)
> -{
> -    QEMU_LOCK_GUARD(&s->error_mutex);
> -    if (s->error) {
> -        error_free(s->error);
> -        s->error = NULL;
> -    }
> -}
> -
>  static void migrate_fd_error(MigrationState *s, const Error *error)
>  {
>      assert(s->to_dst_file == NULL);
> -- 
> 2.34.1
> 

-- 
Peter Xu