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

[Felipe Franciosi]

Heya,

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