Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine
On Mon, Aug 31, 2020 at 11:22:14PM -0400, Raphael Norwitz wrote:
> On Mon, Aug 31, 2020 at 4:37 AM Dima Stepanov 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
> > > 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
> > > > ---
> > > > 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
> > > >
> > > >
Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine
On Mon, Aug 31, 2020 at 4:37 AM Dima Stepanov 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
> > 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
> > > ---
> > > 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
> > >
> > >
Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine
On Thu, Aug 27, 2020 at 09:46:03PM -0400, Raphael Norwitz wrote:
> On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov 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
> > ---
> > 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
> >
> >
Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine
On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov 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
> ---
> 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?
> +break;
> +}
> r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> enable_log);
> if (r < 0) {
> --
> 2.7.4
>
>
[PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine
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
---
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.
+ */
+break;
+}
r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
enable_log);
if (r < 0) {
--
2.7.4
