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

[Vladimir Sementsov-Ogievskiy]

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().

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