Re: [Qemu-devel] [PATCH v2] hostmem-file: reject invalid pmem file sizes

[Wei Yang]

On Wed, Feb 13, 2019 at 03:14:01PM +0800, Stefan Hajnoczi wrote:
>Guests started with NVDIMMs larger than the underlying host file produce
>confusing errors inside the guest.  This happens because the guest
>accesses pages beyond the end of the file.
>
>Check the pmem file size on startup and print a clear error message if
>the size is invalid.
>
>Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1669053
>Cc: Haozhong Zhang <address@hidden>
>Cc: Zhang Yi <address@hidden>
>Cc: Eduardo Habkost <address@hidden>
>Cc: Igor Mammedov <address@hidden>
>Signed-off-by: Stefan Hajnoczi <address@hidden>
>---
>v2:
> * Propagate qemu_get_pmem_size() errors [Igor]
>
> include/qemu/osdep.h    | 13 ++++++++++
> backends/hostmem-file.c | 22 +++++++++++++++++
> util/oslib-posix.c      | 53 +++++++++++++++++++++++++++++++++++++++++
> util/oslib-win32.c      |  5 ++++
> 4 files changed, 93 insertions(+)
>
>diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>index 840af09cb0..303d315c5d 100644
>--- a/include/qemu/osdep.h
>+++ b/include/qemu/osdep.h
>@@ -570,6 +570,19 @@ void qemu_set_tty_echo(int fd, bool echo);
> void os_mem_prealloc(int fd, char *area, size_t sz, int smp_cpus,
>                      Error **errp);
> 
>+/**
>+ * qemu_get_pmem_size:
>+ * @filename: path to a pmem file
>+ * @errp: pointer to a NULL-initialized error object
>+ *
>+ * Determine the size of a persistent memory file.  Besides supporting files 
>on
>+ * DAX file systems, this function also supports Linux devdax character
>+ * devices.
>+ *
>+ * Returns: the size or 0 on failure
>+
> /**
>  * qemu_get_pid_name:
>  * @pid: pid of a process
>diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>index ba601ce940..d62689179b 100644
>--- a/backends/hostmem-file.c
>+++ b/backends/hostmem-file.c
>@@ -46,6 +46,28 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error 
>**errp)
>     gchar *name;
> #endif
> 
>+    /*
>+     * Verify pmem file size since starting a guest with an incorrect size
>+     * leads to confusing failures inside the guest.
>+     */
>+    if (fb->is_pmem && fb->mem_path) {
>+        Error *local_err = NULL;
>+        uint64_t size;
>+
>+        size = qemu_get_pmem_size(fb->mem_path, &local_err);
>+        if (!size) {
>+            error_propagate(errp, local_err);
>+            return;
>+        }
>+
>+        if (backend->size > size) {
>+            error_setg(errp, "size property %" PRIu64 " is larger than "
>+                       "pmem file \"%s\" size %" PRIu64, backend->size,
>+                       fb->mem_path, size);
>+            return;
>+        }
>+    }
>+
>     if (!backend->size) {
>         error_setg(errp, "can't create backend with size 0");
>         return;

Would it be reasonable to move the above check after checking size and
mem_path?

>diff --git a/util/oslib-posix.c b/util/oslib-posix.c
>index 37c5854b9c..10d90d1783 100644
>--- a/util/oslib-posix.c
>+++ b/util/oslib-posix.c
>@@ -500,6 +500,59 @@ void os_mem_prealloc(int fd, char *area, size_t memory, 
>int smp_cpus,
>     }
> }
> 
>+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
>+{
>+    struct stat st;
>+
>+    if (stat(filename, &st) < 0) {
>+        error_setg(errp, "unable to stat pmem file \"%s\"", filename);
>+        return 0;
>+    }
>+
>+#if defined(__linux__)
>+    /* Special handling for devdax character devices */
>+    if (S_ISCHR(st.st_mode)) {
>+        char *subsystem_path = NULL;
>+        char *subsystem = NULL;
>+        char *size_path = NULL;
>+        char *size_str = NULL;
>+        uint64_t ret = 0;
>+
>+        subsystem_path = g_strdup_printf("/sys/dev/char/%d:%d/subsystem",
>+                                         major(st.st_rdev), 
>minor(st.st_rdev));
>+        subsystem = g_file_read_link(subsystem_path, NULL);
>+        if (!subsystem) {
>+            error_setg(errp, "unable to read subsystem for pmem file \"%s\"",
>+                       filename);
>+            goto devdax_err;
>+        }
>+
>+        if (!g_str_has_suffix(subsystem, "/dax")) {
>+            error_setg(errp, "pmem file \"%s\" is not a dax device", 
>filename);
>+            goto devdax_err;
>+        }
>+
>+        size_path = g_strdup_printf("/sys/dev/char/%d:%d/size",
>+                                    major(st.st_rdev), minor(st.st_rdev));
>+        if (!g_file_get_contents(size_path, &size_str, NULL, NULL)) {
>+            error_setg(errp, "unable to read size for pmem file \"%s\"",
>+                       size_path);
>+            goto devdax_err;
>+        }
>+
>+        ret = g_ascii_strtoull(size_str, NULL, 0);
>+
>+devdax_err:
>+        g_free(size_str);
>+        g_free(size_path);
>+        g_free(subsystem);
>+        g_free(subsystem_path);
>+        return ret;
>+    }
>+#endif /* defined(__linux__) */
>+
>+    return st.st_size;
>+}
> 
> char *qemu_get_pid_name(pid_t pid)
> {
>diff --git a/util/oslib-win32.c b/util/oslib-win32.c
>index b4c17f5dfa..bd633afab6 100644
>--- a/util/oslib-win32.c
>+++ b/util/oslib-win32.c
>@@ -560,6 +560,11 @@ void os_mem_prealloc(int fd, char *area, size_t memory, 
>int smp_cpus,
>     }
> }
> 
>+uint64_t qemu_get_pmem_size(const char *filename, Error **errp)
>+{
>+    error_setg(errp, "pmem support not available");
>+    return 0;
>+}
> 
> char *qemu_get_pid_name(pid_t pid)
> {
>-- 
>2.20.1
>

-- 
Wei Yang
Help you, Help me