| Message ID | 1505933586-11296-1-git-send-email-felipe@nutanix.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio/vhost: reset dev->log after syncing | expand |
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 > >
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 >> >> >
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
----- 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 > >
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 > >
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)
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(-)