| Message ID | 1429090543-4736-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 2015/4/15 17:37, Michael S. Tsirkin wrote: > VHOST_SET_LOG_BASE got an incorrect address, causing > migration errors and potentially even memory corruption. > > Cc: Peter Maydell <peter.maydell@linaro.org> > Reported-by: Wen Congyang <wency@cn.fujitsu.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Could you please confirm this fixes the problem for you? > > hw/virtio/vhost.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 8dd2f59..02c5604 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1016,10 +1016,13 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) > } > > if (hdev->log_enabled) { > + uint64_t log_base; > + > hdev->log_size = vhost_get_log_size(hdev); > hdev->log = hdev->log_size ? > g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL; > - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, hdev->log); > + log_base = (uint64_t)(unsigned long)log_base; ^^^^^^^^ s/log_base/hdev->log ? > + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, &log_base); > if (r < 0) { > r = -errno; > goto fail_log; >
On 04/15/2015 05:56 PM, zhanghailiang wrote: > On 2015/4/15 17:37, Michael S. Tsirkin wrote: >> VHOST_SET_LOG_BASE got an incorrect address, causing >> migration errors and potentially even memory corruption. >> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Reported-by: Wen Congyang <wency@cn.fujitsu.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> >> Could you please confirm this fixes the problem for you? >> >> hw/virtio/vhost.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index 8dd2f59..02c5604 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -1016,10 +1016,13 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) >> } >> >> if (hdev->log_enabled) { >> + uint64_t log_base; >> + >> hdev->log_size = vhost_get_log_size(hdev); >> hdev->log = hdev->log_size ? >> g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL; >> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, hdev->log); >> + log_base = (uint64_t)(unsigned long)log_base; > ^^^^^^^^ > > s/log_base/hdev->log ? I test the patch with this modification. It works for me. Thanks Wen Congyang > >> + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, &log_base); >> if (r < 0) { >> r = -errno; >> goto fail_log; >> > > > . >
On 15/04/2015 11:56, zhanghailiang wrote: > On 2015/4/15 17:37, Michael S. Tsirkin wrote: >> VHOST_SET_LOG_BASE got an incorrect address, causing >> migration errors and potentially even memory corruption. >> >> Cc: Peter Maydell <peter.maydell@linaro.org> >> Reported-by: Wen Congyang <wency@cn.fujitsu.com> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> --- >> >> Could you please confirm this fixes the problem for you? >> >> hw/virtio/vhost.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >> index 8dd2f59..02c5604 100644 >> --- a/hw/virtio/vhost.c >> +++ b/hw/virtio/vhost.c >> @@ -1016,10 +1016,13 @@ int vhost_dev_start(struct vhost_dev *hdev, >> VirtIODevice *vdev) >> } >> >> if (hdev->log_enabled) { >> + uint64_t log_base; >> + >> hdev->log_size = vhost_get_log_size(hdev); >> hdev->log = hdev->log_size ? >> g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL; >> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, >> hdev->log); >> + log_base = (uint64_t)(unsigned long)log_base; > ^^^^^^^^ > > s/log_base/hdev->log ? Also s/unsigned long/uintptr_t/ please. The subsequent cast to uint64_t is not necessary. Paolo >> + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, >> &log_base); >> if (r < 0) { >> r = -errno; >> goto fail_log; >> > > > >
On 04/17/2015 05:26 AM, Paolo Bonzini wrote: > > > On 15/04/2015 11:56, zhanghailiang wrote: >> On 2015/4/15 17:37, Michael S. Tsirkin wrote: >>> VHOST_SET_LOG_BASE got an incorrect address, causing >>> migration errors and potentially even memory corruption. >>> >>> Cc: Peter Maydell <peter.maydell@linaro.org> >>> Reported-by: Wen Congyang <wency@cn.fujitsu.com> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> >>> Could you please confirm this fixes the problem for you? >>> >>> hw/virtio/vhost.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>> index 8dd2f59..02c5604 100644 >>> --- a/hw/virtio/vhost.c >>> +++ b/hw/virtio/vhost.c >>> @@ -1016,10 +1016,13 @@ int vhost_dev_start(struct vhost_dev *hdev, >>> VirtIODevice *vdev) >>> } >>> >>> if (hdev->log_enabled) { >>> + uint64_t log_base; >>> + >>> hdev->log_size = vhost_get_log_size(hdev); >>> hdev->log = hdev->log_size ? >>> g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL; >>> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, >>> hdev->log); >>> + log_base = (uint64_t)(unsigned long)log_base; >> ^^^^^^^^ >> >> s/log_base/hdev->log ? > > Also s/unsigned long/uintptr_t/ please. The subsequent cast to uint64_t > is not necessary. Should we also update vhost_dev_log_resize()? Thanks Wen Congyang > > Paolo > >>> + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, >>> &log_base); >>> if (r < 0) { >>> r = -errno; >>> goto fail_log; >>> >> >> >> >> > >
On 16 April 2015 at 22:26, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 15/04/2015 11:56, zhanghailiang wrote: >> On 2015/4/15 17:37, Michael S. Tsirkin wrote: >>> VHOST_SET_LOG_BASE got an incorrect address, causing >>> migration errors and potentially even memory corruption. >>> >>> Cc: Peter Maydell <peter.maydell@linaro.org> >>> Reported-by: Wen Congyang <wency@cn.fujitsu.com> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> >>> Could you please confirm this fixes the problem for you? >>> >>> hw/virtio/vhost.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >>> index 8dd2f59..02c5604 100644 >>> --- a/hw/virtio/vhost.c >>> +++ b/hw/virtio/vhost.c >>> @@ -1016,10 +1016,13 @@ int vhost_dev_start(struct vhost_dev *hdev, >>> VirtIODevice *vdev) >>> } >>> >>> if (hdev->log_enabled) { >>> + uint64_t log_base; >>> + >>> hdev->log_size = vhost_get_log_size(hdev); >>> hdev->log = hdev->log_size ? >>> g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL; >>> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, >>> hdev->log); >>> + log_base = (uint64_t)(unsigned long)log_base; >> ^^^^^^^^ >> >> s/log_base/hdev->log ? > > Also s/unsigned long/uintptr_t/ please. The subsequent cast to uint64_t > is not necessary. I think this is our remaining for-2.3 bug; would somebody like to produce and test a patch with all the fixes mentioned in this thread? thanks -- PMM
On Fri, Apr 17, 2015 at 03:40:50PM +0100, Peter Maydell wrote: > On 16 April 2015 at 22:26, Paolo Bonzini <pbonzini@redhat.com> wrote: > > > > > > On 15/04/2015 11:56, zhanghailiang wrote: > >> On 2015/4/15 17:37, Michael S. Tsirkin wrote: > >>> VHOST_SET_LOG_BASE got an incorrect address, causing > >>> migration errors and potentially even memory corruption. > >>> > >>> Cc: Peter Maydell <peter.maydell@linaro.org> > >>> Reported-by: Wen Congyang <wency@cn.fujitsu.com> > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> --- > >>> > >>> Could you please confirm this fixes the problem for you? > >>> > >>> hw/virtio/vhost.c | 5 ++++- > >>> 1 file changed, 4 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > >>> index 8dd2f59..02c5604 100644 > >>> --- a/hw/virtio/vhost.c > >>> +++ b/hw/virtio/vhost.c > >>> @@ -1016,10 +1016,13 @@ int vhost_dev_start(struct vhost_dev *hdev, > >>> VirtIODevice *vdev) > >>> } > >>> > >>> if (hdev->log_enabled) { > >>> + uint64_t log_base; > >>> + > >>> hdev->log_size = vhost_get_log_size(hdev); > >>> hdev->log = hdev->log_size ? > >>> g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL; > >>> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, > >>> hdev->log); > >>> + log_base = (uint64_t)(unsigned long)log_base; > >> ^^^^^^^^ > >> > >> s/log_base/hdev->log ? > > > > Also s/unsigned long/uintptr_t/ please. The subsequent cast to uint64_t > > is not necessary. > > I think this is our remaining for-2.3 bug; would somebody like > to produce and test a patch with all the fixes mentioned in > this thread? > > thanks > -- PMM I just posted it but it's not tested yet.
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 8dd2f59..02c5604 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1016,10 +1016,13 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev) } if (hdev->log_enabled) { + uint64_t log_base; + hdev->log_size = vhost_get_log_size(hdev); hdev->log = hdev->log_size ? g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL; - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, hdev->log); + log_base = (uint64_t)(unsigned long)log_base; + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE, &log_base); if (r < 0) { r = -errno; goto fail_log;
VHOST_SET_LOG_BASE got an incorrect address, causing migration errors and potentially even memory corruption. Cc: Peter Maydell <peter.maydell@linaro.org> Reported-by: Wen Congyang <wency@cn.fujitsu.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Could you please confirm this fixes the problem for you? hw/virtio/vhost.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)