diff mbox

[3/3] vhost: Check+skip uninitialized VQs in vhost_verify_ring_mappings

Message ID 1364531592-8368-4-git-send-email-nab@linux-iscsi.org
State New
Headers show

Commit Message

Nicholas A. Bellinger March 29, 2013, 4:33 a.m. UTC 
From: Nicholas Bellinger <nab@linux-iscsi.org>

With the virtio_queue_valid() checks in place to skip uninitialized VQs
within virtio-pci code, go ahead and skip the same uninitialized VQs
during vhost_verify_ring_mappings().

Note this patch does not prevent vhost_virtqueue_start() from executing
by checking virtio_queue_valid(), as other logic during seabios ->
virtio-scsi LLD guest hand-off appears to depend upon this execution.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Asias He <asias@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 hw/vhost.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

Comments

Michael S. Tsirkin March 31, 2013, 7:45 a.m. UTC | #1 
On Fri, Mar 29, 2013 at 04:33:12AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> With the virtio_queue_valid() checks in place to skip uninitialized VQs
> within virtio-pci code, go ahead and skip the same uninitialized VQs
> during vhost_verify_ring_mappings().
> 
> Note this patch does not prevent vhost_virtqueue_start() from executing
> by checking virtio_queue_valid(), as other logic during seabios ->
> virtio-scsi LLD guest hand-off appears to depend upon this execution.

Weird.
cpu_physical_memory_map only succeeds for PA==0 by chance,
we really should not depend on this.
So the right thing really should be to skip vhost_virtqueue_start IMHO,
maybe add an explicit valid flag in vhost_virtqueue
so vhost_verify_ring_mappings can check it.
What exactly does it do that is needed?

> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Asias He <asias@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  hw/vhost.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/vhost.c b/hw/vhost.c
> index 4d6aee3..3a71aee 100644
> --- a/hw/vhost.c
> +++ b/hw/vhost.c
> @@ -314,6 +314,9 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
>          hwaddr l;
>          void *p;
>  
> +        if (!vq->ring_phys || !vq->ring_size) {
> +            continue;
> +        }
>          if (!ranges_overlap(start_addr, size, vq->ring_phys, vq->ring_size)) {
>              continue;
>          }
> -- 
> 1.7.2.5
Nicholas A. Bellinger April 1, 2013, 11:49 p.m. UTC | #2 
On Sun, 2013-03-31 at 10:45 +0300, Michael S. Tsirkin wrote:
> On Fri, Mar 29, 2013 at 04:33:12AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > With the virtio_queue_valid() checks in place to skip uninitialized VQs
> > within virtio-pci code, go ahead and skip the same uninitialized VQs
> > during vhost_verify_ring_mappings().
> > 
> > Note this patch does not prevent vhost_virtqueue_start() from executing
> > by checking virtio_queue_valid(), as other logic during seabios ->
> > virtio-scsi LLD guest hand-off appears to depend upon this execution.
> 
> Weird.
> cpu_physical_memory_map only succeeds for PA==0 by chance,
> we really should not depend on this.
> So the right thing really should be to skip vhost_virtqueue_start IMHO,
> maybe add an explicit valid flag in vhost_virtqueue
> so vhost_verify_ring_mappings can check it.
> What exactly does it do that is needed?
> 

So the issue with virtio_queue_valid() preventing
vhost_virtqueue_start() execution in the original patch was that
vhost_virtqueue_stop() was missing a matching virtio_queue_valid() call,
which ended up triggering a bad ram pointer during subsequent
cpu_physical_memory_unmap() calls to non-existent virtio queue memory..

With the matching virtio_queue_valid() call in place preventing
vhost_virtqueue_stop() when vhost_virtqueue_start() is skipped for an
uninitialized VQ, a explicit valid flag should not be necessary.

--nab


> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Asias He <asias@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> > ---
> >  hw/vhost.c |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/vhost.c b/hw/vhost.c
> > index 4d6aee3..3a71aee 100644
> > --- a/hw/vhost.c
> > +++ b/hw/vhost.c
> > @@ -314,6 +314,9 @@ static int vhost_verify_ring_mappings(struct vhost_dev *dev,
> >          hwaddr l;
> >          void *p;
> >  
> > +        if (!vq->ring_phys || !vq->ring_size) {
> > +            continue;
> > +        }
> >          if (!ranges_overlap(start_addr, size, vq->ring_phys, vq->ring_size)) {
> >              continue;
> >          }
> > -- 
> > 1.7.2.5
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/hw/vhost.c b/hw/vhost.c
index 4d6aee3..3a71aee 100644
--- a/hw/vhost.c
+++ b/hw/vhost.c
@@ -314,6 +314,9 @@  static int vhost_verify_ring_mappings(struct vhost_dev *dev,
         hwaddr l;
         void *p;
 
+        if (!vq->ring_phys || !vq->ring_size) {
+            continue;
+        }
         if (!ranges_overlap(start_addr, size, vq->ring_phys, vq->ring_size)) {
             continue;
         }