diff mbox series

[091/104] libvhost-user: Fix some memtable remap cases

Message ID 20191212163904.159893-92-dgilbert@redhat.com
State New
Headers show
Series virtiofs daemon [all] | expand

Commit Message

Dr. David Alan Gilbert Dec. 12, 2019, 4:38 p.m. UTC 
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

If a new setmemtable command comes in once the vhost threads are
running, it will remap the guests address space and the threads
will now be looking in the wrong place.

Fortunately we're running this command under lock, so we can
update the queue mappings so that threads will look in the new-right
place.

Note: This doesn't fix things that the threads might be doing
without a lock (e.g. a readv/writev!)  That's for another time.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 contrib/libvhost-user/libvhost-user.c | 33 ++++++++++++++++++++-------
 contrib/libvhost-user/libvhost-user.h |  3 +++
 2 files changed, 28 insertions(+), 8 deletions(-)

Comments

Marc-André Lureau Jan. 17, 2020, 1:58 p.m. UTC | #1 
Hi

On Thu, Dec 12, 2019 at 10:05 PM Dr. David Alan Gilbert (git)
<dgilbert@redhat.com> wrote:
>
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> If a new setmemtable command comes in once the vhost threads are
> running, it will remap the guests address space and the threads
> will now be looking in the wrong place.
>
> Fortunately we're running this command under lock, so we can
> update the queue mappings so that threads will look in the new-right
> place.
>
> Note: This doesn't fix things that the threads might be doing
> without a lock (e.g. a readv/writev!)  That's for another time.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  contrib/libvhost-user/libvhost-user.c | 33 ++++++++++++++++++++-------
>  contrib/libvhost-user/libvhost-user.h |  3 +++
>  2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> index 63e41062a4..b89bf18501 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -564,6 +564,21 @@ vu_reset_device_exec(VuDev *dev, VhostUserMsg *vmsg)
>      return false;
>  }
>
> +static bool
> +map_ring(VuDev *dev, VuVirtq *vq)
> +{
> +    vq->vring.desc = qva_to_va(dev, vq->vra.desc_user_addr);
> +    vq->vring.used = qva_to_va(dev, vq->vra.used_user_addr);
> +    vq->vring.avail = qva_to_va(dev, vq->vra.avail_user_addr);
> +
> +    DPRINT("Setting virtq addresses:\n");
> +    DPRINT("    vring_desc  at %p\n", vq->vring.desc);
> +    DPRINT("    vring_used  at %p\n", vq->vring.used);
> +    DPRINT("    vring_avail at %p\n", vq->vring.avail);
> +
> +    return !(vq->vring.desc && vq->vring.used && vq->vring.avail);
> +}
> +
>  static bool
>  vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
>  {
> @@ -767,6 +782,14 @@ vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
>          close(vmsg->fds[i]);
>      }
>
> +    for (i = 0; i < dev->max_queues; i++) {
> +        if (dev->vq[i].vring.desc) {

The code usually checks for all (vq->vring.desc && vq->vring.used &&
vq->vring.avail).

Perhaps we should introduce a VQ_RING_IS_SET() macro?

> +            if (map_ring(dev, &dev->vq[i])) {
> +                vu_panic(dev, "remaping queue %d during setmemtable", i);
> +            }
> +        }
> +    }
> +
>      return false;
>  }
>
> @@ -853,18 +876,12 @@ vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
>      DPRINT("    avail_user_addr:  0x%016" PRIx64 "\n", vra->avail_user_addr);
>      DPRINT("    log_guest_addr:   0x%016" PRIx64 "\n", vra->log_guest_addr);
>
> +    vq->vra = *vra;
>      vq->vring.flags = vra->flags;
> -    vq->vring.desc = qva_to_va(dev, vra->desc_user_addr);
> -    vq->vring.used = qva_to_va(dev, vra->used_user_addr);
> -    vq->vring.avail = qva_to_va(dev, vra->avail_user_addr);
>      vq->vring.log_guest_addr = vra->log_guest_addr;
>
> -    DPRINT("Setting virtq addresses:\n");
> -    DPRINT("    vring_desc  at %p\n", vq->vring.desc);
> -    DPRINT("    vring_used  at %p\n", vq->vring.used);
> -    DPRINT("    vring_avail at %p\n", vq->vring.avail);
>
> -    if (!(vq->vring.desc && vq->vring.used && vq->vring.avail)) {
> +    if (map_ring(dev, vq)) {
>          vu_panic(dev, "Invalid vring_addr message");
>          return false;
>      }
> diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h
> index 1844b6f8d4..5cb7708559 100644
> --- a/contrib/libvhost-user/libvhost-user.h
> +++ b/contrib/libvhost-user/libvhost-user.h
> @@ -327,6 +327,9 @@ typedef struct VuVirtq {
>      int err_fd;
>      unsigned int enable;
>      bool started;
> +
> +    /* Guest addresses of our ring */
> +    struct vhost_vring_addr vra;
>  } VuVirtq;
>
>  enum VuWatchCondtion {
> --
> 2.23.0
>
>

Looks reasonable otherwise (assuming all running threads were flushed
somehow by other means)
Dr. David Alan Gilbert Jan. 20, 2020, 3:50 p.m. UTC | #2 
* Marc-André Lureau (marcandre.lureau@gmail.com) wrote:
> Hi
> 
> On Thu, Dec 12, 2019 at 10:05 PM Dr. David Alan Gilbert (git)
> <dgilbert@redhat.com> wrote:
> >
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > If a new setmemtable command comes in once the vhost threads are
> > running, it will remap the guests address space and the threads
> > will now be looking in the wrong place.
> >
> > Fortunately we're running this command under lock, so we can
> > update the queue mappings so that threads will look in the new-right
> > place.
> >
> > Note: This doesn't fix things that the threads might be doing
> > without a lock (e.g. a readv/writev!)  That's for another time.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  contrib/libvhost-user/libvhost-user.c | 33 ++++++++++++++++++++-------
> >  contrib/libvhost-user/libvhost-user.h |  3 +++
> >  2 files changed, 28 insertions(+), 8 deletions(-)
> >
> > diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
> > index 63e41062a4..b89bf18501 100644
> > --- a/contrib/libvhost-user/libvhost-user.c
> > +++ b/contrib/libvhost-user/libvhost-user.c
> > @@ -564,6 +564,21 @@ vu_reset_device_exec(VuDev *dev, VhostUserMsg *vmsg)
> >      return false;
> >  }
> >
> > +static bool
> > +map_ring(VuDev *dev, VuVirtq *vq)
> > +{
> > +    vq->vring.desc = qva_to_va(dev, vq->vra.desc_user_addr);
> > +    vq->vring.used = qva_to_va(dev, vq->vra.used_user_addr);
> > +    vq->vring.avail = qva_to_va(dev, vq->vra.avail_user_addr);
> > +
> > +    DPRINT("Setting virtq addresses:\n");
> > +    DPRINT("    vring_desc  at %p\n", vq->vring.desc);
> > +    DPRINT("    vring_used  at %p\n", vq->vring.used);
> > +    DPRINT("    vring_avail at %p\n", vq->vring.avail);
> > +
> > +    return !(vq->vring.desc && vq->vring.used && vq->vring.avail);
> > +}
> > +
> >  static bool
> >  vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
> >  {
> > @@ -767,6 +782,14 @@ vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
> >          close(vmsg->fds[i]);
> >      }
> >
> > +    for (i = 0; i < dev->max_queues; i++) {
> > +        if (dev->vq[i].vring.desc) {
> 
> The code usually checks for all (vq->vring.desc && vq->vring.used &&
> vq->vring.avail).
> 
> Perhaps we should introduce a VQ_RING_IS_SET() macro?

I'd like to understand why? and what to do in the case only one was set?
In this case I'm intending to make sure that there's no old mapping left
in any of the 3.

> > +            if (map_ring(dev, &dev->vq[i])) {
> > +                vu_panic(dev, "remaping queue %d during setmemtable", i);
> > +            }
> > +        }
> > +    }
> > +
> >      return false;
> >  }
> >
> > @@ -853,18 +876,12 @@ vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
> >      DPRINT("    avail_user_addr:  0x%016" PRIx64 "\n", vra->avail_user_addr);
> >      DPRINT("    log_guest_addr:   0x%016" PRIx64 "\n", vra->log_guest_addr);
> >
> > +    vq->vra = *vra;
> >      vq->vring.flags = vra->flags;
> > -    vq->vring.desc = qva_to_va(dev, vra->desc_user_addr);
> > -    vq->vring.used = qva_to_va(dev, vra->used_user_addr);
> > -    vq->vring.avail = qva_to_va(dev, vra->avail_user_addr);
> >      vq->vring.log_guest_addr = vra->log_guest_addr;
> >
> > -    DPRINT("Setting virtq addresses:\n");
> > -    DPRINT("    vring_desc  at %p\n", vq->vring.desc);
> > -    DPRINT("    vring_used  at %p\n", vq->vring.used);
> > -    DPRINT("    vring_avail at %p\n", vq->vring.avail);
> >
> > -    if (!(vq->vring.desc && vq->vring.used && vq->vring.avail)) {
> > +    if (map_ring(dev, vq)) {
> >          vu_panic(dev, "Invalid vring_addr message");
> >          return false;
> >      }
> > diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h
> > index 1844b6f8d4..5cb7708559 100644
> > --- a/contrib/libvhost-user/libvhost-user.h
> > +++ b/contrib/libvhost-user/libvhost-user.h
> > @@ -327,6 +327,9 @@ typedef struct VuVirtq {
> >      int err_fd;
> >      unsigned int enable;
> >      bool started;
> > +
> > +    /* Guest addresses of our ring */
> > +    struct vhost_vring_addr vra;
> >  } VuVirtq;
> >
> >  enum VuWatchCondtion {
> > --
> > 2.23.0
> >
> >
> 
> Looks reasonable otherwise (assuming all running threads were flushed
> somehow by other means)

Yeh, well, that's a separate question - which I think there's room for
more caution over; but that is why there's a 'some' in the subject line.

Dave

> -- 
> Marc-André Lureau
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

diff --git a/contrib/libvhost-user/libvhost-user.c b/contrib/libvhost-user/libvhost-user.c
index 63e41062a4..b89bf18501 100644
--- a/contrib/libvhost-user/libvhost-user.c
+++ b/contrib/libvhost-user/libvhost-user.c
@@ -564,6 +564,21 @@  vu_reset_device_exec(VuDev *dev, VhostUserMsg *vmsg)
     return false;
 }
 
+static bool
+map_ring(VuDev *dev, VuVirtq *vq)
+{
+    vq->vring.desc = qva_to_va(dev, vq->vra.desc_user_addr);
+    vq->vring.used = qva_to_va(dev, vq->vra.used_user_addr);
+    vq->vring.avail = qva_to_va(dev, vq->vra.avail_user_addr);
+
+    DPRINT("Setting virtq addresses:\n");
+    DPRINT("    vring_desc  at %p\n", vq->vring.desc);
+    DPRINT("    vring_used  at %p\n", vq->vring.used);
+    DPRINT("    vring_avail at %p\n", vq->vring.avail);
+
+    return !(vq->vring.desc && vq->vring.used && vq->vring.avail);
+}
+
 static bool
 vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
 {
@@ -767,6 +782,14 @@  vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
         close(vmsg->fds[i]);
     }
 
+    for (i = 0; i < dev->max_queues; i++) {
+        if (dev->vq[i].vring.desc) {
+            if (map_ring(dev, &dev->vq[i])) {
+                vu_panic(dev, "remaping queue %d during setmemtable", i);
+            }
+        }
+    }
+
     return false;
 }
 
@@ -853,18 +876,12 @@  vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
     DPRINT("    avail_user_addr:  0x%016" PRIx64 "\n", vra->avail_user_addr);
     DPRINT("    log_guest_addr:   0x%016" PRIx64 "\n", vra->log_guest_addr);
 
+    vq->vra = *vra;
     vq->vring.flags = vra->flags;
-    vq->vring.desc = qva_to_va(dev, vra->desc_user_addr);
-    vq->vring.used = qva_to_va(dev, vra->used_user_addr);
-    vq->vring.avail = qva_to_va(dev, vra->avail_user_addr);
     vq->vring.log_guest_addr = vra->log_guest_addr;
 
-    DPRINT("Setting virtq addresses:\n");
-    DPRINT("    vring_desc  at %p\n", vq->vring.desc);
-    DPRINT("    vring_used  at %p\n", vq->vring.used);
-    DPRINT("    vring_avail at %p\n", vq->vring.avail);
 
-    if (!(vq->vring.desc && vq->vring.used && vq->vring.avail)) {
+    if (map_ring(dev, vq)) {
         vu_panic(dev, "Invalid vring_addr message");
         return false;
     }
diff --git a/contrib/libvhost-user/libvhost-user.h b/contrib/libvhost-user/libvhost-user.h
index 1844b6f8d4..5cb7708559 100644
--- a/contrib/libvhost-user/libvhost-user.h
+++ b/contrib/libvhost-user/libvhost-user.h
@@ -327,6 +327,9 @@  typedef struct VuVirtq {
     int err_fd;
     unsigned int enable;
     bool started;
+
+    /* Guest addresses of our ring */
+    struct vhost_vring_addr vra;
 } VuVirtq;
 
 enum VuWatchCondtion {