Heya,
> On 20 Sep 2017, at 13:33, Marc-André Lureau <marcandre.lur...@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 <fel...@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
>>
>>
>
