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

[David Hildenbrand <david@redhat.com>]

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?

> 
> 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?

-- 
Cheers,

David / dhildenb