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

[Vladimir Sementsov-Ogievskiy]

On 30.04.24 11:09, Vladimir Sementsov-Ogievskiy wrote:
> On 30.04.24 11:06, Vladimir Sementsov-Ogievskiy wrote:
> > On 29.04.24 22:32, Peter Xu wrote:
> > > 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?
> >
> > Hmm. But we want to call migration_incoming_state_destroy() in case when 
> > exit-on-error=false too. And in this case we want to keep the error for further 
> > query-migrate commands.
>
> Actually, I think we shouldn't care about freeing the error for exit() case. I 
> think, we skip a lot of other cleanups which we normally do in main() in case 
> of this exit().

Or, just do the simplest fix of potential use-after-free, and don't care:

WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
   error_report_err(s->error);
   s->error = NULL;
}

- I'll send v6 with it.

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

--
Best regards,
Vladimir