[PATCH 0/3] migration/hostmem: Allow to fail early for postcopy on specific fs type

[PATCH 0/3] migration/hostmem: Allow to fail early for postcopy on specific fs type

[Peter Xu]

Postcopy can fail in a weird way when guest mem is put onto a random file:

https://bugzilla.redhat.com/show_bug.cgi?id=2057267

It's because we only check userfault privilege on dest QEMU but don't check
memory types.  We do so only until the UFFDIO_REGISTER right after we
switch to postcopy live migration from precopy but it could be too late.

This series tries to make it fail early by checking ramblock fs type if
backed by a memory-backend-file.

Now when it happens it'll fail the dest QEMU from the start:

./qemu-system-x86_64 \
        -global migration.x-postcopy-ram=on \
        -incoming defer \
        -object memory-backend-file,id=mem,size=128M,mem-path=$memfile \
        -machine memory-backend=mem

qemu-system-x86_64: Host backend files need to be TMPFS or HUGETLBFS only
qemu-system-x86_64: Postcopy is not supported

It will also fail e.g. QMP migrate-set-capabilities properly.

Please have a look, thanks.

Peter Xu (3):
  hostmem: Detect and cache fs type for file hostmem
  vl.c: Create late backends before migration object
  migration/postcopy: Detect file system on dest host

 backends/hostmem-file.c  | 37 ++++++++++++++++++++++++++++++++++++-
 include/sysemu/hostmem.h |  1 +
 migration/postcopy-ram.c | 28 ++++++++++++++++++++++++----
 softmmu/vl.c             |  9 +++++++--
 4 files changed, 68 insertions(+), 7 deletions(-)

-- 
2.39.1

[PATCH 1/3] hostmem: Detect and cache fs type for file hostmem

[Peter Xu]

Detect the file system for a memory-backend-file object and cache it within
the object if possible when CONFIG_LINUX (using statfs).

Only support the two important types of memory (tmpfs, hugetlbfs) and keep
the rest as "unknown" for now.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 backends/hostmem-file.c  | 37 ++++++++++++++++++++++++++++++++++++-
 include/sysemu/hostmem.h |  1 +
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
index 25141283c4..2484e45a11 100644
--- a/backends/hostmem-file.c
+++ b/backends/hostmem-file.c
@@ -18,13 +18,17 @@
 #include "sysemu/hostmem.h"
 #include "qom/object_interfaces.h"
 #include "qom/object.h"
+#ifdef CONFIG_LINUX
+#include <sys/vfs.h>
+#include <linux/magic.h>
+#endif
 
 OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendFile, MEMORY_BACKEND_FILE)
 
 
 struct HostMemoryBackendFile {
     HostMemoryBackend parent_obj;
-
+    __fsword_t fs_type;
     char *mem_path;
     uint64_t align;
     bool discard_data;
@@ -52,6 +56,15 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
         return;
     }
 
+#ifdef CONFIG_LINUX
+    struct statfs fs;
+    if (!statfs(fb->mem_path, &fs)) {
+        fb->fs_type = fs.f_type;
+    } else {
+        fb->fs_type = 0;
+    }
+#endif
+
     name = host_memory_backend_get_name(backend);
     ram_flags = backend->share ? RAM_SHARED : 0;
     ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
@@ -181,6 +194,28 @@ static void file_backend_unparent(Object *obj)
     }
 }
 
+const char *file_memory_backend_get_fs_type(Object *obj)
+{
+#ifdef CONFIG_LINUX
+    HostMemoryBackendFile *fb = (HostMemoryBackendFile *)
+        object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_FILE);
+
+    if (!fb) {
+        goto out;
+    }
+
+    switch (fb->fs_type) {
+    case TMPFS_MAGIC:
+        return "tmpfs";
+    case HUGETLBFS_MAGIC:
+        return "hugetlbfs";
+    }
+
+out:
+#endif
+    return "unknown";
+}
+
 static void
 file_backend_class_init(ObjectClass *oc, void *data)
 {
diff --git a/include/sysemu/hostmem.h b/include/sysemu/hostmem.h
index 39326f1d4f..0354cffa6b 100644
--- a/include/sysemu/hostmem.h
+++ b/include/sysemu/hostmem.h
@@ -81,5 +81,6 @@ void host_memory_backend_set_mapped(HostMemoryBackend *backend, bool mapped);
 bool host_memory_backend_is_mapped(HostMemoryBackend *backend);
 size_t host_memory_backend_pagesize(HostMemoryBackend *memdev);
 char *host_memory_backend_get_name(HostMemoryBackend *backend);
+const char *file_memory_backend_get_fs_type(Object *obj);
 
 #endif
-- 
2.39.1

[PATCH 2/3] vl.c: Create late backends before migration object

[Peter Xu]

The migration object may want to check against different types of memory
when initialized.  Delay the creation to be after late backends.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 softmmu/vl.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/softmmu/vl.c b/softmmu/vl.c
index ea20b23e4c..ad394b402f 100644
--- a/softmmu/vl.c
+++ b/softmmu/vl.c
@@ -3583,14 +3583,19 @@ void qemu_init(int argc, char **argv)
                      machine_class->name, machine_class->deprecation_reason);
     }
 
+    /*
+     * Create backends before creating migration objects, so that it can
+     * check against compatibilities on the backend memories (e.g. postcopy
+     * over memory-backend-file objects).
+     */
+    qemu_create_late_backends();
+
     /*
      * Note: creates a QOM object, must run only after global and
      * compat properties have been set up.
      */
     migration_object_init();
 
-    qemu_create_late_backends();
-
     /* parse features once if machine provides default cpu_type */
     current_machine->cpu_type = machine_class->default_cpu_type;
     if (cpu_option) {
-- 
2.39.1

[PATCH 3/3] migration/postcopy: Detect file system on dest host

[Peter Xu]

Postcopy requires the memory support userfaultfd to work.  Right now we
check it but it's a bit too late (when switching to postcopy migration).

Do that early right at enabling of postcopy.

Note that this is still only a best effort because ramblocks can be
dynamically created.  We can add check in hostmem creations and fail if
postcopy enabled, but maybe that's too aggressive.

Still, we have chance to fail the most obvious where we know there's an
existing unsupported ramblock.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/postcopy-ram.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
index 93f39f8e06..560530b758 100644
--- a/migration/postcopy-ram.c
+++ b/migration/postcopy-ram.c
@@ -336,11 +336,12 @@ static bool ufd_check_and_apply(int ufd, MigrationIncomingState *mis)
 
 /* Callback from postcopy_ram_supported_by_host block iterator.
  */
-static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
+static int test_ramblock_postcopiable(RAMBlock *rb)
 {
     const char *block_name = qemu_ram_get_idstr(rb);
     ram_addr_t length = qemu_ram_get_used_length(rb);
     size_t pagesize = qemu_ram_pagesize(rb);
+    const char *fs;
 
     if (length % pagesize) {
         error_report("Postcopy requires RAM blocks to be a page size multiple,"
@@ -348,6 +349,15 @@ static int test_ramblock_postcopiable(RAMBlock *rb, void *opaque)
                      "page size of 0x%zx", block_name, length, pagesize);
         return 1;
     }
+
+    if (rb->fd >= 0) {
+        fs = file_memory_backend_get_fs_type(rb->mr->owner);
+        if (strcmp(fs, "tmpfs") && strcmp(fs, "hugetlbfs")) {
+            error_report("Host backend files need to be TMPFS or HUGETLBFS only");
+            return 1;
+        }
+    }
+
     return 0;
 }
 
@@ -366,6 +376,7 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
     struct uffdio_range range_struct;
     uint64_t feature_mask;
     Error *local_err = NULL;
+    RAMBlock *block;
 
     if (qemu_target_page_size() > pagesize) {
         error_report("Target page size bigger than host page size");
@@ -390,9 +401,18 @@ bool postcopy_ram_supported_by_host(MigrationIncomingState *mis)
         goto out;
     }
 
-    /* We don't support postcopy with shared RAM yet */
-    if (foreach_not_ignored_block(test_ramblock_postcopiable, NULL)) {
-        goto out;
+    /*
+     * We don't support postcopy with some type of ramblocks.
+     *
+     * NOTE: we explicitly ignored ramblock_is_ignored() instead we checked
+     * all possible ramblocks.  This is because this function can be called
+     * when creating the migration object, during the phase RAM_MIGRATABLE
+     * is not even properly set for all the ramblocks.
+     */
+    RAMBLOCK_FOREACH(block) {
+        if (test_ramblock_postcopiable(block)) {
+            goto out;
+        }
     }
 
     /*
-- 
2.39.1

Re: [PATCH 1/3] hostmem: Detect and cache fs type for file hostmem

[Daniel P. Berrangé]

On Tue, Apr 18, 2023 at 06:57:47PM -0400, Peter Xu wrote:
> Detect the file system for a memory-backend-file object and cache it within
> the object if possible when CONFIG_LINUX (using statfs).
> 
> Only support the two important types of memory (tmpfs, hugetlbfs) and keep
> the rest as "unknown" for now.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  backends/hostmem-file.c  | 37 ++++++++++++++++++++++++++++++++++++-
>  include/sysemu/hostmem.h |  1 +
>  2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 25141283c4..2484e45a11 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -18,13 +18,17 @@
>  #include "sysemu/hostmem.h"
>  #include "qom/object_interfaces.h"
>  #include "qom/object.h"
> +#ifdef CONFIG_LINUX
> +#include <sys/vfs.h>
> +#include <linux/magic.h>
> +#endif
>  
>  OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendFile, MEMORY_BACKEND_FILE)
>  
>  
>  struct HostMemoryBackendFile {
>      HostMemoryBackend parent_obj;
> -
> +    __fsword_t fs_type;
>      char *mem_path;
>      uint64_t align;
>      bool discard_data;
> @@ -52,6 +56,15 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>          return;
>      }
>  
> +#ifdef CONFIG_LINUX
> +    struct statfs fs;
> +    if (!statfs(fb->mem_path, &fs)) {
> +        fb->fs_type = fs.f_type;
> +    } else {
> +        fb->fs_type = 0;
> +    }
> +#endif
> +
>      name = host_memory_backend_get_name(backend);
>      ram_flags = backend->share ? RAM_SHARED : 0;
>      ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
> @@ -181,6 +194,28 @@ static void file_backend_unparent(Object *obj)
>      }
>  }
>  
> +const char *file_memory_backend_get_fs_type(Object *obj)
> +{
> +#ifdef CONFIG_LINUX
> +    HostMemoryBackendFile *fb = (HostMemoryBackendFile *)
> +        object_dynamic_cast(obj, TYPE_MEMORY_BACKEND_FILE);
> +
> +    if (!fb) {
> +        goto out;
> +    }
> +
> +    switch (fb->fs_type) {
> +    case TMPFS_MAGIC:
> +        return "tmpfs";
> +    case HUGETLBFS_MAGIC:
> +        return "hugetlbfs";
> +    }
> +
> +out:
> +#endif
> +    return "unknown";
> +}

I rather feel this should be returning an enm, not strings, so
the caller just does a plain equality test rather than string
comparisons


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 1/3] hostmem: Detect and cache fs type for file hostmem

[David Hildenbrand]

On 19.04.23 00:57, Peter Xu wrote:
> Detect the file system for a memory-backend-file object and cache it within
> the object if possible when CONFIG_LINUX (using statfs).
> 
> Only support the two important types of memory (tmpfs, hugetlbfs) and keep
> the rest as "unknown" for now.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   backends/hostmem-file.c  | 37 ++++++++++++++++++++++++++++++++++++-
>   include/sysemu/hostmem.h |  1 +
>   2 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index 25141283c4..2484e45a11 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -18,13 +18,17 @@
>   #include "sysemu/hostmem.h"
>   #include "qom/object_interfaces.h"
>   #include "qom/object.h"
> +#ifdef CONFIG_LINUX
> +#include <sys/vfs.h>
> +#include <linux/magic.h>
> +#endif
>   
>   OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendFile, MEMORY_BACKEND_FILE)
>   
>   
>   struct HostMemoryBackendFile {
>       HostMemoryBackend parent_obj;
> -
> +    __fsword_t fs_type;
>       char *mem_path;
>       uint64_t align;
>       bool discard_data;
> @@ -52,6 +56,15 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>           return;
>       }
>   
> +#ifdef CONFIG_LINUX
> +    struct statfs fs;
> +    if (!statfs(fb->mem_path, &fs)) {
> +        fb->fs_type = fs.f_type;
> +    } else {
> +        fb->fs_type = 0;
> +    }
> +#endif
> +


Instead of using statfs, why not implement something like 
qemu_fd_getpagesize(), that also relies on HUGETLBFS_MAGIC already, meaning

	size_t qemu_fd_type(int fd)

which uses fstatfs() instead? As an abstraction, as Daniel suggests, use 
a new enum to return the type -- "0" meaning "unknown".

Then you can even avoid the caching in hostmem code and simply call it 
directly from uffd code.

-- 
Thanks,

David / dhildenb

Re: [PATCH 1/3] hostmem: Detect and cache fs type for file hostmem

[Peter Xu]

On Wed, Apr 19, 2023 at 09:28:21AM +0200, David Hildenbrand wrote:
> On 19.04.23 00:57, Peter Xu wrote:
> > Detect the file system for a memory-backend-file object and cache it within
> > the object if possible when CONFIG_LINUX (using statfs).
> > 
> > Only support the two important types of memory (tmpfs, hugetlbfs) and keep
> > the rest as "unknown" for now.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   backends/hostmem-file.c  | 37 ++++++++++++++++++++++++++++++++++++-
> >   include/sysemu/hostmem.h |  1 +
> >   2 files changed, 37 insertions(+), 1 deletion(-)
> > 
> > diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> > index 25141283c4..2484e45a11 100644
> > --- a/backends/hostmem-file.c
> > +++ b/backends/hostmem-file.c
> > @@ -18,13 +18,17 @@
> >   #include "sysemu/hostmem.h"
> >   #include "qom/object_interfaces.h"
> >   #include "qom/object.h"
> > +#ifdef CONFIG_LINUX
> > +#include <sys/vfs.h>
> > +#include <linux/magic.h>
> > +#endif
> >   OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendFile, MEMORY_BACKEND_FILE)
> >   struct HostMemoryBackendFile {
> >       HostMemoryBackend parent_obj;
> > -
> > +    __fsword_t fs_type;
> >       char *mem_path;
> >       uint64_t align;
> >       bool discard_data;
> > @@ -52,6 +56,15 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
> >           return;
> >       }
> > +#ifdef CONFIG_LINUX
> > +    struct statfs fs;
> > +    if (!statfs(fb->mem_path, &fs)) {
> > +        fb->fs_type = fs.f_type;
> > +    } else {
> > +        fb->fs_type = 0;
> > +    }
> > +#endif
> > +
> 
> 
> Instead of using statfs, why not implement something like
> qemu_fd_getpagesize(), that also relies on HUGETLBFS_MAGIC already, meaning
> 
> 	size_t qemu_fd_type(int fd)
> 
> which uses fstatfs() instead? As an abstraction, as Daniel suggests, use a
> new enum to return the type -- "0" meaning "unknown".
> 
> Then you can even avoid the caching in hostmem code and simply call it
> directly from uffd code.

Yeah, can do.

I think it depends on whether this can also be useful for other users of a
file memory backend object where the fd may not be on hand.  So far this's
indeed the only one that needs it, though, so I can switch.

Thanks,

-- 
Peter Xu