diff mbox series

[v3,03/10] migration: Fix possible race when checking to_dst_file for errors

Message ID 20230811150836.2895-4-farosas@suse.de
State New
Headers show
Series Fix segfault on migration return path | expand

Commit Message

Fabiano Rosas Aug. 11, 2023, 3:08 p.m. UTC 
Checking ms->to_dst_file for errors when cleaning up the return path
could race with migrate_fd_cleanup() which clears the pointer.

Since migrate_fd_cleanup() is reachable via qmp_migrate(), which is
issued by the user, it is safer if we take the lock when reading
ms->to_dst_file.

Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
 migration/migration.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Peter Xu Aug. 15, 2023, 9:49 p.m. UTC | #1 
On Fri, Aug 11, 2023 at 12:08:29PM -0300, Fabiano Rosas wrote:
> diff --git a/migration/migration.c b/migration/migration.c
> index 0067c927fa..85c171f32c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2057,11 +2057,10 @@ static int await_return_path_close_on_source(MigrationState *ms)
>       * need to cause it to exit. shutdown(2), if we have it, will
>       * cause it to unblock if it's stuck waiting for the destination.
>       */
> -    if (qemu_file_get_error(ms->to_dst_file)) {
> -        WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
> -            if (ms->rp_state.from_dst_file) {
> -                qemu_file_shutdown(ms->rp_state.from_dst_file);
> -            }
> +    WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
> +        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
> +            qemu_file_get_error(ms->to_dst_file)) {
> +            qemu_file_shutdown(ms->rp_state.from_dst_file);
>          }
>      }

Squash into previous one?
diff mbox series

Patch

diff --git a/migration/migration.c b/migration/migration.c
index 0067c927fa..85c171f32c 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2057,11 +2057,10 @@  static int await_return_path_close_on_source(MigrationState *ms)
      * need to cause it to exit. shutdown(2), if we have it, will
      * cause it to unblock if it's stuck waiting for the destination.
      */
-    if (qemu_file_get_error(ms->to_dst_file)) {
-        WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
-            if (ms->rp_state.from_dst_file) {
-                qemu_file_shutdown(ms->rp_state.from_dst_file);
-            }
+    WITH_QEMU_LOCK_GUARD(&ms->qemu_file_lock) {
+        if (ms->to_dst_file && ms->rp_state.from_dst_file &&
+            qemu_file_get_error(ms->to_dst_file)) {
+            qemu_file_shutdown(ms->rp_state.from_dst_file);
         }
     }