* [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing
@ 2017-09-20 18:53 Felipe Franciosi
2017-09-20 20:33 ` Marc-André Lureau
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Felipe Franciosi @ 2017-09-20 18:53 UTC (permalink / raw)
To: Jason Wang, Marc-Andre Lureau, Michael S. Tsirkin, Paolo Bonzini
Cc: qemu-devel, Felipe Franciosi
vhost_log_put() is called to decomission the dirty log between qemu and
a vhost device when stopping the device. Such a call can happen from
migration_completion().
Present code sets dev->log_size to zero too early in vhost_log_put(),
causing the sync check to always return false. As a consequence, the
last pass on the dirty bitmap never happens at the end of migration.
If a vhost device was busy (writing to guest memory) until the last
moments before vhost_virtqueue_stop(), this error will result in guest
memory corruption (at least) following migrations.
Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
---
hw/virtio/vhost.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 5fd69f0..ddc42f0 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -375,8 +375,6 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
if (!log) {
return;
}
- dev->log = NULL;
- dev->log_size = 0;
--log->refcnt;
if (log->refcnt == 0) {
@@ -396,6 +394,9 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
g_free(log);
}
+
+ dev->log = NULL;
+ dev->log_size = 0;
}
static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing
2017-09-20 18:53 [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing Felipe Franciosi
@ 2017-09-20 20:33 ` Marc-André Lureau
2017-09-20 23:40 ` Felipe Franciosi
2017-09-21 1:47 ` Jason Wang
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Marc-André Lureau @ 2017-09-20 20:33 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Jason Wang, Michael S. Tsirkin, Paolo Bonzini, qemu-devel
Hi
----- Original Message -----
> vhost_log_put() is called to decomission the dirty log between qemu and
> a vhost device when stopping the device. Such a call can happen from
> migration_completion().
>
> Present code sets dev->log_size to zero too early in vhost_log_put(),
> causing the sync check to always return false. As a consequence, the
> last pass on the dirty bitmap never happens at the end of migration.
>
> If a vhost device was busy (writing to guest memory) until the last
> moments before vhost_virtqueue_stop(), this error will result in guest
> memory corruption (at least) following migrations.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
> hw/virtio/vhost.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5fd69f0..ddc42f0 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -375,8 +375,6 @@ static void vhost_log_put(struct vhost_dev *dev, bool
> sync)
> if (!log) {
> return;
> }
> - dev->log = NULL;
> - dev->log_size = 0;
>
Good catch. This reminds me of another patch, but I can't find it.
What if we replace dev->log_size with log->size below?
(and I don't see a clear reason why dev->log_size would be different from "log ? log->size : 0", am I missing something?)
> --log->refcnt;
> if (log->refcnt == 0) {
> @@ -396,6 +394,9 @@ static void vhost_log_put(struct vhost_dev *dev, bool
> sync)
>
> g_free(log);
> }
> +
> + dev->log = NULL;
> + dev->log_size = 0;
> }
>
> static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing
2017-09-20 20:33 ` Marc-André Lureau
@ 2017-09-20 23:40 ` Felipe Franciosi
0 siblings, 0 replies; 6+ messages in thread
From: Felipe Franciosi @ 2017-09-20 23:40 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Jason Wang, Michael S. Tsirkin, Paolo Bonzini, QEMU Developers
Heya,
> On 20 Sep 2017, at 13:33, Marc-André Lureau <marcandre.lureau@redhat.com> wrote:
>
> Hi
>
> ----- Original Message -----
>> vhost_log_put() is called to decomission the dirty log between qemu and
>> a vhost device when stopping the device. Such a call can happen from
>> migration_completion().
>>
>> Present code sets dev->log_size to zero too early in vhost_log_put(),
>> causing the sync check to always return false. As a consequence, the
>> last pass on the dirty bitmap never happens at the end of migration.
>>
>> If a vhost device was busy (writing to guest memory) until the last
>> moments before vhost_virtqueue_stop(), this error will result in guest
>> memory corruption (at least) following migrations.
>>
>> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
>> ---
>> hw/virtio/vhost.c | 5 +++--
>> 1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 5fd69f0..ddc42f0 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -375,8 +375,6 @@ static void vhost_log_put(struct vhost_dev *dev, bool
>> sync)
>> if (!log) {
>> return;
>> }
>> - dev->log = NULL;
>> - dev->log_size = 0;
>>
>
>
> Good catch. This reminds me of another patch, but I can't find it.
I actually ran into this error when testing migration with a vhost-user-scsi backed controller and a request that completes while quiescing on a VRING_GET_BASE. Took me a while to figure out why the bitmap wasn't being honoured by Qemu. :(
>
> What if we replace dev->log_size with log->size below?
>
> (and I don't see a clear reason why dev->log_size would be different from "log ? log->size : 0", am I missing something?)
I can see that vhost_dev_log_resize() changes dev->log_size without necessarily changing dev->log_size. Having said that, it seems like the latter gets correctly updated during vhost_log_get() which happens on the same function.
In any case, I feel like any further improvement to the function should be discussed and done in a separate commit.
Cheers,
Felipe
>
>> --log->refcnt;
>> if (log->refcnt == 0) {
>> @@ -396,6 +394,9 @@ static void vhost_log_put(struct vhost_dev *dev, bool
>> sync)
>>
>> g_free(log);
>> }
>> +
>> + dev->log = NULL;
>> + dev->log_size = 0;
>
>> }
>>
>> static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
>> --
>> 1.7.1
>>
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing
2017-09-20 18:53 [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing Felipe Franciosi
2017-09-20 20:33 ` Marc-André Lureau
@ 2017-09-21 1:47 ` Jason Wang
2017-09-21 10:03 ` Marc-André Lureau
2017-09-25 20:52 ` Michael Roth
3 siblings, 0 replies; 6+ messages in thread
From: Jason Wang @ 2017-09-21 1:47 UTC (permalink / raw)
To: Felipe Franciosi, Marc-Andre Lureau, Michael S. Tsirkin, Paolo Bonzini
Cc: qemu-devel, qemu-stable
On 2017年09月21日 02:53, Felipe Franciosi wrote:
> vhost_log_put() is called to decomission the dirty log between qemu and
> a vhost device when stopping the device. Such a call can happen from
> migration_completion().
>
> Present code sets dev->log_size to zero too early in vhost_log_put(),
> causing the sync check to always return false. As a consequence, the
> last pass on the dirty bitmap never happens at the end of migration.
>
> If a vhost device was busy (writing to guest memory) until the last
> moments before vhost_virtqueue_stop(), this error will result in guest
> memory corruption (at least) following migrations.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
> ---
> hw/virtio/vhost.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5fd69f0..ddc42f0 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -375,8 +375,6 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
> if (!log) {
> return;
> }
> - dev->log = NULL;
> - dev->log_size = 0;
>
> --log->refcnt;
> if (log->refcnt == 0) {
> @@ -396,6 +394,9 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
>
> g_free(log);
> }
> +
> + dev->log = NULL;
> + dev->log_size = 0;
> }
>
> static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
Cc: qemu-stable@nongnu.org
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing
2017-09-20 18:53 [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing Felipe Franciosi
2017-09-20 20:33 ` Marc-André Lureau
2017-09-21 1:47 ` Jason Wang
@ 2017-09-21 10:03 ` Marc-André Lureau
2017-09-25 20:52 ` Michael Roth
3 siblings, 0 replies; 6+ messages in thread
From: Marc-André Lureau @ 2017-09-21 10:03 UTC (permalink / raw)
To: Felipe Franciosi
Cc: Jason Wang, Michael S. Tsirkin, Paolo Bonzini, qemu-devel
----- Original Message -----
> vhost_log_put() is called to decomission the dirty log between qemu and
> a vhost device when stopping the device. Such a call can happen from
> migration_completion().
>
> Present code sets dev->log_size to zero too early in vhost_log_put(),
> causing the sync check to always return false. As a consequence, the
> last pass on the dirty bitmap never happens at the end of migration.
>
> If a vhost device was busy (writing to guest memory) until the last
> moments before vhost_virtqueue_stop(), this error will result in guest
> memory corruption (at least) following migrations.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/virtio/vhost.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5fd69f0..ddc42f0 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -375,8 +375,6 @@ static void vhost_log_put(struct vhost_dev *dev, bool
> sync)
> if (!log) {
> return;
> }
> - dev->log = NULL;
> - dev->log_size = 0;
>
> --log->refcnt;
> if (log->refcnt == 0) {
> @@ -396,6 +394,9 @@ static void vhost_log_put(struct vhost_dev *dev, bool
> sync)
>
> g_free(log);
> }
> +
> + dev->log = NULL;
> + dev->log_size = 0;
> }
>
> static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing
2017-09-20 18:53 [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing Felipe Franciosi
` (2 preceding siblings ...)
2017-09-21 10:03 ` Marc-André Lureau
@ 2017-09-25 20:52 ` Michael Roth
3 siblings, 0 replies; 6+ messages in thread
From: Michael Roth @ 2017-09-25 20:52 UTC (permalink / raw)
To: Michael S. Tsirkin, Felipe Franciosi, Jason Wang,
Marc-Andre Lureau, Paolo Bonzini
Cc: qemu-devel
Quoting Felipe Franciosi (2017-09-20 13:53:06)
> vhost_log_put() is called to decomission the dirty log between qemu and
> a vhost device when stopping the device. Such a call can happen from
> migration_completion().
>
> Present code sets dev->log_size to zero too early in vhost_log_put(),
> causing the sync check to always return false. As a consequence, the
> last pass on the dirty bitmap never happens at the end of migration.
>
> If a vhost device was busy (writing to guest memory) until the last
> moments before vhost_virtqueue_stop(), this error will result in guest
> memory corruption (at least) following migrations.
>
> Signed-off-by: Felipe Franciosi <felipe@nutanix.com>
FYI: this patch has been tagged for stable 2.10.1, but is not yet
upstream. Patch freeze for 2.10.1 is September 27th.
> ---
> hw/virtio/vhost.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5fd69f0..ddc42f0 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -375,8 +375,6 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
> if (!log) {
> return;
> }
> - dev->log = NULL;
> - dev->log_size = 0;
>
> --log->refcnt;
> if (log->refcnt == 0) {
> @@ -396,6 +394,9 @@ static void vhost_log_put(struct vhost_dev *dev, bool sync)
>
> g_free(log);
> }
> +
> + dev->log = NULL;
> + dev->log_size = 0;
> }
>
> static bool vhost_dev_log_is_shared(struct vhost_dev *dev)
> --
> 1.7.1
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-09-25 20:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-20 18:53 [Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing Felipe Franciosi
2017-09-20 20:33 ` Marc-André Lureau
2017-09-20 23:40 ` Felipe Franciosi
2017-09-21 1:47 ` Jason Wang
2017-09-21 10:03 ` Marc-André Lureau
2017-09-25 20:52 ` Michael Roth
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.