RE: [PATCH] Revert "vhost-blk: set features before setting inflight feature"

[Yu, Jin]

Hi  Stefan,
        I have sent a version 2 and it should fix this issue.
        I also test it several times and I did not meet this issue again.

        Thanks for your report.

Best Regards
Jin

> -----Original Message-----
> From: Stefan Hajnoczi <stefanha@redhat.com>
> Sent: Tuesday, November 3, 2020 12:57 AM
> To: qemu-devel@nongnu.org
> Cc: qemu-block@nongnu.org; Raphael Norwitz
> <raphael.norwitz@nutanix.com>; Max Reitz <mreitz@redhat.com>; Kevin
> Wolf <kwolf@redhat.com>; Michael S. Tsirkin <mst@redhat.com>; Stefan
> Hajnoczi <stefanha@redhat.com>; Yu, Jin <jin.yu@intel.com>
> Subject: [PATCH] Revert "vhost-blk: set features before setting inflight
> feature"
> 
> This reverts commit adb29c027341ba095a3ef4beef6aaef86d3a520e.
> 
> The commit broke -device vhost-user-blk-pci because the
> vhost_dev_prepare_inflight() function it introduced segfaults in
> vhost_dev_set_features() when attempting to access struct vhost_dev's vdev
> pointer before it has been assigned.
> 
> To reproduce the segfault simply launch a vhost-user-blk device with the
> contrib vhost-user-blk device backend:
> 
>   $ build/contrib/vhost-user-blk/vhost-user-blk -s /tmp/vhost-user-blk.sock -r
> -b /var/tmp/foo.img
>   $ build/qemu-system-x86_64 \
>         -device vhost-user-blk-pci,id=drv0,chardev=char1,addr=4.0 \
>         -object memory-backend-memfd,id=mem,size=1G,share=on \
>         -M memory-backend=mem,accel=kvm \
>         -chardev socket,id=char1,path=/tmp/vhost-user-blk.sock
>   Segmentation fault (core dumped)
> 
> Cc: Jin Yu <jin.yu@intel.com>
> Cc: Raphael Norwitz <raphael.norwitz@nutanix.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/hw/virtio/vhost.h |  1 -
>  hw/block/vhost-user-blk.c |  6 ------
>  hw/virtio/vhost.c         | 18 ------------------
>  3 files changed, 25 deletions(-)
> 
> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h index
> 839bfb153c..94585067f7 100644
> --- a/include/hw/virtio/vhost.h
> +++ b/include/hw/virtio/vhost.h
> @@ -141,7 +141,6 @@ void vhost_dev_reset_inflight(struct vhost_inflight
> *inflight);  void vhost_dev_free_inflight(struct vhost_inflight *inflight);  
> void
> vhost_dev_save_inflight(struct vhost_inflight *inflight, QEMUFile *f);  int
> vhost_dev_load_inflight(struct vhost_inflight *inflight, QEMUFile *f); -int
> vhost_dev_prepare_inflight(struct vhost_dev *hdev);  int
> vhost_dev_set_inflight(struct vhost_dev *dev,
>                             struct vhost_inflight *inflight);  int
> vhost_dev_get_inflight(struct vhost_dev *dev, uint16_t queue_size, diff --git
> a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c index
> f67b29bbf3..a076b1e54d 100644
> --- a/hw/block/vhost-user-blk.c
> +++ b/hw/block/vhost-user-blk.c
> @@ -131,12 +131,6 @@ static int vhost_user_blk_start(VirtIODevice *vdev)
> 
>      s->dev.acked_features = vdev->guest_features;
> 
> -    ret = vhost_dev_prepare_inflight(&s->dev);
> -    if (ret < 0) {
> -        error_report("Error set inflight format: %d", -ret);
> -        goto err_guest_notifiers;
> -    }
> -
>      if (!s->inflight->addr) {
>          ret = vhost_dev_get_inflight(&s->dev, s->queue_size, s->inflight);
>          if (ret < 0) {
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index
> f2482378c6..79b2be20df 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1645,24 +1645,6 @@ int vhost_dev_load_inflight(struct vhost_inflight
> *inflight, QEMUFile *f)
>      return 0;
>  }
> 
> -int vhost_dev_prepare_inflight(struct vhost_dev *hdev) -{
> -    int r;
> -
> -    if (hdev->vhost_ops->vhost_get_inflight_fd == NULL ||
> -        hdev->vhost_ops->vhost_set_inflight_fd == NULL) {
> -        return 0;
> -    }
> -
> -    r = vhost_dev_set_features(hdev, hdev->log_enabled);
> -    if (r < 0) {
> -        VHOST_OPS_DEBUG("vhost_dev_prepare_inflight failed");
> -        return r;
> -    }
> -
> -    return 0;
> -}
> -
>  int vhost_dev_set_inflight(struct vhost_dev *dev,
>                             struct vhost_inflight *inflight)  {
> --
> 2.28.0