Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[David Hildenbrand <david@redhat.com>]

On 17.08.23 17:13, Stefan Hajnoczi wrote:
> On Thu, 17 Aug 2023 at 05:08, David Hildenbrand <david@redhat.com> wrote:
>>
>> @Stefan, see below on a R/O NVDIMM question.
>>
>> We're discussing how to get MAPR_PRIVATE R/W mapping of a
>> memory-backend-file running when using R/O files.
>>
>>>
>>> This seems a good idea. I am good with the solution you proposed
>>> here as well.
>>
>> I was just going to get started working on that, when I realized
>> something important:
>>
>>
>> "@readonly: if true, the backing file is opened read-only; if false,
>>    it is opened read-write.  (default: false)"
>>
>> "@share: if false, the memory is private to QEMU; if true, it is
>>    shared (default: false)"
>>
>> So readonly is *all* about the file access mode already ... the mmap()
>> parameters are just a side-effect of that. Adding a new
>> "file-access-mode" or similar would be wrong.
>>
>>
>> Here are the combinations we have right now:
>>
>> -object memory-backend-file,share=on,readonly=on
>>
>>    -> Existing behavior: Open readonly, mmap readonly shared
>>    -> Makes sense, mmap'ing readwrite would fail
>>
>> -object memory-backend-file,share=on,readonly=off
>>
>>    -> Existing behavior: Open readwrite, mmap readwrite shared
>>    -> Mostly makes sense: why open a shared file R/W and not mmap it
>>       R/W?
>>
>> -object memory-backend-file,share=off,readonly=off
>>    -> Existing behavior: Open readwrite, mmap readwrite private
>>    -> Mostly makes sense: why open a file R/W and not map it R/W (even if
>>       private)?
>>
>> -object memory-backend-file,share=off,readonly=on
>>    -> Existing behavior: Open readonly, mmap readonly private
>>    -> That's the problematic one
>>
>>
>> So for your use case (VM templating using a readonly file), you
>> would actually want to use:
>>
>> -object memory-backend-file,share=off,readonly=on
>>
>> BUT, have the mmap be writable (instead of currently readonly).
>>
>> Assuming we would change the current behavior, what if someone would
>> specify:
>>
>> -object memory-backend-file,readonly=on
>>
>> (because the default is share=off ...) and using it for a R/O NVDIMM,
>> where we expect any write access to fail.
>>
>>
>> But let's look at the commit that added the "readonly" parameter:
>>
>> commit 86635aa4e9d627d5142b81c57a33dd1f36627d07
>> Author: Stefan Hajnoczi <stefanha@redhat.com>
>> Date:   Mon Jan 4 17:13:19 2021 +0000
>>
>>       hostmem-file: add readonly=on|off option
>>
>>       Let -object memory-backend-file work on read-only files when the
>>       readonly=on option is given. This can be used to share the contents of a
>>       file between multiple guests while preventing them from consuming
>>       Copy-on-Write memory if guests dirty the pages, for example.
>>
>> That was part of
>>
>> https://lore.kernel.org/all/20210104171320.575838-3-stefanha@redhat.com/T/#m712f995e6dcfdde433958bae5095b145dd0ee640
>>
>>   From what I understand, for NVDIMMs we always use
>> "-object memory-backend-file,share=on", even when we want a
>> readonly NVDIMM.
>>
>>
>> So we have two options:
>>
>> 1) Change the current behavior of -object memory-backend-file,share=off,readonly=on:
>>
>> -> Open the file r/o but mmap it writable
> 
> Commit 86635aa4e9d627d5142b81c57a33dd1f36627d07 mentions that we don't
> want guests to be able to dirty pages on the host. The change you're
> proposing would not protect against guests that dirty the memory.

The guest could write memory but not modify the file. Only with 
"share=off,readonly=on" of course, not with "share=on,readonly=on".

> 
> I don't know how important that requirement was (that commit was a
> request from Kata Containers).

Let me take a look if Kata passes "share=on,readonly=on" or 
"share=off,readonly=off".

Thanks!

-- 
Cheers,

David / dhildenb