thr3ads.net - Linux Virtualization - [PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs [Mar 2013]

If this information is useful, please help other people find it:
Share via:

Nicholas A. Bellinger

2013-Mar-29 04:33 UTC

[PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs

From: Nicholas Bellinger <nab at linux-iscsi.org>

Hi folks,

This series adds a virtio_queue_valid() for use by virtio-pci code in
order to prevent opreations upon uninitialized VQs, that is currently
expected to occur during seabios setup of virtio-scsi.

This also includes a vhost specific check for uninitialized VQs in
vhost_verify_ring_mappings() to avoid this same case.

Please review.

--nab

Michael S. Tsirkin (1):
  virtio: add API to check that ring is setup

Nicholas Bellinger (2):
  virtio-pci: Add virtio_queue_valid checks ahead of
    virtio_queue_get_num
  vhost: Check+skip uninitialized VQs in vhost_verify_ring_mappings

 hw/vhost.c      |    3 +++
 hw/virtio-pci.c |   27 +++++++++++++++++++++++++++
 hw/virtio.c     |    5 +++++
 hw/virtio.h     |    1 +
 4 files changed, 36 insertions(+), 0 deletions(-)

-- 
1.7.2.5

Nicholas A. Bellinger

2013-Mar-29 04:33 UTC

head link

[PATCH 1/3] virtio: add API to check that ring is setup

From: Michael S. Tsirkin <mst at redhat.com>

virtio scsi makes it legal to only setup a subset of rings.  The only
way to detect the ring is setup seems to be to check whether PA was
written to.  Add API to do this, and teach code to use it instead of
checking hardware queue size.

(nab: use .vring.desc instead of .vring.pa)

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

diff --git a/hw/virtio.c b/hw/virtio.c
index 26fbc79..65ba253 100644
--- a/hw/virtio.c
+++ b/hw/virtio.c
@@ -651,6 +651,11 @@ int virtio_queue_get_num(VirtIODevice *vdev, int n)
     return vdev->vq[n].vring.num;
 }
 
+bool virtio_queue_valid(VirtIODevice *vdev, int n)
+{
+    return vdev->vq[n].vring.num && vdev->vq[n].vring.desc;
+}
+
 int virtio_queue_get_id(VirtQueue *vq)
 {
     VirtIODevice *vdev = vq->vdev;
diff --git a/hw/virtio.h b/hw/virtio.h
index fdbe931..3086798 100644
--- a/hw/virtio.h
+++ b/hw/virtio.h
@@ -227,6 +227,7 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr,
uint32_t data);
 void virtio_queue_set_addr(VirtIODevice *vdev, int n, hwaddr addr);
 hwaddr virtio_queue_get_addr(VirtIODevice *vdev, int n);
 int virtio_queue_get_num(VirtIODevice *vdev, int n);
+bool virtio_queue_valid(VirtIODevice *vdev, int n);
 void virtio_queue_notify(VirtIODevice *vdev, int n);
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n);
 void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector);
-- 
1.7.2.5

Nicholas A. Bellinger

2013-Mar-29 04:33 UTC

head link

[PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num

From: Nicholas Bellinger <nab at linux-iscsi.org>

This patch adds a number of virtio_queue_valid() checks to virtio-pci
ahead of virtio_queue_get_num() usage in order to skip operation upon
the detection of an uninitialized VQ.

There is one exception in virtio_ioport_read():VIRTIO_PCI_QUEUE_NUM,
where virtio_queue_get_num() may still be called without a valid
vdev->vq[n].vring.desc physical address.

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

diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
index 0d67b84..231ca0c 100644
--- a/hw/virtio-pci.c
+++ b/hw/virtio-pci.c
@@ -211,6 +211,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy
*proxy)
     }
 
     for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+        if (!virtio_queue_valid(proxy->vdev, n)) {
+            continue;
+        }
         if (!virtio_queue_get_num(proxy->vdev, n)) {
             continue;
         }
@@ -225,6 +228,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy
*proxy)
 
 assign_error:
     while (--n >= 0) {
+        if (!virtio_queue_valid(proxy->vdev, n)) {
+            continue;
+        }
         if (!virtio_queue_get_num(proxy->vdev, n)) {
             continue;
         }
@@ -246,6 +252,9 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
     }
 
     for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
+        if (!virtio_queue_valid(proxy->vdev, n)) {
+            continue;
+        }
         if (!virtio_queue_get_num(proxy->vdev, n)) {
             continue;
         }
@@ -546,6 +555,9 @@ static int kvm_virtio_pci_vector_use(VirtIOPCIProxy *proxy,
int nvqs)
     MSIMessage msg;
 
     for (queue_no = 0; queue_no < nvqs; queue_no++) {
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
+        }
         if (!virtio_queue_get_num(vdev, queue_no)) {
             break;
         }
@@ -593,6 +605,9 @@ static void kvm_virtio_pci_vector_release(VirtIOPCIProxy
*proxy, int nvqs)
     int queue_no;
 
     for (queue_no = 0; queue_no < nvqs; queue_no++) {
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
+        }
         if (!virtio_queue_get_num(vdev, queue_no)) {
             break;
         }
@@ -665,6 +680,9 @@ static int kvm_virtio_pci_vector_unmask(PCIDevice *dev,
unsigned vector,
     int ret, queue_no;
 
     for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers; queue_no++)
{
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
+        }
         if (!virtio_queue_get_num(vdev, queue_no)) {
             break;
         }
@@ -695,6 +713,9 @@ static void kvm_virtio_pci_vector_mask(PCIDevice *dev,
unsigned vector)
     int queue_no;
 
     for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers; queue_no++)
{
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
+        }
         if (!virtio_queue_get_num(vdev, queue_no)) {
             break;
         }
@@ -717,6 +738,9 @@ static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
     VirtQueue *vq;
 
     for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers; queue_no++)
{
+        if (!virtio_queue_valid(vdev, queue_no)) {
+            continue;
+        }
         if (!virtio_queue_get_num(vdev, queue_no)) {
             break;
         }
@@ -790,6 +814,9 @@ static int virtio_pci_set_guest_notifiers(DeviceState *d,
int nvqs, bool assign)
     }
 
     for (n = 0; n < nvqs; n++) {
+        if (!virtio_queue_valid(vdev, n)) {
+            continue;
+        }
         if (!virtio_queue_get_num(vdev, n)) {
             break;
         }
-- 
1.7.2.5

Nicholas A. Bellinger

2013-Mar-29 04:33 UTC

head link

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

From: Nicholas Bellinger <nab at 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 at redhat.com>
Cc: Asias He <asias at redhat.com>
Cc: Paolo Bonzini <pbonzini at redhat.com>
Signed-off-by: Nicholas Bellinger <nab at 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

Michael S. Tsirkin

2013-Mar-31 07:37 UTC

head link

[PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num

On Fri, Mar 29, 2013 at 04:33:11AM +0000, Nicholas A. Bellinger
wrote:> From: Nicholas Bellinger <nab at linux-iscsi.org>
> 
> This patch adds a number of virtio_queue_valid() checks to virtio-pci
> ahead of virtio_queue_get_num() usage in order to skip operation upon
> the detection of an uninitialized VQ.
> 
> There is one exception in virtio_ioport_read():VIRTIO_PCI_QUEUE_NUM,
> where virtio_queue_get_num() may still be called without a valid
> vdev->vq[n].vring.desc physical address.
> 
> Cc: Michael S. Tsirkin <mst at redhat.com>
> Cc: Asias He <asias at redhat.com>
> Cc: Paolo Bonzini <pbonzini at redhat.com>
> Signed-off-by: Nicholas Bellinger <nab at linux-iscsi.org>
Makes sense. Minor nit: virtio_queue_valid calls virtio_queue_get_num
internally, so we can drop it everywhere we know queue is valid.
> ---
>  hw/virtio-pci.c |   27 +++++++++++++++++++++++++++
>  1 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> index 0d67b84..231ca0c 100644
> --- a/hw/virtio-pci.c
> +++ b/hw/virtio-pci.c
> @@ -211,6 +211,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy
*proxy)
>      }
>  
>      for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
> +        if (!virtio_queue_valid(proxy->vdev, n)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(proxy->vdev, n)) {
>              continue;
>          }
> @@ -225,6 +228,9 @@ static void virtio_pci_start_ioeventfd(VirtIOPCIProxy
*proxy)
>  
>  assign_error:
>      while (--n >= 0) {
> +        if (!virtio_queue_valid(proxy->vdev, n)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(proxy->vdev, n)) {
>              continue;
>          }
> @@ -246,6 +252,9 @@ static void virtio_pci_stop_ioeventfd(VirtIOPCIProxy
*proxy)
>      }
>  
>      for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
> +        if (!virtio_queue_valid(proxy->vdev, n)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(proxy->vdev, n)) {
>              continue;
>          }
> @@ -546,6 +555,9 @@ static int kvm_virtio_pci_vector_use(VirtIOPCIProxy
*proxy, int nvqs)
>      MSIMessage msg;
>  
>      for (queue_no = 0; queue_no < nvqs; queue_no++) {
> +        if (!virtio_queue_valid(vdev, queue_no)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(vdev, queue_no)) {
>              break;
>          }
> @@ -593,6 +605,9 @@ static void
kvm_virtio_pci_vector_release(VirtIOPCIProxy *proxy, int nvqs)
>      int queue_no;
>  
>      for (queue_no = 0; queue_no < nvqs; queue_no++) {
> +        if (!virtio_queue_valid(vdev, queue_no)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(vdev, queue_no)) {
>              break;
>          }
> @@ -665,6 +680,9 @@ static int kvm_virtio_pci_vector_unmask(PCIDevice *dev,
unsigned vector,
>      int ret, queue_no;
>  
>      for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers;
queue_no++) {
> +        if (!virtio_queue_valid(vdev, queue_no)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(vdev, queue_no)) {
>              break;
>          }
> @@ -695,6 +713,9 @@ static void kvm_virtio_pci_vector_mask(PCIDevice *dev,
unsigned vector)
>      int queue_no;
>  
>      for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers;
queue_no++) {
> +        if (!virtio_queue_valid(vdev, queue_no)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(vdev, queue_no)) {
>              break;
>          }
> @@ -717,6 +738,9 @@ static void kvm_virtio_pci_vector_poll(PCIDevice *dev,
>      VirtQueue *vq;
>  
>      for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers;
queue_no++) {
> +        if (!virtio_queue_valid(vdev, queue_no)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(vdev, queue_no)) {
>              break;
>          }
> @@ -790,6 +814,9 @@ static int virtio_pci_set_guest_notifiers(DeviceState
*d, int nvqs, bool assign)
>      }
>  
>      for (n = 0; n < nvqs; n++) {
> +        if (!virtio_queue_valid(vdev, n)) {
> +            continue;
> +        }
>          if (!virtio_queue_get_num(vdev, n)) {
>              break;
>          }
> -- 
> 1.7.2.5

Michael S. Tsirkin

2013-Mar-31 07:45 UTC

head link

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

On Fri, Mar 29, 2013 at 04:33:12AM +0000, Nicholas A. Bellinger
wrote:> From: Nicholas Bellinger <nab at 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 at redhat.com>
> Cc: Asias He <asias at redhat.com>
> Cc: Paolo Bonzini <pbonzini at redhat.com>
> Signed-off-by: Nicholas Bellinger <nab at 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

Michael S. Tsirkin

2013-Mar-31 07:46 UTC

head link

[PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs

On Fri, Mar 29, 2013 at 04:33:09AM +0000, Nicholas A. Bellinger
wrote:> From: Nicholas Bellinger <nab at linux-iscsi.org>
> 
> Hi folks,
> 
> This series adds a virtio_queue_valid() for use by virtio-pci code in
> order to prevent opreations upon uninitialized VQs, that is currently
> expected to occur during seabios setup of virtio-scsi.
> 
> This also includes a vhost specific check for uninitialized VQs in
> vhost_verify_ring_mappings() to avoid this same case.
> 
> Please review.
> 
> --nab
Okay, and does this fix the failures in vhost_verify_ring_mappings
that you've observed?
> Michael S. Tsirkin (1):
>   virtio: add API to check that ring is setup
> 
> Nicholas Bellinger (2):
>   virtio-pci: Add virtio_queue_valid checks ahead of
>     virtio_queue_get_num
>   vhost: Check+skip uninitialized VQs in vhost_verify_ring_mappings
> 
>  hw/vhost.c      |    3 +++
>  hw/virtio-pci.c |   27 +++++++++++++++++++++++++++
>  hw/virtio.c     |    5 +++++
>  hw/virtio.h     |    1 +
>  4 files changed, 36 insertions(+), 0 deletions(-)
> 
> -- 
> 1.7.2.5

Nicholas A. Bellinger

2013-Apr-01 23:16 UTC

head link

[PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num

On Sun, 2013-03-31 at 10:37 +0300, Michael S. Tsirkin
wrote:> On Fri, Mar 29, 2013 at 04:33:11AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab at linux-iscsi.org>
> > 
> > This patch adds a number of virtio_queue_valid() checks to virtio-pci
> > ahead of virtio_queue_get_num() usage in order to skip operation upon
> > the detection of an uninitialized VQ.
> > 
> > There is one exception in virtio_ioport_read():VIRTIO_PCI_QUEUE_NUM,
> > where virtio_queue_get_num() may still be called without a valid
> > vdev->vq[n].vring.desc physical address.
> > 
> > Cc: Michael S. Tsirkin <mst at redhat.com>
> > Cc: Asias He <asias at redhat.com>
> > Cc: Paolo Bonzini <pbonzini at redhat.com>
> > Signed-off-by: Nicholas Bellinger <nab at linux-iscsi.org>
> 
> Makes sense. Minor nit: virtio_queue_valid calls virtio_queue_get_num
> internally, so we can drop it everywhere we know queue is valid.
> 
Yes, of course.  This includes every location in virtio-pci.c below..

Including for patch-v2.
> > ---
> >  hw/virtio-pci.c |   27 +++++++++++++++++++++++++++
> >  1 files changed, 27 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/virtio-pci.c b/hw/virtio-pci.c
> > index 0d67b84..231ca0c 100644
> > --- a/hw/virtio-pci.c
> > +++ b/hw/virtio-pci.c
> > @@ -211,6 +211,9 @@ static void
virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
> >      }
> >  
> >      for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
> > +        if (!virtio_queue_valid(proxy->vdev, n)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(proxy->vdev, n)) {
> >              continue;
> >          }
> > @@ -225,6 +228,9 @@ static void
virtio_pci_start_ioeventfd(VirtIOPCIProxy *proxy)
> >  
> >  assign_error:
> >      while (--n >= 0) {
> > +        if (!virtio_queue_valid(proxy->vdev, n)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(proxy->vdev, n)) {
> >              continue;
> >          }
> > @@ -246,6 +252,9 @@ static void
virtio_pci_stop_ioeventfd(VirtIOPCIProxy *proxy)
> >      }
> >  
> >      for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
> > +        if (!virtio_queue_valid(proxy->vdev, n)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(proxy->vdev, n)) {
> >              continue;
> >          }
> > @@ -546,6 +555,9 @@ static int
kvm_virtio_pci_vector_use(VirtIOPCIProxy *proxy, int nvqs)
> >      MSIMessage msg;
> >  
> >      for (queue_no = 0; queue_no < nvqs; queue_no++) {
> > +        if (!virtio_queue_valid(vdev, queue_no)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(vdev, queue_no)) {
> >              break;
> >          }
> > @@ -593,6 +605,9 @@ static void
kvm_virtio_pci_vector_release(VirtIOPCIProxy *proxy, int nvqs)
> >      int queue_no;
> >  
> >      for (queue_no = 0; queue_no < nvqs; queue_no++) {
> > +        if (!virtio_queue_valid(vdev, queue_no)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(vdev, queue_no)) {
> >              break;
> >          }
> > @@ -665,6 +680,9 @@ static int kvm_virtio_pci_vector_unmask(PCIDevice
*dev, unsigned vector,
> >      int ret, queue_no;
> >  
> >      for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers;
queue_no++) {
> > +        if (!virtio_queue_valid(vdev, queue_no)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(vdev, queue_no)) {
> >              break;
> >          }
> > @@ -695,6 +713,9 @@ static void kvm_virtio_pci_vector_mask(PCIDevice
*dev, unsigned vector)
> >      int queue_no;
> >  
> >      for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers;
queue_no++) {
> > +        if (!virtio_queue_valid(vdev, queue_no)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(vdev, queue_no)) {
> >              break;
> >          }
> > @@ -717,6 +738,9 @@ static void kvm_virtio_pci_vector_poll(PCIDevice
*dev,
> >      VirtQueue *vq;
> >  
> >      for (queue_no = 0; queue_no < proxy->nvqs_with_notifiers;
queue_no++) {
> > +        if (!virtio_queue_valid(vdev, queue_no)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(vdev, queue_no)) {
> >              break;
> >          }
> > @@ -790,6 +814,9 @@ static int
virtio_pci_set_guest_notifiers(DeviceState *d, int nvqs, bool assign)
> >      }
> >  
> >      for (n = 0; n < nvqs; n++) {
> > +        if (!virtio_queue_valid(vdev, n)) {
> > +            continue;
> > +        }
> >          if (!virtio_queue_get_num(vdev, n)) {
> >              break;
> >          }
> > -- 
> > 1.7.2.5
> --
> To unsubscribe from this list: send the line "unsubscribe
target-devel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nicholas A. Bellinger

2013-Apr-01 23:49 UTC

head link

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

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 at 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 at redhat.com>
> > Cc: Asias He <asias at redhat.com>
> > Cc: Paolo Bonzini <pbonzini at redhat.com>
> > Signed-off-by: Nicholas Bellinger <nab at 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 at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reasonably Related Threads

Search for more maybe matching threads

Linux Virtualization - Mar 2013 - [PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs

[PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs

[PATCH 1/3] virtio: add API to check that ring is setup

[PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num

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

[PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num

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

[PATCH 0/3] virtio/vhost: Add checks for uninitialized VQs

[PATCH 2/3] virtio-pci: Add virtio_queue_valid checks ahead of virtio_queue_get_num

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

Reasonably Related Threads