[Qemu-devel] [PATCH 0/2] vhost-user: fix crashes on disconnect when iommu is on

[Qemu-devel] [PATCH 0/2] vhost-user: fix crashes on disconnect when iommu is on

[Maxime Coquelin]

This two patches series aims at fixing a couple of crashes
that happens when the vhost-user socket is closed and iommu
enabled.

Maxime Coquelin (2):
  vhost: ensure vhost_ops are set before calling iotlb callback
  vhost-user: unregister slave req handler at cleanup time

 hw/virtio/vhost-backend.c | 10 ++++++++--
 hw/virtio/vhost-user.c    |  1 +
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.9.4

[Qemu-devel] [PATCH 1/2] vhost: ensure vhost_ops are set before calling iotlb callback

[Maxime Coquelin]

This patch fixes a crash that happens when vhost-user iommu
support is enabled and vhost-user socket is closed.

When it happens, if an IOTLB invalidation notification is sent
by the IOMMU, vhost_ops's NULL pointer is dereferenced.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/virtio/vhost-backend.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 4e31de1..cb055e8 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -309,7 +309,10 @@ int vhost_backend_update_device_iotlb(struct vhost_dev *dev,
         return -EINVAL;
     }
 
-    return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
+    if (dev->vhost_ops && dev->vhost_ops->vhost_send_device_iotlb_msg)
+        return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
+
+    return -ENODEV;
 }
 
 int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev,
@@ -321,7 +324,10 @@ int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev,
     imsg.size = len;
     imsg.type = VHOST_IOTLB_INVALIDATE;
 
-    return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
+    if (dev->vhost_ops && dev->vhost_ops->vhost_send_device_iotlb_msg)
+        return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
+
+    return -ENODEV;
 }
 
 int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
-- 
2.9.4

Re: [Qemu-devel] [PATCH 1/2] vhost: ensure vhost_ops are set before calling iotlb callback

[Marc-André Lureau]

----- Original Message -----
> This patch fixes a crash that happens when vhost-user iommu
> support is enabled and vhost-user socket is closed.
> 
> When it happens, if an IOTLB invalidation notification is sent
> by the IOMMU, vhost_ops's NULL pointer is dereferenced.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

looks fine to me,

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  hw/virtio/vhost-backend.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
> index 4e31de1..cb055e8 100644
> --- a/hw/virtio/vhost-backend.c
> +++ b/hw/virtio/vhost-backend.c
> @@ -309,7 +309,10 @@ int vhost_backend_update_device_iotlb(struct vhost_dev
> *dev,
>          return -EINVAL;
>      }
>  
> -    return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
> +    if (dev->vhost_ops && dev->vhost_ops->vhost_send_device_iotlb_msg)
> +        return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
> +
> +    return -ENODEV;
>  }
>  
>  int vhost_backend_invalidate_device_iotlb(struct vhost_dev *dev,
> @@ -321,7 +324,10 @@ int vhost_backend_invalidate_device_iotlb(struct
> vhost_dev *dev,
>      imsg.size = len;
>      imsg.type = VHOST_IOTLB_INVALIDATE;
>  
> -    return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
> +    if (dev->vhost_ops && dev->vhost_ops->vhost_send_device_iotlb_msg)
> +        return dev->vhost_ops->vhost_send_device_iotlb_msg(dev, &imsg);
> +
> +    return -ENODEV;
>  }
>  
>  int vhost_backend_handle_iotlb_msg(struct vhost_dev *dev,
> --
> 2.9.4
> 
> 

[Qemu-devel] [PATCH 2/2] vhost-user: unregister slave req handler at cleanup time

[Maxime Coquelin]

If the backend sends a request just before closing the socket,
the aio dispatcher might schedule its reading after the vhost
device has been cleaned, leading to a NULL pointer dereference
in slave_read();

vhost_user_cleanup() already closes the socket but it is not
enough, the handler has to be unregistered.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 hw/virtio/vhost-user.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 958ee09..2203011 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -779,6 +779,7 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
 
     u = dev->opaque;
     if (u->slave_fd >= 0) {
+        qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
         close(u->slave_fd);
         u->slave_fd = -1;
     }
-- 
2.9.4

Re: [Qemu-devel] [PATCH 2/2] vhost-user: unregister slave req handler at cleanup time

[Marc-André Lureau]

----- Original Message -----
> If the backend sends a request just before closing the socket,
> the aio dispatcher might schedule its reading after the vhost
> device has been cleaned, leading to a NULL pointer dereference
> in slave_read();
> 
> vhost_user_cleanup() already closes the socket but it is not
> enough, the handler has to be unregistered.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  hw/virtio/vhost-user.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 958ee09..2203011 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -779,6 +779,7 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
>  
>      u = dev->opaque;
>      if (u->slave_fd >= 0) {
> +        qemu_set_fd_handler(u->slave_fd, NULL, NULL, NULL);
>          close(u->slave_fd);
>          u->slave_fd = -1;
>      }
> --
> 2.9.4
> 
>