Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-14 Thread Jason Wang 



On 2020/5/13 下午5:36, Dima Stepanov wrote:

On Wed, May 13, 2020 at 11:00:38AM +0800, Jason Wang wrote:

On 2020/5/12 下午5:08, Dima Stepanov wrote:

On Tue, May 12, 2020 at 11:26:11AM +0800, Jason Wang wrote:

On 2020/5/11 下午5:11, Dima Stepanov wrote:

On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:

On 2020/4/30 下午9:36, Dima Stepanov wrote:

Since disconnect can happen at any time during initialization not all
vring buffers (for instance used vring) can be intialized successfully.
If the buffer was not initialized then vhost_memory_unmap call will lead
to SIGSEGV. Add checks for the vring address value before calling unmap.
Also add assert() in the vhost_memory_unmap() routine.

Signed-off-by: Dima Stepanov
---
  hw/virtio/vhost.c | 27 +--
  1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ddbdc53..3ee50c4 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void 
*buffer,
 hwaddr len, int is_write,
 hwaddr access_len)
  {
+assert(buffer);
+
  if (!vhost_dev_has_iommu(dev)) {
  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
  }
@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
  vhost_vq_index);
  }
-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
-   1, virtio_queue_get_used_size(vdev, idx));
-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
-   0, virtio_queue_get_avail_size(vdev, idx));
-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
-   0, virtio_queue_get_desc_size(vdev, idx));
+/*
+ * Since the vhost-user disconnect can happen during initialization
+ * check if vring was initialized, before making unmap.
+ */
+if (vq->used) {
+vhost_memory_unmap(dev, vq->used,
+   virtio_queue_get_used_size(vdev, idx),
+   1, virtio_queue_get_used_size(vdev, idx));
+}
+if (vq->avail) {
+vhost_memory_unmap(dev, vq->avail,
+   virtio_queue_get_avail_size(vdev, idx),
+   0, virtio_queue_get_avail_size(vdev, idx));
+}
+if (vq->desc) {
+vhost_memory_unmap(dev, vq->desc,
+   virtio_queue_get_desc_size(vdev, idx),
+   0, virtio_queue_get_desc_size(vdev, idx));
+}

Any reason not checking hdev->started instead? vhost_dev_start() will set it
to true if virtqueues were correctly mapped.

Thanks

Well i see it a little bit different:
  - vhost_dev_start() sets hdev->started to true before starting
virtqueues
  - vhost_virtqueue_start() maps all the memory
If we hit the vhost disconnect at the start of the
vhost_virtqueue_start(), for instance for this call:
   r = dev->vhost_ops->vhost_set_vring_base(dev, );
Then we will call vhost_user_blk_disconnect:
   vhost_user_blk_disconnect()->
 vhost_user_blk_stop()->
   vhost_dev_stop()->
 vhost_virtqueue_stop()
As a result we will come in this routine with the hdev->started still
set to true, but if used/avail/desc fields still uninitialized and set
to 0.

I may miss something, but consider both vhost_dev_start() and
vhost_user_blk_disconnect() were serialized in main loop. Can this really
happen?

Yes, consider the case when we start the vhost-user-blk device:
   vhost_dev_start->
 vhost_virtqueue_start
And we got a disconnect in the middle of vhost_virtqueue_start()
routine, for instance:
   1000 vq->num = state.num = virtio_queue_get_num(vdev, idx);
   1001 r = dev->vhost_ops->vhost_set_vring_num(dev, );
   1002 if (r) {
   1003 VHOST_OPS_DEBUG("vhost_set_vring_num failed");
   1004 return -errno;
   1005 }
   --> Here we got a disconnect <--
   1006
   1007 state.num = virtio_queue_get_last_avail_idx(vdev, idx);
   1008 r = dev->vhost_ops->vhost_set_vring_base(dev, );
   1009 if (r) {
   1010 VHOST_OPS_DEBUG("vhost_set_vring_base failed");
   1011 return -errno;
   1012 }
As a result call to vhost_set_vring_base will call the disconnect
routine. The backtrace log for SIGSEGV is as follows:
   Thread 4 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
   [Switching to Thread 0x72ea9700 (LWP 183150)]
   0x74d60840 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
   (gdb) bt
   #0  0x74d60840 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
   #1  0x5590fd90 in flatview_write_continue (fv=0x7fffec4a2600,
   addr=0, attrs=..., ptr=0x0, len=1028, addr1=0,
   l=1028, mr=0x56b1b310) at ./exec.c:3142
   #2  0x5590fe98 in flatview_write

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-13 Thread Dima Stepanov 
On Wed, May 13, 2020 at 11:00:38AM +0800, Jason Wang wrote:
> 
> On 2020/5/12 下午5:08, Dima Stepanov wrote:
> >On Tue, May 12, 2020 at 11:26:11AM +0800, Jason Wang wrote:
> >>On 2020/5/11 下午5:11, Dima Stepanov wrote:
> >>>On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:
> On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >Since disconnect can happen at any time during initialization not all
> >vring buffers (for instance used vring) can be intialized successfully.
> >If the buffer was not initialized then vhost_memory_unmap call will lead
> >to SIGSEGV. Add checks for the vring address value before calling unmap.
> >Also add assert() in the vhost_memory_unmap() routine.
> >
> >Signed-off-by: Dima Stepanov 
> >---
> >  hw/virtio/vhost.c | 27 +--
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> >
> >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >index ddbdc53..3ee50c4 100644
> >--- a/hw/virtio/vhost.c
> >+++ b/hw/virtio/vhost.c
> >@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev 
> >*dev, void *buffer,
> > hwaddr len, int is_write,
> > hwaddr access_len)
> >  {
> >+assert(buffer);
> >+
> >  if (!vhost_dev_has_iommu(dev)) {
> >  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
> >  }
> >@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct 
> >vhost_dev *dev,
> >  vhost_vq_index);
> >  }
> >-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, 
> >idx),
> >-   1, virtio_queue_get_used_size(vdev, idx));
> >-vhost_memory_unmap(dev, vq->avail, 
> >virtio_queue_get_avail_size(vdev, idx),
> >-   0, virtio_queue_get_avail_size(vdev, idx));
> >-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, 
> >idx),
> >-   0, virtio_queue_get_desc_size(vdev, idx));
> >+/*
> >+ * Since the vhost-user disconnect can happen during initialization
> >+ * check if vring was initialized, before making unmap.
> >+ */
> >+if (vq->used) {
> >+vhost_memory_unmap(dev, vq->used,
> >+   virtio_queue_get_used_size(vdev, idx),
> >+   1, virtio_queue_get_used_size(vdev, idx));
> >+}
> >+if (vq->avail) {
> >+vhost_memory_unmap(dev, vq->avail,
> >+   virtio_queue_get_avail_size(vdev, idx),
> >+   0, virtio_queue_get_avail_size(vdev, idx));
> >+}
> >+if (vq->desc) {
> >+vhost_memory_unmap(dev, vq->desc,
> >+   virtio_queue_get_desc_size(vdev, idx),
> >+   0, virtio_queue_get_desc_size(vdev, idx));
> >+}
> Any reason not checking hdev->started instead? vhost_dev_start() will set 
> it
> to true if virtqueues were correctly mapped.
> 
> Thanks
> >>>Well i see it a little bit different:
> >>>  - vhost_dev_start() sets hdev->started to true before starting
> >>>virtqueues
> >>>  - vhost_virtqueue_start() maps all the memory
> >>>If we hit the vhost disconnect at the start of the
> >>>vhost_virtqueue_start(), for instance for this call:
> >>>   r = dev->vhost_ops->vhost_set_vring_base(dev, );
> >>>Then we will call vhost_user_blk_disconnect:
> >>>   vhost_user_blk_disconnect()->
> >>> vhost_user_blk_stop()->
> >>>   vhost_dev_stop()->
> >>> vhost_virtqueue_stop()
> >>>As a result we will come in this routine with the hdev->started still
> >>>set to true, but if used/avail/desc fields still uninitialized and set
> >>>to 0.
> >>
> >>I may miss something, but consider both vhost_dev_start() and
> >>vhost_user_blk_disconnect() were serialized in main loop. Can this really
> >>happen?
> >Yes, consider the case when we start the vhost-user-blk device:
> >   vhost_dev_start->
> > vhost_virtqueue_start
> >And we got a disconnect in the middle of vhost_virtqueue_start()
> >routine, for instance:
> >   1000 vq->num = state.num = virtio_queue_get_num(vdev, idx);
> >   1001 r = dev->vhost_ops->vhost_set_vring_num(dev, );
> >   1002 if (r) {
> >   1003 VHOST_OPS_DEBUG("vhost_set_vring_num failed");
> >   1004 return -errno;
> >   1005 }
> >   --> Here we got a disconnect <--
> >   1006
> >   1007 state.num = virtio_queue_get_last_avail_idx(vdev, idx);
> >   1008 r = dev->vhost_ops->vhost_set_vring_base(dev, );
> >   1009 if (r) {
> >   1010 VHOST_OPS_DEBUG("vhost_set_vring_base failed");
> >   1011 return -errno;
> >   1012 }
> >As a result call to vhost_set_vring_base will call the disconnect
> >routine. The

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-12 Thread Jason Wang 



On 2020/5/12 下午5:08, Dima Stepanov wrote:

On Tue, May 12, 2020 at 11:26:11AM +0800, Jason Wang wrote:

On 2020/5/11 下午5:11, Dima Stepanov wrote:

On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:

On 2020/4/30 下午9:36, Dima Stepanov wrote:

Since disconnect can happen at any time during initialization not all
vring buffers (for instance used vring) can be intialized successfully.
If the buffer was not initialized then vhost_memory_unmap call will lead
to SIGSEGV. Add checks for the vring address value before calling unmap.
Also add assert() in the vhost_memory_unmap() routine.

Signed-off-by: Dima Stepanov 
---
  hw/virtio/vhost.c | 27 +--
  1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ddbdc53..3ee50c4 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void 
*buffer,
 hwaddr len, int is_write,
 hwaddr access_len)
  {
+assert(buffer);
+
  if (!vhost_dev_has_iommu(dev)) {
  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
  }
@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
  vhost_vq_index);
  }
-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
-   1, virtio_queue_get_used_size(vdev, idx));
-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
-   0, virtio_queue_get_avail_size(vdev, idx));
-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
-   0, virtio_queue_get_desc_size(vdev, idx));
+/*
+ * Since the vhost-user disconnect can happen during initialization
+ * check if vring was initialized, before making unmap.
+ */
+if (vq->used) {
+vhost_memory_unmap(dev, vq->used,
+   virtio_queue_get_used_size(vdev, idx),
+   1, virtio_queue_get_used_size(vdev, idx));
+}
+if (vq->avail) {
+vhost_memory_unmap(dev, vq->avail,
+   virtio_queue_get_avail_size(vdev, idx),
+   0, virtio_queue_get_avail_size(vdev, idx));
+}
+if (vq->desc) {
+vhost_memory_unmap(dev, vq->desc,
+   virtio_queue_get_desc_size(vdev, idx),
+   0, virtio_queue_get_desc_size(vdev, idx));
+}

Any reason not checking hdev->started instead? vhost_dev_start() will set it
to true if virtqueues were correctly mapped.

Thanks

Well i see it a little bit different:
  - vhost_dev_start() sets hdev->started to true before starting
virtqueues
  - vhost_virtqueue_start() maps all the memory
If we hit the vhost disconnect at the start of the
vhost_virtqueue_start(), for instance for this call:
   r = dev->vhost_ops->vhost_set_vring_base(dev, );
Then we will call vhost_user_blk_disconnect:
   vhost_user_blk_disconnect()->
 vhost_user_blk_stop()->
   vhost_dev_stop()->
 vhost_virtqueue_stop()
As a result we will come in this routine with the hdev->started still
set to true, but if used/avail/desc fields still uninitialized and set
to 0.


I may miss something, but consider both vhost_dev_start() and
vhost_user_blk_disconnect() were serialized in main loop. Can this really
happen?

Yes, consider the case when we start the vhost-user-blk device:
   vhost_dev_start->
 vhost_virtqueue_start
And we got a disconnect in the middle of vhost_virtqueue_start()
routine, for instance:
   1000 vq->num = state.num = virtio_queue_get_num(vdev, idx);
   1001 r = dev->vhost_ops->vhost_set_vring_num(dev, );
   1002 if (r) {
   1003 VHOST_OPS_DEBUG("vhost_set_vring_num failed");
   1004 return -errno;
   1005 }
   --> Here we got a disconnect <--
   1006
   1007 state.num = virtio_queue_get_last_avail_idx(vdev, idx);
   1008 r = dev->vhost_ops->vhost_set_vring_base(dev, );
   1009 if (r) {
   1010 VHOST_OPS_DEBUG("vhost_set_vring_base failed");
   1011 return -errno;
   1012 }
As a result call to vhost_set_vring_base will call the disconnect
routine. The backtrace log for SIGSEGV is as follows:
   Thread 4 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
   [Switching to Thread 0x72ea9700 (LWP 183150)]
   0x74d60840 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
   (gdb) bt
   #0  0x74d60840 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
   #1  0x5590fd90 in flatview_write_continue (fv=0x7fffec4a2600,
   addr=0, attrs=..., ptr=0x0, len=1028, addr1=0,
   l=1028, mr=0x56b1b310) at ./exec.c:3142
   #2  0x5590fe98 in flatview_write (fv=0x7fffec4a2600, addr=0,
   attrs=..., buf=0x0, len=1028) at ./exec.c:3177
   #3  0x559101ed in

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-12 Thread Dima Stepanov 
On Tue, May 12, 2020 at 11:26:11AM +0800, Jason Wang wrote:
> 
> On 2020/5/11 下午5:11, Dima Stepanov wrote:
> >On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:
> >>On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >>>Since disconnect can happen at any time during initialization not all
> >>>vring buffers (for instance used vring) can be intialized successfully.
> >>>If the buffer was not initialized then vhost_memory_unmap call will lead
> >>>to SIGSEGV. Add checks for the vring address value before calling unmap.
> >>>Also add assert() in the vhost_memory_unmap() routine.
> >>>
> >>>Signed-off-by: Dima Stepanov 
> >>>---
> >>>  hw/virtio/vhost.c | 27 +--
> >>>  1 file changed, 21 insertions(+), 6 deletions(-)
> >>>
> >>>diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >>>index ddbdc53..3ee50c4 100644
> >>>--- a/hw/virtio/vhost.c
> >>>+++ b/hw/virtio/vhost.c
> >>>@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, 
> >>>void *buffer,
> >>> hwaddr len, int is_write,
> >>> hwaddr access_len)
> >>>  {
> >>>+assert(buffer);
> >>>+
> >>>  if (!vhost_dev_has_iommu(dev)) {
> >>>  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
> >>>  }
> >>>@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev 
> >>>*dev,
> >>>  vhost_vq_index);
> >>>  }
> >>>-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, 
> >>>idx),
> >>>-   1, virtio_queue_get_used_size(vdev, idx));
> >>>-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, 
> >>>idx),
> >>>-   0, virtio_queue_get_avail_size(vdev, idx));
> >>>-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, 
> >>>idx),
> >>>-   0, virtio_queue_get_desc_size(vdev, idx));
> >>>+/*
> >>>+ * Since the vhost-user disconnect can happen during initialization
> >>>+ * check if vring was initialized, before making unmap.
> >>>+ */
> >>>+if (vq->used) {
> >>>+vhost_memory_unmap(dev, vq->used,
> >>>+   virtio_queue_get_used_size(vdev, idx),
> >>>+   1, virtio_queue_get_used_size(vdev, idx));
> >>>+}
> >>>+if (vq->avail) {
> >>>+vhost_memory_unmap(dev, vq->avail,
> >>>+   virtio_queue_get_avail_size(vdev, idx),
> >>>+   0, virtio_queue_get_avail_size(vdev, idx));
> >>>+}
> >>>+if (vq->desc) {
> >>>+vhost_memory_unmap(dev, vq->desc,
> >>>+   virtio_queue_get_desc_size(vdev, idx),
> >>>+   0, virtio_queue_get_desc_size(vdev, idx));
> >>>+}
> >>
> >>Any reason not checking hdev->started instead? vhost_dev_start() will set it
> >>to true if virtqueues were correctly mapped.
> >>
> >>Thanks
> >Well i see it a little bit different:
> >  - vhost_dev_start() sets hdev->started to true before starting
> >virtqueues
> >  - vhost_virtqueue_start() maps all the memory
> >If we hit the vhost disconnect at the start of the
> >vhost_virtqueue_start(), for instance for this call:
> >   r = dev->vhost_ops->vhost_set_vring_base(dev, );
> >Then we will call vhost_user_blk_disconnect:
> >   vhost_user_blk_disconnect()->
> > vhost_user_blk_stop()->
> >   vhost_dev_stop()->
> > vhost_virtqueue_stop()
> >As a result we will come in this routine with the hdev->started still
> >set to true, but if used/avail/desc fields still uninitialized and set
> >to 0.
> 
> 
> I may miss something, but consider both vhost_dev_start() and
> vhost_user_blk_disconnect() were serialized in main loop. Can this really
> happen?
Yes, consider the case when we start the vhost-user-blk device:
  vhost_dev_start->
vhost_virtqueue_start
And we got a disconnect in the middle of vhost_virtqueue_start()
routine, for instance:
  1000 vq->num = state.num = virtio_queue_get_num(vdev, idx);
  1001 r = dev->vhost_ops->vhost_set_vring_num(dev, );
  1002 if (r) {
  1003 VHOST_OPS_DEBUG("vhost_set_vring_num failed");
  1004 return -errno;
  1005 }
  --> Here we got a disconnect <--
  1006 
  1007 state.num = virtio_queue_get_last_avail_idx(vdev, idx);
  1008 r = dev->vhost_ops->vhost_set_vring_base(dev, );
  1009 if (r) {
  1010 VHOST_OPS_DEBUG("vhost_set_vring_base failed");
  1011 return -errno;
  1012 }
As a result call to vhost_set_vring_base will call the disconnect
routine. The backtrace log for SIGSEGV is as follows:
  Thread 4 "qemu-system-x86" received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 0x72ea9700 (LWP 183150)]
  0x74d60840 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
  (gdb) bt
  #0  0x74d60840 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
  #1  0x5590fd90 in

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-11 Thread Jason Wang 



On 2020/5/11 下午5:11, Dima Stepanov wrote:

On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:

On 2020/4/30 下午9:36, Dima Stepanov wrote:

Since disconnect can happen at any time during initialization not all
vring buffers (for instance used vring) can be intialized successfully.
If the buffer was not initialized then vhost_memory_unmap call will lead
to SIGSEGV. Add checks for the vring address value before calling unmap.
Also add assert() in the vhost_memory_unmap() routine.

Signed-off-by: Dima Stepanov 
---
  hw/virtio/vhost.c | 27 +--
  1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ddbdc53..3ee50c4 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void 
*buffer,
 hwaddr len, int is_write,
 hwaddr access_len)
  {
+assert(buffer);
+
  if (!vhost_dev_has_iommu(dev)) {
  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
  }
@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
  vhost_vq_index);
  }
-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
-   1, virtio_queue_get_used_size(vdev, idx));
-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
-   0, virtio_queue_get_avail_size(vdev, idx));
-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
-   0, virtio_queue_get_desc_size(vdev, idx));
+/*
+ * Since the vhost-user disconnect can happen during initialization
+ * check if vring was initialized, before making unmap.
+ */
+if (vq->used) {
+vhost_memory_unmap(dev, vq->used,
+   virtio_queue_get_used_size(vdev, idx),
+   1, virtio_queue_get_used_size(vdev, idx));
+}
+if (vq->avail) {
+vhost_memory_unmap(dev, vq->avail,
+   virtio_queue_get_avail_size(vdev, idx),
+   0, virtio_queue_get_avail_size(vdev, idx));
+}
+if (vq->desc) {
+vhost_memory_unmap(dev, vq->desc,
+   virtio_queue_get_desc_size(vdev, idx),
+   0, virtio_queue_get_desc_size(vdev, idx));
+}


Any reason not checking hdev->started instead? vhost_dev_start() will set it
to true if virtqueues were correctly mapped.

Thanks

Well i see it a little bit different:
  - vhost_dev_start() sets hdev->started to true before starting
virtqueues
  - vhost_virtqueue_start() maps all the memory
If we hit the vhost disconnect at the start of the
vhost_virtqueue_start(), for instance for this call:
   r = dev->vhost_ops->vhost_set_vring_base(dev, );
Then we will call vhost_user_blk_disconnect:
   vhost_user_blk_disconnect()->
 vhost_user_blk_stop()->
   vhost_dev_stop()->
 vhost_virtqueue_stop()
As a result we will come in this routine with the hdev->started still
set to true, but if used/avail/desc fields still uninitialized and set
to 0.



I may miss something, but consider both vhost_dev_start() and 
vhost_user_blk_disconnect() were serialized in main loop. Can this 
really happen?


Thanks







  }
  static void vhost_eventfd_add(MemoryListener *listener,

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-11 Thread Dima Stepanov 
On Mon, May 11, 2020 at 11:05:58AM +0800, Jason Wang wrote:
> 
> On 2020/4/30 下午9:36, Dima Stepanov wrote:
> >Since disconnect can happen at any time during initialization not all
> >vring buffers (for instance used vring) can be intialized successfully.
> >If the buffer was not initialized then vhost_memory_unmap call will lead
> >to SIGSEGV. Add checks for the vring address value before calling unmap.
> >Also add assert() in the vhost_memory_unmap() routine.
> >
> >Signed-off-by: Dima Stepanov 
> >---
> >  hw/virtio/vhost.c | 27 +--
> >  1 file changed, 21 insertions(+), 6 deletions(-)
> >
> >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >index ddbdc53..3ee50c4 100644
> >--- a/hw/virtio/vhost.c
> >+++ b/hw/virtio/vhost.c
> >@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, 
> >void *buffer,
> > hwaddr len, int is_write,
> > hwaddr access_len)
> >  {
> >+assert(buffer);
> >+
> >  if (!vhost_dev_has_iommu(dev)) {
> >  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
> >  }
> >@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev 
> >*dev,
> >  vhost_vq_index);
> >  }
> >-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
> >-   1, virtio_queue_get_used_size(vdev, idx));
> >-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, 
> >idx),
> >-   0, virtio_queue_get_avail_size(vdev, idx));
> >-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
> >-   0, virtio_queue_get_desc_size(vdev, idx));
> >+/*
> >+ * Since the vhost-user disconnect can happen during initialization
> >+ * check if vring was initialized, before making unmap.
> >+ */
> >+if (vq->used) {
> >+vhost_memory_unmap(dev, vq->used,
> >+   virtio_queue_get_used_size(vdev, idx),
> >+   1, virtio_queue_get_used_size(vdev, idx));
> >+}
> >+if (vq->avail) {
> >+vhost_memory_unmap(dev, vq->avail,
> >+   virtio_queue_get_avail_size(vdev, idx),
> >+   0, virtio_queue_get_avail_size(vdev, idx));
> >+}
> >+if (vq->desc) {
> >+vhost_memory_unmap(dev, vq->desc,
> >+   virtio_queue_get_desc_size(vdev, idx),
> >+   0, virtio_queue_get_desc_size(vdev, idx));
> >+}
> 
> 
> Any reason not checking hdev->started instead? vhost_dev_start() will set it
> to true if virtqueues were correctly mapped.
> 
> Thanks
Well i see it a little bit different:
 - vhost_dev_start() sets hdev->started to true before starting
   virtqueues
 - vhost_virtqueue_start() maps all the memory
If we hit the vhost disconnect at the start of the
vhost_virtqueue_start(), for instance for this call:
  r = dev->vhost_ops->vhost_set_vring_base(dev, );
Then we will call vhost_user_blk_disconnect:
  vhost_user_blk_disconnect()->
vhost_user_blk_stop()->
  vhost_dev_stop()->
vhost_virtqueue_stop()
As a result we will come in this routine with the hdev->started still
set to true, but if used/avail/desc fields still uninitialized and set
to 0.

> 
> 
> >  }
> >  static void vhost_eventfd_add(MemoryListener *listener,
>

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-10 Thread Jason Wang 



On 2020/4/30 下午9:36, Dima Stepanov wrote:

Since disconnect can happen at any time during initialization not all
vring buffers (for instance used vring) can be intialized successfully.
If the buffer was not initialized then vhost_memory_unmap call will lead
to SIGSEGV. Add checks for the vring address value before calling unmap.
Also add assert() in the vhost_memory_unmap() routine.

Signed-off-by: Dima Stepanov 
---
  hw/virtio/vhost.c | 27 +--
  1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ddbdc53..3ee50c4 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void 
*buffer,
 hwaddr len, int is_write,
 hwaddr access_len)
  {
+assert(buffer);
+
  if (!vhost_dev_has_iommu(dev)) {
  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
  }
@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
  vhost_vq_index);
  }
  
-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),

-   1, virtio_queue_get_used_size(vdev, idx));
-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
-   0, virtio_queue_get_avail_size(vdev, idx));
-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
-   0, virtio_queue_get_desc_size(vdev, idx));
+/*
+ * Since the vhost-user disconnect can happen during initialization
+ * check if vring was initialized, before making unmap.
+ */
+if (vq->used) {
+vhost_memory_unmap(dev, vq->used,
+   virtio_queue_get_used_size(vdev, idx),
+   1, virtio_queue_get_used_size(vdev, idx));
+}
+if (vq->avail) {
+vhost_memory_unmap(dev, vq->avail,
+   virtio_queue_get_avail_size(vdev, idx),
+   0, virtio_queue_get_avail_size(vdev, idx));
+}
+if (vq->desc) {
+vhost_memory_unmap(dev, vq->desc,
+   virtio_queue_get_desc_size(vdev, idx),
+   0, virtio_queue_get_desc_size(vdev, idx));
+}



Any reason not checking hdev->started instead? vhost_dev_start() will 
set it to true if virtqueues were correctly mapped.


Thanks



  }
  
  static void vhost_eventfd_add(MemoryListener *listener,

Re: [PATCH v2 4/5] vhost: check vring address before calling unmap

2020-05-03 Thread Raphael Norwitz 
On Thu, Apr 30, 2020 at 9:50 AM Dima Stepanov  wrote:
>
> Since disconnect can happen at any time during initialization not all
> vring buffers (for instance used vring) can be intialized successfully.
> If the buffer was not initialized then vhost_memory_unmap call will lead
> to SIGSEGV. Add checks for the vring address value before calling unmap.
> Also add assert() in the vhost_memory_unmap() routine.
>
> Signed-off-by: Dima Stepanov 

Reviewed-by: Raphael Norwitz 

> ---
>  hw/virtio/vhost.c | 27 +--
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index ddbdc53..3ee50c4 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, 
> void *buffer,
> hwaddr len, int is_write,
> hwaddr access_len)
>  {
> +assert(buffer);
> +
>  if (!vhost_dev_has_iommu(dev)) {
>  cpu_physical_memory_unmap(buffer, len, is_write, access_len);
>  }
> @@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev 
> *dev,
>  vhost_vq_index);
>  }
>
> -vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
> -   1, virtio_queue_get_used_size(vdev, idx));
> -vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, 
> idx),
> -   0, virtio_queue_get_avail_size(vdev, idx));
> -vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
> -   0, virtio_queue_get_desc_size(vdev, idx));
> +/*
> + * Since the vhost-user disconnect can happen during initialization
> + * check if vring was initialized, before making unmap.
> + */
> +if (vq->used) {
> +vhost_memory_unmap(dev, vq->used,
> +   virtio_queue_get_used_size(vdev, idx),
> +   1, virtio_queue_get_used_size(vdev, idx));
> +}
> +if (vq->avail) {
> +vhost_memory_unmap(dev, vq->avail,
> +   virtio_queue_get_avail_size(vdev, idx),
> +   0, virtio_queue_get_avail_size(vdev, idx));
> +}
> +if (vq->desc) {
> +vhost_memory_unmap(dev, vq->desc,
> +   virtio_queue_get_desc_size(vdev, idx),
> +   0, virtio_queue_get_desc_size(vdev, idx));
> +}
>  }
>
>  static void vhost_eventfd_add(MemoryListener *listener,
> --
> 2.7.4
>
>

[PATCH v2 4/5] vhost: check vring address before calling unmap

2020-04-30 Thread Dima Stepanov 
Since disconnect can happen at any time during initialization not all
vring buffers (for instance used vring) can be intialized successfully.
If the buffer was not initialized then vhost_memory_unmap call will lead
to SIGSEGV. Add checks for the vring address value before calling unmap.
Also add assert() in the vhost_memory_unmap() routine.

Signed-off-by: Dima Stepanov 
---
 hw/virtio/vhost.c | 27 +--
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index ddbdc53..3ee50c4 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -314,6 +314,8 @@ static void vhost_memory_unmap(struct vhost_dev *dev, void 
*buffer,
hwaddr len, int is_write,
hwaddr access_len)
 {
+assert(buffer);
+
 if (!vhost_dev_has_iommu(dev)) {
 cpu_physical_memory_unmap(buffer, len, is_write, access_len);
 }
@@ -1132,12 +1134,25 @@ static void vhost_virtqueue_stop(struct vhost_dev *dev,
 vhost_vq_index);
 }
 
-vhost_memory_unmap(dev, vq->used, virtio_queue_get_used_size(vdev, idx),
-   1, virtio_queue_get_used_size(vdev, idx));
-vhost_memory_unmap(dev, vq->avail, virtio_queue_get_avail_size(vdev, idx),
-   0, virtio_queue_get_avail_size(vdev, idx));
-vhost_memory_unmap(dev, vq->desc, virtio_queue_get_desc_size(vdev, idx),
-   0, virtio_queue_get_desc_size(vdev, idx));
+/*
+ * Since the vhost-user disconnect can happen during initialization
+ * check if vring was initialized, before making unmap.
+ */
+if (vq->used) {
+vhost_memory_unmap(dev, vq->used,
+   virtio_queue_get_used_size(vdev, idx),
+   1, virtio_queue_get_used_size(vdev, idx));
+}
+if (vq->avail) {
+vhost_memory_unmap(dev, vq->avail,
+   virtio_queue_get_avail_size(vdev, idx),
+   0, virtio_queue_get_avail_size(vdev, idx));
+}
+if (vq->desc) {
+vhost_memory_unmap(dev, vq->desc,
+   virtio_queue_get_desc_size(vdev, idx),
+   0, virtio_queue_get_desc_size(vdev, idx));
+}
 }
 
 static void vhost_eventfd_add(MemoryListener *listener,
-- 
2.7.4