Re: [PATCH v2 3/9] backends/hostmem-file: Add "rom" property to support VM templating with R/O files

[David Hildenbrand <david@redhat.com>]

On 22.08.23 16:26, ThinerLogoer wrote:
> Hello,
> 
> At 2023-08-22 19:44:51, "David Hildenbrand" <david@redhat.com> wrote:
>> For now, "share=off,readonly=on" would always result in us opening the
>> file R/O and mmap'ing the opened file MAP_PRIVATE R/O -- effectively
>> turning it into ROM.
>>
>> Especially for VM templating, "share=off" is a common use case. However,
>> that use case is impossible with files that lack write permissions,
>> because "share=off,readonly=on" will not give us writable RAM.
>>
>> The sole user of ROM via memory-backend-file are R/O NVDIMMs, but as we
>> have users (Kata Containers) that rely on the existing behavior --
>> malicious VMs should not be able to consume COW memory for R/O NVDIMMs --
>> we cannot change the semantics of "share=off,readonly=on"
>>
>> So let's add a new "rom" property with on/off/auto values. "auto" is
>> the default and what most people will use: for historical reasons, to not
>> change the old semantics, it defaults to the value of the "readonly"
>> property.
>>
>> For VM templating, one can now use:
>>     -object memory-backend-file,share=off,readonly=on,rom=off,...
>>
>> But we'll disallow:
>>     -object memory-backend-file,share=on,readonly=on,rom=off,...
>> because we would otherwise get an error when trying to mmap the R/O file
>> shared and writable. An explicit error message is cleaner.
>>
>> We will also disallow for now:
>>     -object memory-backend-file,share=off,readonly=off,rom=on,...
>>     -object memory-backend-file,share=on,readonly=off,rom=on,...
>> It's not harmful, but also not really required for now.
>>
>> Alternatives that were abandoned:
>> * Make "unarmed=on" for the NVDIMM set the memory region container
>>   readonly. We would still see a change of ROM->RAM and possibly run
>>   into memslot limits with vhost-user. Further, there might be use cases
>>   for "unarmed=on" that should still allow writing to that memory
>>   (temporary files, system RAM, ...).
>> * Add a new "readonly=on/off/auto" parameter for NVDIMMs. Similar issues
>>   as with "unarmed=on".
>> * Make "readonly" consume "on/off/file" instead of being a 'bool' type.
>>   This would slightly changes the behavior of the "readonly" parameter:
>>   values like true/false (as accepted by a 'bool'type) would no longer be
>>   accepted.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>> backends/hostmem-file.c | 59 ++++++++++++++++++++++++++++++++++++++++-
>> qapi/qom.json           |  6 ++++-
>> qemu-options.hx         | 10 ++++++-
>> 3 files changed, 72 insertions(+), 3 deletions(-)
>>
>> diff --git a/backends/hostmem-file.c b/backends/hostmem-file.c
>> index ef2d5533af..361d4a8103 100644
>> --- a/backends/hostmem-file.c
>> +++ b/backends/hostmem-file.c
>> @@ -18,6 +18,8 @@
>> #include "sysemu/hostmem.h"
>> #include "qom/object_interfaces.h"
>> #include "qom/object.h"
>> +#include "qapi/visitor.h"
>> +#include "qapi/qapi-visit-common.h"
>>
>> OBJECT_DECLARE_SIMPLE_TYPE(HostMemoryBackendFile, MEMORY_BACKEND_FILE)
>>
>> @@ -31,6 +33,7 @@ struct HostMemoryBackendFile {
>>      bool discard_data;
>>      bool is_pmem;
>>      bool readonly;
>> +    OnOffAuto rom;
>> };
>>
>> static void
>> @@ -53,9 +56,33 @@ file_backend_memory_alloc(HostMemoryBackend *backend, Error **errp)
>>          return;
>>      }
>>
>> +    switch (fb->rom) {
>> +    case ON_OFF_AUTO_AUTO:
>> +        /* Traditionally, opening the file readonly always resulted in ROM. */
>> +        fb->rom = fb->readonly ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
>> +        break;
>> +    case ON_OFF_AUTO_ON:
>> +        if (!fb->readonly) {
>> +            error_setg(errp, "property 'rom' = 'on' is not supported with"
>> +                       " 'readonly' = 'off'");
>> +            return;
>> +        }
>> +        break;
>> +    case ON_OFF_AUTO_OFF:
>> +        if (fb->readonly && backend->share) {
>> +            error_setg(errp, "property 'rom' = 'off' is incompatible with"
>> +                       " 'readonly' = 'on' and 'share' = 'on'");
>> +            return;
>> +        }
>> +        break;
>> +    default:
>> +        assert(false);
>> +    }
>> +
>>      name = host_memory_backend_get_name(backend);
>>      ram_flags = backend->share ? RAM_SHARED : 0;
>> -    ram_flags |= fb->readonly ? RAM_READONLY | RAM_READONLY_FD : 0;
>> +    ram_flags |= fb->readonly ? RAM_READONLY_FD : 0;
>> +    ram_flags |= fb->rom == ON_OFF_AUTO_ON ? RAM_READONLY : 0;
>>      ram_flags |= backend->reserve ? 0 : RAM_NORESERVE;
>>      ram_flags |= fb->is_pmem ? RAM_PMEM : 0;
>>      ram_flags |= RAM_NAMED_FILE;
>> @@ -201,6 +228,32 @@ static void file_memory_backend_set_readonly(Object *obj, bool value,
>>      fb->readonly = value;
>> }
>>
>> +static void file_memory_backend_get_rom(Object *obj, Visitor *v,
>> +                                        const char *name, void *opaque,
>> +                                        Error **errp)
>> +{
>> +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(obj);
>> +    OnOffAuto rom = fb->rom;
>> +
>> +    visit_type_OnOffAuto(v, name, &rom, errp);
>> +}
>> +
>> +static void file_memory_backend_set_rom(Object *obj, Visitor *v,
>> +                                        const char *name, void *opaque,
>> +                                        Error **errp)
>> +{
>> +    HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>> +    HostMemoryBackendFile *fb = MEMORY_BACKEND_FILE(obj);
>> +
>> +    if (host_memory_backend_mr_inited(backend)) {
>> +        error_setg(errp, "cannot change property '%s' of %s.", name,
>> +                   object_get_typename(obj));
>> +        return;
>> +    }
>> +
>> +    visit_type_OnOffAuto(v, name, &fb->rom, errp);
>> +}
>> +
>> static void file_backend_unparent(Object *obj)
>> {
>>      HostMemoryBackend *backend = MEMORY_BACKEND(obj);
>> @@ -243,6 +296,10 @@ file_backend_class_init(ObjectClass *oc, void *data)
>>      object_class_property_add_bool(oc, "readonly",
>>          file_memory_backend_get_readonly,
>>          file_memory_backend_set_readonly);
>> +    object_class_property_add(oc, "rom", "OnOffAuto",
>> +        file_memory_backend_get_rom, file_memory_backend_set_rom, NULL, NULL);
>> +    object_class_property_set_description(oc, "rom",
>> +        "Whether to create Read Only Memory (ROM)");
>> }
>>
>> static void file_backend_instance_finalize(Object *o)
>> diff --git a/qapi/qom.json b/qapi/qom.json
>> index fa3e88c8e6..0cf83c6f39 100644
>> --- a/qapi/qom.json
>> +++ b/qapi/qom.json
>> @@ -668,6 +668,9 @@
>> # @readonly: if true, the backing file is opened read-only; if false,
>> #     it is opened read-write.  (default: false)
>> #
>> +# @rom: whether to create Read Only Memory (ROM).  If set to 'auto', it
>> +#       defaults to the value of @readonly.  (default: auto, since 8.2)
>> +#
>> # Since: 2.1
>> ##
>> { 'struct': 'MemoryBackendFileProperties',
>> @@ -677,7 +680,8 @@
>>              '*discard-data': 'bool',
>>              'mem-path': 'str',
>>              '*pmem': { 'type': 'bool', 'if': 'CONFIG_LIBPMEM' },
>> -            '*readonly': 'bool' } }
>> +            '*readonly': 'bool',
>> +            '*rom': 'OnOffAuto' } }
>>
>> ##
>> # @MemoryBackendMemfdProperties:
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 29b98c3d4c..03ce0b0a30 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -4976,7 +4976,7 @@ SRST
>>      they are specified. Note that the 'id' property must be set. These
>>      objects are placed in the '/objects' path.
>>
>> -    ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,offset=offset,readonly=on|off``
>> +    ``-object memory-backend-file,id=id,size=size,mem-path=dir,share=on|off,discard-data=on|off,merge=on|off,dump=on|off,prealloc=on|off,host-nodes=host-nodes,policy=default|preferred|bind|interleave,align=align,offset=offset,readonly=on|off,rom=on|off|auto``
>>          Creates a memory file backend object, which can be used to back
>>          the guest RAM with huge pages.
>>
>> @@ -5066,6 +5066,14 @@ SRST
>>          The ``readonly`` option specifies whether the backing file is opened
>>          read-only or read-write (default).
>>
>> +        The ``rom`` option specifies whether to create Read Only Memory (ROM)
>> +        that cannot be modified by the VM. If set to ``on``, the VM cannot
>> +        modify the memory. If set to ``off``, the VM can modify the memory.
>> +        If set to ``auto`` (default), the value of the ``readonly`` property
>> +        is used. This option is primarily helpful for VM templating, where we
>> +        want to open a file readonly (``readonly=on``) and allow private
>> +        modifications of the memory by the VM (``share=off``, ``rom=off``).
>> +
>>      ``-object memory-backend-ram,id=id,merge=on|off,dump=on|off,share=on|off,prealloc=on|off,size=size,host-nodes=host-nodes,policy=default|preferred|bind|interleave``
>>          Creates a memory backend object, which can be used to back the
>>          guest RAM. Memory backend objects offer more control than the
> 
> In one word, I'd suggest advertising the existence of "rom" option more invasively, whenever
> private file mapping is used.
> 
> IMHO you should probably be more invasive here to warn unconditionally when
> the memory backend file is going to be opened readwrite but is mapped non shared.

As Daniel said, we should not add new warnings for sane use cases. But we can indeed give a hint when opening the file failed, see below.

> 
> I as a user find the patch series indeed work functionally when I am aware of the "rom"
> option - but what if I am not aware, the outcome is still that qemu tried
> to open the file readwrite even when it is going to be mapped private.

Yes, the implicit "readonly=off" is active in any case, and we cannot change that due to existing users unfortunately.

> 
> When the file is readonly, the error message is:
> ```
> qemu-system-x86_64: can't open backing store pc.ram for guest RAM: Permission denied
> ```
> 
> This should be probably helpful if qemu found that the file exists as a regular file and
> is mapped private, to say something like
> 
> ```
> qemu-system-x86_64: can't open backing store pc.ram for guest RAM: Permission denied
> tip: mapping is private and ram file is probably readonly, maybe you should append "readonly=on,rom=off"
> to "-object memory-backend-file,..." option list. see documentation xxx for details
> ```

What about the following, if we can indeed open the file R/O and we're dealing witha  private mapping:

qemu-system-x86_64: can't open backing store tmp-file for guest RAM: Permission denied
Consider opening the backing store read-only using '-object memory-backend-file,readonly=on,rom=off,...' (see "VM templating" documentation)

?

-- 
Cheers,

David / dhildenb