[Qemu-devel] [RFC 0/2] vhost+postcopy fixes

[Qemu-devel] [RFC 0/2] vhost+postcopy fixes

[Ilya Maximets]

Sending as RFC because it's not fully tested yet.

Ilya Maximets (2):
  migration: Stop postcopy fault thread before notifying
  vhost-user: Fix userfaultfd leak

 hw/virtio/vhost-user.c   |  7 +++++++
 migration/postcopy-ram.c | 11 ++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

-- 
2.17.1

[Qemu-devel] [RFC 1/2] migration: Stop postcopy fault thread before notifying

[Ilya Maximets]

POSTCOPY_NOTIFY_INBOUND_END handlers will remove userfault fds
from the postcopy_remote_fds array which could be still in
use by the fault thread. Let's stop the thread before
notification to avoid possible accessing wrong memory.

Fixes: 46343570c06e ("vhost+postcopy: Wire up POSTCOPY_END notify")
Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 migration/postcopy-ram.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 853d8b32ca..e5c02a32c5 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -533,6 +533,12 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
     if (mis->have_fault_thread) {
         Error *local_err = NULL;
 
+        /* Let the fault thread quit */
+        atomic_set(&mis->fault_thread_quit, 1);
+        postcopy_fault_thread_notify(mis);
+        trace_postcopy_ram_incoming_cleanup_join();
+        qemu_thread_join(&mis->fault_thread);
+
         if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_END, &local_err)) {
             error_report_err(local_err);
             return -1;
@@ -541,11 +547,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
         if (qemu_ram_foreach_migratable_block(cleanup_range, mis)) {
             return -1;
         }
-        /* Let the fault thread quit */
-        atomic_set(&mis->fault_thread_quit, 1);
-        postcopy_fault_thread_notify(mis);
-        trace_postcopy_ram_incoming_cleanup_join();
-        qemu_thread_join(&mis->fault_thread);
 
         trace_postcopy_ram_incoming_cleanup_closeuf();
         close(mis->userfault_fd);
-- 
2.17.1

Re: [Qemu-devel] [RFC 1/2] migration: Stop postcopy fault thread before notifying

[Dr. David Alan Gilbert]

* Ilya Maximets (i.maximets@samsung.com) wrote:
> POSTCOPY_NOTIFY_INBOUND_END handlers will remove userfault fds
> from the postcopy_remote_fds array which could be still in
> use by the fault thread. Let's stop the thread before
> notification to avoid possible accessing wrong memory.

OK I think; since this is already in the cleanup we shouldn't
be getting faults anyway at that point.


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> Fixes: 46343570c06e ("vhost+postcopy: Wire up POSTCOPY_END notify")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  migration/postcopy-ram.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> index 853d8b32ca..e5c02a32c5 100644
> --- a/migration/postcopy-ram.c
> +++ b/migration/postcopy-ram.c
> @@ -533,6 +533,12 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>      if (mis->have_fault_thread) {
>          Error *local_err = NULL;
>  
> +        /* Let the fault thread quit */
> +        atomic_set(&mis->fault_thread_quit, 1);
> +        postcopy_fault_thread_notify(mis);
> +        trace_postcopy_ram_incoming_cleanup_join();
> +        qemu_thread_join(&mis->fault_thread);
> +
>          if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_END, &local_err)) {
>              error_report_err(local_err);
>              return -1;
> @@ -541,11 +547,6 @@ int postcopy_ram_incoming_cleanup(MigrationIncomingState *mis)
>          if (qemu_ram_foreach_migratable_block(cleanup_range, mis)) {
>              return -1;
>          }
> -        /* Let the fault thread quit */
> -        atomic_set(&mis->fault_thread_quit, 1);
> -        postcopy_fault_thread_notify(mis);
> -        trace_postcopy_ram_incoming_cleanup_join();
> -        qemu_thread_join(&mis->fault_thread);
>  
>          trace_postcopy_ram_incoming_cleanup_closeuf();
>          close(mis->userfault_fd);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

[Qemu-devel] [RFC 2/2] vhost-user: Fix userfaultfd leak

[Ilya Maximets]

'fd' received from the vhost side is never freed.
Also, everything (including 'postcopy_listen' state) should be
cleaned up on vhost cleanup.

Fixes: 46343570c06e ("vhost+postcopy: Wire up POSTCOPY_END notify")
Fixes: f82c11165ffa ("vhost+postcopy: Register shared ufd with postcopy")
Cc: qemu-stable@nongnu.org
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 hw/virtio/vhost-user.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index c442daa562..e09bed0e4a 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1280,6 +1280,7 @@ static int vhost_user_postcopy_end(struct vhost_dev *dev, Error **errp)
         return ret;
     }
     postcopy_unregister_shared_ufd(&u->postcopy_fd);
+    close(u->postcopy_fd.fd);
     u->postcopy_fd.handler = NULL;
 
     trace_vhost_user_postcopy_end_exit();
@@ -1419,6 +1420,12 @@ static int vhost_user_backend_cleanup(struct vhost_dev *dev)
         postcopy_remove_notifier(&u->postcopy_notifier);
         u->postcopy_notifier.notify = NULL;
     }
+    u->postcopy_listen = false;
+    if (u->postcopy_fd.handler) {
+        postcopy_unregister_shared_ufd(&u->postcopy_fd);
+        close(u->postcopy_fd.fd);
+        u->postcopy_fd.handler = NULL;
+    }
     if (u->slave_fd >= 0) {
         qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
         close(u->slave_fd);
-- 
2.17.1

Re: [Qemu-devel] [RFC 2/2] vhost-user: Fix userfaultfd leak

[Dr. David Alan Gilbert]

* Ilya Maximets (i.maximets@samsung.com) wrote:
> 'fd' received from the vhost side is never freed.
> Also, everything (including 'postcopy_listen' state) should be
> cleaned up on vhost cleanup.
> 
> Fixes: 46343570c06e ("vhost+postcopy: Wire up POSTCOPY_END notify")
> Fixes: f82c11165ffa ("vhost+postcopy: Register shared ufd with postcopy")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>

Thanks,


Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  hw/virtio/vhost-user.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index c442daa562..e09bed0e4a 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1280,6 +1280,7 @@ static int vhost_user_postcopy_end(struct vhost_dev *dev, Error **errp)
>          return ret;
>      }
>      postcopy_unregister_shared_ufd(&u->postcopy_fd);
> +    close(u->postcopy_fd.fd);
>      u->postcopy_fd.handler = NULL;
>  
>      trace_vhost_user_postcopy_end_exit();
> @@ -1419,6 +1420,12 @@ static int vhost_user_backend_cleanup(struct vhost_dev *dev)
>          postcopy_remove_notifier(&u->postcopy_notifier);
>          u->postcopy_notifier.notify = NULL;
>      }
> +    u->postcopy_listen = false;
> +    if (u->postcopy_fd.handler) {
> +        postcopy_unregister_shared_ufd(&u->postcopy_fd);
> +        close(u->postcopy_fd.fd);
> +        u->postcopy_fd.handler = NULL;
> +    }
>      if (u->slave_fd >= 0) {
>          qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
>          close(u->slave_fd);
> -- 
> 2.17.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [RFC 0/2] vhost+postcopy fixes

[Maxime Coquelin]

On 10/08/2018 06:05 PM, Ilya Maximets wrote:
> Sending as RFC because it's not fully tested yet.
> 
> Ilya Maximets (2):
>    migration: Stop postcopy fault thread before notifying
>    vhost-user: Fix userfaultfd leak
> 
>   hw/virtio/vhost-user.c   |  7 +++++++
>   migration/postcopy-ram.c | 11 ++++++-----
>   2 files changed, 13 insertions(+), 5 deletions(-)
> 

For the series:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [Qemu-devel] [RFC 0/2] vhost+postcopy fixes

[Dr. David Alan Gilbert]

* Ilya Maximets (i.maximets@samsung.com) wrote:
> Sending as RFC because it's not fully tested yet.

Since we've bothed Review-by it, I've queued it.

Dave

> Ilya Maximets (2):
>   migration: Stop postcopy fault thread before notifying
>   vhost-user: Fix userfaultfd leak
> 
>  hw/virtio/vhost-user.c   |  7 +++++++
>  migration/postcopy-ram.c | 11 ++++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)
> 
> -- 
> 2.17.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK