Re: [PATCH v2 1/4] softmmu/physmem: Warn with ram_block_discard_range() on MAP_PRIVATE file mapping

[David Hildenbrand <david@redhat.com>]

On 19.10.23 11:26, Xiaoyao Li wrote:
> On 10/19/2023 4:26 PM, David Hildenbrand wrote:
>> On 18.10.23 18:27, Xiaoyao Li wrote:
>>> On 10/18/2023 5:26 PM, David Hildenbrand wrote:
>>>> On 18.10.23 11:02, Xiaoyao Li wrote:
>>>>> On 10/18/2023 3:42 PM, David Hildenbrand wrote:
>>>>>> On 18.10.23 05:02, Xiaoyao Li wrote:
>>>>>>> David,
>>>>>>>
>>>>>>> On 7/6/2023 3:56 PM, David Hildenbrand wrote:
>>>>>>>> ram_block_discard_range() cannot possibly do the right thing in
>>>>>>>> MAP_PRIVATE file mappings in the general case.
>>>>>>>>
>>>>>>>> To achieve the documented semantics, we also have to punch a hole
>>>>>>>> into
>>>>>>>> the file, possibly messing with other MAP_PRIVATE/MAP_SHARED
>>>>>>>> mappings
>>>>>>>> of such a file.
>>>>>>>>
>>>>>>>> For example, using VM templating -- see commit b17fbbe55cba
>>>>>>>> ("migration:
>>>>>>>> allow private destination ram with x-ignore-shared") -- in
>>>>>>>> combination with
>>>>>>>> any mechanism that relies on discarding of RAM is problematic. This
>>>>>>>> includes:
>>>>>>>> * Postcopy live migration
>>>>>>>> * virtio-balloon inflation/deflation or free-page-reporting
>>>>>>>> * virtio-mem
>>>>>>>>
>>>>>>>> So at least warn that there is something possibly dangerous is
>>>>>>>> going on
>>>>>>>> when using ram_block_discard_range() in these cases.
>>>>>>>>
>>>>>>>> Acked-by: Peter Xu <peterx@redhat.com>
>>>>>>>> Tested-by: Mario Casquero <mcasquer@redhat.com>
>>>>>>>> Signed-off-by: David Hildenbrand <david@redhat.com>
>>>>>>>> ---
>>>>>>>>       softmmu/physmem.c | 18 ++++++++++++++++++
>>>>>>>>       1 file changed, 18 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
>>>>>>>> index bda475a719..4ee157bda4 100644
>>>>>>>> --- a/softmmu/physmem.c
>>>>>>>> +++ b/softmmu/physmem.c
>>>>>>>> @@ -3456,6 +3456,24 @@ int ram_block_discard_range(RAMBlock *rb,
>>>>>>>> uint64_t start, size_t length)
>>>>>>>>                    * so a userfault will trigger.
>>>>>>>>                    */
>>>>>>>>       #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
>>>>>>>> +            /*
>>>>>>>> +             * We'll discard data from the actual file, even though
>>>>>>>> we only
>>>>>>>> +             * have a MAP_PRIVATE mapping, possibly messing with
>>>>>>>> other
>>>>>>>> +             * MAP_PRIVATE/MAP_SHARED mappings. There is no easy
>>>>>>>> way to
>>>>>>>> +             * change that behavior whithout violating the promised
>>>>>>>> +             * semantics of ram_block_discard_range().
>>>>>>>> +             *
>>>>>>>> +             * Only warn, because it work as long as nobody else
>>>>>>>> uses that
>>>>>>>> +             * file.
>>>>>>>> +             */
>>>>>>>> +            if (!qemu_ram_is_shared(rb)) {
>>>>>>>> +                warn_report_once("ram_block_discard_range:
>>>>>>>> Discarding RAM"
>>>>>>>> +                                 " in private file mappings is
>>>>>>>> possibly"
>>>>>>>> +                                 " dangerous, because it will
>>>>>>>> modify
>>>>>>>> the"
>>>>>>>> +                                 " underlying file and will affect
>>>>>>>> other"
>>>>>>>> +                                 " users of the file");
>>>>>>>> +            }
>>>>>>>> +
>>>>>>>
>>>>>>> TDX has two types of memory backend for each RAM, shared memory and
>>>>>>> private memory. Private memory is serviced by guest memfd and shared
>>>>>>> memory can also be backed with a fd.
>>>>>>>
>>>>>>> At any time, only one type needs to be valid, which means the
>>>>>>> opposite
>>>>>>> can be discarded. We do implement the memory discard when TDX
>>>>>>> converts
>>>>>>> the memory[1]. It will trigger this warning 100% because by
>>>>>>> default the
>>>>>>> guest memfd is not mapped as shared (MAP_SHARED).
>>>>>>
>>>>>> If MAP_PRIVATE is not involved and you are taking the pages
>>>>>> directly out
>>>>>> of the memfd, you should mark that thing as shared.
>>>>>
>>>>> Is it the general rule of Linux? Of just the rule of QEMU memory
>>>>> discard?
>>>>>
>>>>
>>>> MAP_SHARED vs. MAP_PRIVATE is a common UNIX principle, and that's what
>>>> this flag and the check is about.
>>>>
>>>>    From mmap(2)
>>>>
>>>> MAP_SHARED: Share this mapping.  Updates to the mapping are visible to
>>>> other processes mapping the same region, and (in the case of file-backed
>>>> mappings) are carried through to the underlying file.
>>>>
>>>> MAP_PRIVATE: Create a private copy-on-write mapping.  Updates to the
>>>> mapping are not visible to other processes mapping the same file, and
>>>> are not carried through to the underlying file.  It is unspecified
>>>> whether changes made  to the file after the mmap() call are visible in
>>>> the mapped region.
>>>>
>>>> For your purpose (no mmap() at all), we behave like MAP_SHARED -- as if
>>>> nothing special is done. No Copy-on-write, no anonymous memory.
>>>>
>>>>>> Anonymous memory is never involved.
>>>>>
>>>>> Could you please elaborate more on this? What do you want to express
>>>>> here regrading anonymous memory? (Sorry that I'm newbie for mmap stuff)
>>>>
>>>> Anonymous memory is memory that is private to a specific process, and
>>>> (see MAP_PRIVATE) modifications remain private to the process and are
>>>> not reflected to the file.
>>>>
>>>> If you have a MAP_PRIVATE file mapping and write to a virtual memory
>>>> location, you'll get a process-private copy of the underlying pagecache
>>>> page. that's what we call anonymous memory, because it does not belong
>>>> to a specific file. fallocate(punch) would not free up that anonymous
>>>> memory.
>>>
>>> For guest memfd, it does implement kvm_gmem_fallocate as .fallocate()
>>> callback, which calls truncate_inode_pages_range() [*].
>>>
>>> I'm not sure if it frees up the memory. I need to learn it.
>>>
>>> [*]
>>> https://github.com/kvm-x86/linux/blob/911b515af3ec5f53992b9cc162cf7d3893c2fbe2/virt/kvm/guest_memfd.c#L147C73-L147C73
>>>
>>>>>
>>>>>>
>>>>>> "Private memory" is only private from the guest POV, not from a mmap()
>>>>>> point of view.
>>>>>>
>>>>>> Two different concepts of "private".
>>>>>>
>>>>>>>
>>>>>>> Simply remove the warning will fail the purpose of this patch. The
>>>>>>> other
>>>>>>> option is to skip the warning for TDX case, which looks vary
>>>>>>> hacky. Do
>>>>>>> you have any idea?
>>>>>>
>>>>>> For TDX, all memory backends / RAMBlocks should be marked as "shared",
>>>>>> and you should fail if that is not provided by the user.
>>>>>
>>>>> As I asked above, I want to understand the logic clearly. Is mapped as
>>>>> shared is a must to support the memory discard? i.e., if we want to
>>>>> support memory discard after memory type change, then the memory
>>>>> must be
>>>>> mapped with MAP_SHARED?
>>>>
>>>> MAP_PIRVATE means that it's not sufficient to only fallocate(punch) the
>>>> fd to free up all memory for a virtual address, because there might be
>>>> anonymous memory in a private mapping that has to be freed up using
>>>> MADV_DONTNEED.
>>>
>>> I can understand this. But it seems unrelated to my question: Is mapped
>>> as shared is a must to support the memory discard?
>>
>> Sorry, I don't quite get what you are asking that I haven't answered
>> yet. Let's talk about the issue you are seeing below.
>>
>>>
>>> e.g., if use below parameters to specify the RAM for a VM
>>>
>>>      -object memory-backend-memfd,id=mem0,size=2G    \
>>>      -machine memory-backend=mem0
>>>
>>> since not specifying "share" property, the ram_block doesn't have
>>> RAM_SHARED set. If want to discard some range of this memfd, it triggers
>>> the warning. Is this warning expected?
>>
>> That should not be the case. See "memfd_backend_instance_init" where we
>> set share=true. In memfd_backend_memory_alloc(), we set RAM_SHARED.
>>
>> We only default to share=off for memory-backend-file (well, and
>> memory-backend-ram).
>>
>> So are you sure you get this error message in the configuration you are
>> describing here?
> 
> Sorry, I made an mistake. I was using "-object
> memory-backend-ram,id=mem0,size=2G" instead of "memory-backend-memfd".
> 
> yes, when using "memory-backend-memfd" as the backend for TDX shared
> memory, it doesn't trigger the warning because
> memfd_backend_instance_init() set "share" to true.
> 
> When using "memory-backend-ram" as the backend for TDX shared memory,
> the warning is triggered converting memory from private (kvm gmem) to
> shared (backend-ram). In this case, there is a valid fd (kvm gmem fd),
> so it goes to the path of need_fallocate. However,
> qemu_ram_is_shared(rb) returns false because "memory-backend-ram"
> doesn't have "share" default on. Thus, the warning is triggered.
> 
> It seems I need figure out a more proper solution to refactor the
> ram_block_discard_range(), to make it applicable for kvm gmem discard case.

You should probably completely ignore any ramblock flags when 
fallocate(punch) the kvm_gmem_fd. kvm_gmem_fd is a rather special 
"secondary storage that's never mapped", independent of the ordinary 
RAMBlock memory.

-- 
Cheers,

David / dhildenb