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

[Xiaoyao Li <xiaoyao.li@intel.com>]

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.

>>
>> I know it is not a possible case for current QEMU, but it's true for
>> future TDX VM. TDX VM can have memory-backend-memfd as the backend for
>> shared memory and kvm gmem memfd for private memory. At any time, for
>> any memory range, only one type is valid, thus the range in opposite
>> memfd can be fallocated.
> 
> Right.
> 
>>
>> Here I get your message as "the ramblock needs to have RAM_SHARED flag
>> to allow the fallocate of the memfd". This is what I don't understand.
> 
> The problem I am seeing is that either
> 
> (a) Someone explicitly sets share=off for some reason for 
> memory-backend-memfd, triggering the warning.
> 
> (b) We somehow lose RAM_SHARED in above configuration, which would be 
> bad and trigger the warning.
> 
> Can you make sure that (a) is not the case?

Above reply answers it. Sorry for it.