1. Patchew
  2. QEMU
  3. [PATCH v5 0/5] migration: do not exit on incoming failure
  4. View patch

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

Vladimir Sementsov-Ogievskiy posted 5 patches 1 year, 11 months ago
There is a newer version of this series
[PATCH v5 3/5] migration: process_incoming_migration_co(): fix reporting s->error
Posted by Vladimir Sementsov-Ogievskiy 1 year, 11 months ago 
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));
             }
         }
         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);
     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
Re: [PATCH v5 3/5] migration: process_incoming_migration_co(): fix reporting s->error
Posted by Peter Xu 1 year, 11 months ago 
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
Re: [PATCH v5 3/5] migration: process_incoming_migration_co(): fix reporting s->error
Posted by Vladimir Sementsov-Ogievskiy 1 year, 11 months ago 
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.

> 
> 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
Re: [PATCH v5 3/5] migration: process_incoming_migration_co(): fix reporting s->error
Posted by Vladimir Sementsov-Ogievskiy 1 year, 11 months ago 
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
Re: [PATCH v5 3/5] migration: process_incoming_migration_co(): fix reporting s->error
Posted by Vladimir Sementsov-Ogievskiy 1 year, 11 months ago 
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

Patchew 2.1-devel-8c64711

© 2016 - 2026 Red Hat, Inc.