* [Qemu-devel] [RFC 0/2] vhost+postcopy fixes
[not found] <CGME20181008160317eucas1p12156552c6876786bd3087d3922d49399@eucas1p1.samsung.com>
@ 2018-10-08 16:05 ` Ilya Maximets
[not found] ` <CGME20181008160325eucas1p23bd6b6ce112175d274db884fb2a83f54@eucas1p2.samsung.com>
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Ilya Maximets @ 2018-10-08 16:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Dr. David Alan Gilbert, Marc-André Lureau,
Maxime Coquelin, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [RFC 1/2] migration: Stop postcopy fault thread before notifying
[not found] ` <CGME20181008160325eucas1p23bd6b6ce112175d274db884fb2a83f54@eucas1p2.samsung.com>
@ 2018-10-08 16:05 ` Ilya Maximets
2018-10-10 19:01 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 7+ messages in thread
From: Ilya Maximets @ 2018-10-08 16:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Dr. David Alan Gilbert, Marc-André Lureau,
Maxime Coquelin, Ilya Maximets, qemu-stable
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [RFC 2/2] vhost-user: Fix userfaultfd leak
[not found] ` <CGME20181008160328eucas1p14fff8e1e5e5f065019353fb86b845340@eucas1p1.samsung.com>
@ 2018-10-08 16:05 ` Ilya Maximets
2018-10-10 19:05 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 7+ messages in thread
From: Ilya Maximets @ 2018-10-08 16:05 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: qemu-devel, Dr. David Alan Gilbert, Marc-André Lureau,
Maxime Coquelin, Ilya Maximets, qemu-stable
'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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC 0/2] vhost+postcopy fixes
2018-10-08 16:05 ` [Qemu-devel] [RFC 0/2] vhost+postcopy fixes Ilya Maximets
[not found] ` <CGME20181008160325eucas1p23bd6b6ce112175d274db884fb2a83f54@eucas1p2.samsung.com>
[not found] ` <CGME20181008160328eucas1p14fff8e1e5e5f065019353fb86b845340@eucas1p1.samsung.com>
@ 2018-10-10 7:28 ` Maxime Coquelin
2018-10-11 17:43 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 7+ messages in thread
From: Maxime Coquelin @ 2018-10-10 7:28 UTC (permalink / raw)
To: Ilya Maximets, Michael S. Tsirkin
Cc: qemu-devel, Dr. David Alan Gilbert, Marc-André Lureau
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC 1/2] migration: Stop postcopy fault thread before notifying
2018-10-08 16:05 ` [Qemu-devel] [RFC 1/2] migration: Stop postcopy fault thread before notifying Ilya Maximets
@ 2018-10-10 19:01 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-10 19:01 UTC (permalink / raw)
To: Ilya Maximets
Cc: Michael S. Tsirkin, qemu-devel, Marc-André Lureau,
Maxime Coquelin, qemu-stable
* 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC 2/2] vhost-user: Fix userfaultfd leak
2018-10-08 16:05 ` [Qemu-devel] [RFC 2/2] vhost-user: Fix userfaultfd leak Ilya Maximets
@ 2018-10-10 19:05 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-10 19:05 UTC (permalink / raw)
To: Ilya Maximets
Cc: Michael S. Tsirkin, qemu-devel, Marc-André Lureau,
Maxime Coquelin, qemu-stable
* 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [RFC 0/2] vhost+postcopy fixes
2018-10-08 16:05 ` [Qemu-devel] [RFC 0/2] vhost+postcopy fixes Ilya Maximets
` (2 preceding siblings ...)
2018-10-10 7:28 ` [Qemu-devel] [RFC 0/2] vhost+postcopy fixes Maxime Coquelin
@ 2018-10-11 17:43 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 7+ messages in thread
From: Dr. David Alan Gilbert @ 2018-10-11 17:43 UTC (permalink / raw)
To: Ilya Maximets
Cc: Michael S. Tsirkin, qemu-devel, Marc-André Lureau, Maxime Coquelin
* 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-10-11 17:44 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20181008160317eucas1p12156552c6876786bd3087d3922d49399@eucas1p1.samsung.com>
2018-10-08 16:05 ` [Qemu-devel] [RFC 0/2] vhost+postcopy fixes Ilya Maximets
[not found] ` <CGME20181008160325eucas1p23bd6b6ce112175d274db884fb2a83f54@eucas1p2.samsung.com>
2018-10-08 16:05 ` [Qemu-devel] [RFC 1/2] migration: Stop postcopy fault thread before notifying Ilya Maximets
2018-10-10 19:01 ` Dr. David Alan Gilbert
[not found] ` <CGME20181008160328eucas1p14fff8e1e5e5f065019353fb86b845340@eucas1p1.samsung.com>
2018-10-08 16:05 ` [Qemu-devel] [RFC 2/2] vhost-user: Fix userfaultfd leak Ilya Maximets
2018-10-10 19:05 ` Dr. David Alan Gilbert
2018-10-10 7:28 ` [Qemu-devel] [RFC 0/2] vhost+postcopy fixes Maxime Coquelin
2018-10-11 17:43 ` Dr. David Alan Gilbert
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.