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

[Raphael Norwitz]

On Mon, Aug 31, 2020 at 4:37 AM Dima Stepanov <dimastep@yandex-team.ru> wrote:
>
> 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 see - makes more sense now.

>
> 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.
>

In your example is a reason why, if queue 3 is uninitialized, queue 4
must also be uninitialized? I realize queue 4 being initialized while
queue 3 is not is a strange case, but it may still make the code more
robust to use a "continue" here instead of a "break". This also seems
more like the logic in vhost_virtqueue_start()/vhost_dev_start().

> 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
> > >
> > >