Re: [PATCH] hw/virtio/vhost: check nvqs at dev_start

[Albert Esteve]

Ah I see, I wanted to move the fail check as early as possible, and went a bit too far ahead, before initialisation.

But is ok, it needs its own value either way. What about returning -EFAULT? Or maybe -EINVAL? I think they would fit for this error.

And then I can use `VHOST_OPS_DEBUG` to make it consistent and print the error number.

On Mon, Oct 2, 2023 at 11:27 AM Michael S. Tsirkin <mst@redhat.com> wrote:

On Fri, Sep 01, 2023 at 02:23:23PM +0200, Albert Esteve wrote:
> While this is not expected to happen, it could still
> be that a vhost_dev did not set its nvqs member.
>
> Since `vhost_dev_start` access the device's vqs array
> later without checking its size, it would cause a
> Segmentation fault when nvqs is 0.
>
> To avoid this `rare` case and made the code safer,
> add a clause that ensures nvqs has been set, and
> warn the user if it has not.
>
> Signed-off-by: Albert Esteve <aesteve@redhat.com>
> ---
>  hw/virtio/vhost.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index e2f6ffb446..78805fe5b7 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1935,6 +1935,11 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings)
>      hdev->started = true;
>      hdev->vdev = vdev;
> 
> +    if (!hdev->nvqs) {
> +        error_report("device nvqs not set");
> +        goto fail_nvqs;
> +    }
> +
>      r = vhost_dev_set_features(hdev, hdev->log_enabled);
>      if (r < 0) {
>          goto fail_features;
> @@ -2028,6 +2033,7 @@ fail_mem:
>      if (vhost_dev_has_iommu(hdev)) {
>          memory_listener_unregister(&hdev->iommu_listener);
>      }
> +fail_nvqs:
>  fail_features:
>      vdev->vhost_started = false;
>      hdev->started = false;

What do we want to return in this case?
ATM the value we return (r) will be uninitialized.

> --
> 2.41.0