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

[Dima Stepanov]

On Mon, Aug 31, 2020 at 11:22:14PM -0400, Raphael Norwitz wrote:
> 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().
Good point! Should really use "continue" instead of a "break" to keep
the logic. Will update it in v4. Hope to get some review feedback for
the qtest framework update aswell ).

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