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

[Michael Roth]

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 

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
> 
> 

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

[Marc-André Lureau]

- 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 

Reviewed-by: Marc-André Lureau 

> ---
>  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
> 
> 

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

[Jason Wang]

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 
---
  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-sta...@nongnu.org

Acked-by: Jason Wang 

Thanks

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

[Felipe Franciosi]

Heya,

> On 20 Sep 2017, at 13:33, Marc-André Lureau  
> 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 
>> ---
>> 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
>> 
>> 
> 

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

[Marc-André Lureau]

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 
> ---
>  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
> 
> 

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

[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 
---
 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