[Qemu-devel] [PATCH] virtio/vhost: reset dev->log after syncing

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).