Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine

[Dima Stepanov]

On Thu, Aug 27, 2020 at 09:46:03PM -0400, Raphael Norwitz wrote:
> On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov <dimastep@yandex-team.ru> wrote:
> >
> > If the vhost-user-blk daemon provides only one virtqueue, but device was
> > added with several queues, then QEMU will send more VHOST-USER command
> > than expected by daemon side. The vhost_virtqueue_start() routine
> > handles such case by checking the return value from the
> > virtio_queue_get_desc_addr() function call. Add the same check to the
> > vhost_dev_set_log() routine.
> >
> > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru>
> > ---
> >  hw/virtio/vhost.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index ffef7ab..a33ffd4 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev 
> > *dev,
> >  static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> >  {
> >      int r, i, idx;
> > +    hwaddr addr;
> > +
> >      r = vhost_dev_set_features(dev, enable_log);
> >      if (r < 0) {
> >          goto err_features;
> >      }
> >      for (i = 0; i < dev->nvqs; ++i) {
> >          idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> > +        addr = virtio_queue_get_desc_addr(dev->vdev, idx);
> > +        if (!addr) {
> > +            /*
> > +             * The queue might not be ready for start. If this
> > +             * is the case there is no reason to continue the process.
> > +             * The similar logic is used by the vhost_virtqueue_start()
> > +             * routine.
> > +             */
> 
> Shouldn’t we goto err_vq here to reset the logging state of any vqs
> which have already been set?
As i understand it, no we shouldn't reset the state of other queues. In
general it is pretty valid case. Let's assume that the backend
vhost-user device supports only two queues. But for instance, the QEMU
command line is using value 4 to define number of virtqueues of such
device. In this case only 2 queues will be initializaed.

I've tried to reflect it in the comment section, that the
vhost_virtqueue_start() routine has been alread made the same:
  "if a queue isn't ready for start, just return 0 without any error"
So i made the same here.

I've found this issue, while testing migration with the default
vhost-user-blk daemon. It fails with assert or sigsegv (don't remember),
because it receives NULL for the queues it doesn't have. In general
the daemon should not fall, because of unexpected VHOST_USER
communication, but also there is no reason for QEMU to send additional
packets.

> 
> > +            break;
> > +        }
> >          r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> >                                       enable_log);
> >          if (r < 0) {
> > --
> > 2.7.4
> >
> >