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

[Pankaj Gupta]

Hi Stefan,

> 
> 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>
> ---
>  include/qemu/osdep.h    | 13 ++++++++++
>  backends/hostmem-file.c | 16 +++++++++++++
>  util/oslib-posix.c      | 53 +++++++++++++++++++++++++++++++++++++++++
>  util/oslib-win32.c      |  5 ++++
>  4 files changed, 87 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
> + */
> +uint64_t qemu_get_pmem_size(const char *filename, Error **errp);
> +
>  /**
>   * qemu_get_pid_name:
>   * @pid: pid of a process
> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
> index ba601ce940..325ab4aad9 100644
> --- a/backends/hostmem-file.c
> +++ b/backends/hostmem-file.c
> @@ -46,6 +46,22 @@ 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) {
> +        uint64_t size;
> +
> +        size = qemu_get_pmem_size(fb->mem_path, NULL);
> +        if (size && 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;
> 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

This patch looks good to me.

Reviewed-by: Pankaj Gupta <address@hidden>

Thanks,
Pankaj