Re: [PATCH v12 10/18] KVM: x86/mmu: Handle guest page faults for guest_memfd with shared memory

[David Hildenbrand <david@redhat.com>]

On 30.06.25 21:26, Shivank Garg wrote:
> On 6/30/2025 8:38 PM, Fuad Tabba wrote:
>> Hi Ackerley,
>>
>> On Mon, 30 Jun 2025 at 15:44, Ackerley Tng <ackerleytng@google.com> wrote:
>>>
>>> Fuad Tabba <tabba@google.com> writes:
>>>
>>>> Hi Ackerley,
>>>>
>>>> On Fri, 27 Jun 2025 at 16:01, Ackerley Tng <ackerleytng@google.com> wrote:
>>>>>
>>>>> Ackerley Tng <ackerleytng@google.com> writes:
>>>>>
>>>>>> [...]
>>>>>
>>>>>>>> +/*
>>>>>>>> + * Returns true if the given gfn's private/shared status (in the CoCo sense) is
>>>>>>>> + * private.
>>>>>>>> + *
>>>>>>>> + * A return value of false indicates that the gfn is explicitly or implicitly
>>>>>>>> + * shared (i.e., non-CoCo VMs).
>>>>>>>> + */
>>>>>>>>   static inline bool kvm_mem_is_private(struct kvm *kvm, gfn_t gfn)
>>>>>>>>   {
>>>>>>>> -   return IS_ENABLED(CONFIG_KVM_GMEM) &&
>>>>>>>> -          kvm_get_memory_attributes(kvm, gfn) & KVM_MEMORY_ATTRIBUTE_PRIVATE;
>>>>>>>> +   struct kvm_memory_slot *slot;
>>>>>>>> +
>>>>>>>> +   if (!IS_ENABLED(CONFIG_KVM_GMEM))
>>>>>>>> +           return false;
>>>>>>>> +
>>>>>>>> +   slot = gfn_to_memslot(kvm, gfn);
>>>>>>>> +   if (kvm_slot_has_gmem(slot) && kvm_gmem_memslot_supports_shared(slot)) {
>>>>>>>> +           /*
>>>>>>>> +            * Without in-place conversion support, if a guest_memfd memslot
>>>>>>>> +            * supports shared memory, then all the slot's memory is
>>>>>>>> +            * considered not private, i.e., implicitly shared.
>>>>>>>> +            */
>>>>>>>> +           return false;
>>>>>>>
>>>>>>> Why!?!?  Just make sure KVM_MEMORY_ATTRIBUTE_PRIVATE is mutually exclusive with
>>>>>>> mappable guest_memfd.  You need to do that no matter what.
>>>>>>
>>>>>> Thanks, I agree that setting KVM_MEMORY_ATTRIBUTE_PRIVATE should be
>>>>>> disallowed for gfn ranges whose slot is guest_memfd-only. Missed that
>>>>>> out. Where do people think we should check the mutual exclusivity?
>>>>>>
>>>>>> In kvm_supported_mem_attributes() I'm thiking that we should still allow
>>>>>> the use of KVM_MEMORY_ATTRIBUTE_PRIVATE for other non-guest_memfd-only
>>>>>> gfn ranges. Or do people think we should just disallow
>>>>>> KVM_MEMORY_ATTRIBUTE_PRIVATE for the entire VM as long as one memslot is
>>>>>> a guest_memfd-only memslot?
>>>>>>
>>>>>> If we check mutually exclusivity when handling
>>>>>> kvm_vm_set_memory_attributes(), as long as part of the range where
>>>>>> KVM_MEMORY_ATTRIBUTE_PRIVATE is requested to be set intersects a range
>>>>>> whose slot is guest_memfd-only, the ioctl will return EINVAL.
>>>>>>
>>>>>
>>>>> At yesterday's (2025-06-26) guest_memfd upstream call discussion,
>>>>>
>>>>> * Fuad brought up a possible use case where within the *same* VM, we
>>>>>    want to allow both memslots that supports and does not support mmap in
>>>>>    guest_memfd.
>>>>> * Shivank suggested a concrete use case for this: the user wants a
>>>>>    guest_memfd memslot that supports mmap just so userspace addresses can
>>>>>    be used as references for specifying memory policy.
>>>>> * Sean then added on that allowing both types of guest_memfd memslots
>>>>>    (support and not supporting mmap) will allow the user to have a second
>>>>>    layer of protection and ensure that for some memslots, the user
>>>>>    expects never to be able to mmap from the memslot.
>>>>>
>>>>> I agree it will be useful to allow both guest_memfd memslots that
>>>>> support and do not support mmap in a single VM.
>>>>>
>>>>> I think I found an issue with flags, which is that GUEST_MEMFD_FLAG_MMAP
>>>>> should not imply that the guest_memfd will provide memory for all guest
>>>>> faults within the memslot's gfn range (KVM_MEMSLOT_GMEM_ONLY).
>>>>>
>>>>> For the use case Shivank raised, if the user wants a guest_memfd memslot
>>>>> that supports mmap just so userspace addresses can be used as references
>>>>> for specifying memory policy for legacy Coco VMs where shared memory
>>>>> should still come from other sources, GUEST_MEMFD_FLAG_MMAP will be set,
>>>>> but KVM can't fault shared memory from guest_memfd. Hence,
>>>>> GUEST_MEMFD_FLAG_MMAP should not imply KVM_MEMSLOT_GMEM_ONLY.
>>>>>
>>>>> Thinking forward, if we want guest_memfd to provide (no-mmap) protection
>>>>> even for non-CoCo VMs (such that perhaps initial VM image is populated
>>>>> and then VM memory should never be mmap-ed at all), we will want
>>>>> guest_memfd to be the source of memory even if GUEST_MEMFD_FLAG_MMAP is
>>>>> not set.
>>>>>
>>>>> I propose that we should have a single VM-level flag to solve this (in
>>>>> line with Sean's guideline that we should just move towards what we want
>>>>> and not support non-existent use cases): something like
>>>>> KVM_CAP_PREFER_GMEM.
>>>>>
>>>>> If KVM_CAP_PREFER_GMEM_MEMORY is set,
>>>>>
>>>>> * memory for any gfn range in a guest_memfd memslot will be requested
>>>>>    from guest_memfd
>>>>> * any privacy status queries will also be directed to guest_memfd
>>>>> * KVM_MEMORY_ATTRIBUTE_PRIVATE will not be a valid attribute
>>>>>
>>>>> KVM_CAP_PREFER_GMEM_MEMORY will be orthogonal with no validation on
>>>>> GUEST_MEMFD_FLAG_MMAP, which should just purely guard mmap support in
>>>>> guest_memfd.
>>>>>
>>>>> Here's a table that I set up [1]. I believe the proposed
>>>>> KVM_CAP_PREFER_GMEM_MEMORY (column 7) lines up with requirements
>>>>> (columns 1 to 4) correctly.
>>>>>
>>>>> [1] https://lpc.events/event/18/contributions/1764/attachments/1409/3710/guest_memfd%20use%20cases%20vs%20guest_memfd%20flags%20and%20privacy%20tracking.pdf
>>>>
>>>> I'm not sure this naming helps. What does "prefer" imply here? If the
>>>> caller from user space does not prefer, does it mean that they
>>>> mind/oppose?
>>>>
>>>
>>> Sorry, bad naming.
>>>
>>> I used "prefer" because some memslots may not have guest_memfd at
>>> all. To clarify, a "guest_memfd memslot" is a memslot that has some
>>> valid guest_memfd fd and offset. The memslot may also have a valid
>>> userspace_addr configured, either mmap-ed from the same guest_memfd fd
>>> or from some other backing memory (for legacy CoCo VMs), or NULL for
>>> userspace_addr.
>>>
>>> I meant to have the CAP enable KVM_MEMSLOT_GMEM_ONLY of this patch
>>> series for all memslots that have some valid guest_memfd fd and offset,
>>> except if we have a VM-level CAP, KVM_MEMSLOT_GMEM_ONLY should be moved
>>> to the VM level.
>>
>> Regardless of the name, I feel that this functionality at best does
>> not belong in this series, and potentially adds more confusion.
>>
>> Userspace should be specific about what it wants, and they know what
>> kind of memslots there are in the VM: userspace creates them. In that
>> case, userspace can either create a legacy memslot, no need for any of
>> the new flags, or it can create a guest_memfd memslot, and then use
>> any new flags to qualify that. Having a flag/capability that means
>> something for guest_memfd memslots, but effectively keeps the same
>> behavior for legacy ones seems to add more confusion.
>>
>>>> Regarding the use case Shivank mentioned, mmaping for policy, while
>>>> the use case is a valid one, the raison d'être of mmap is to map into
>>>> user space (i.e., fault it in). I would argue that if you opt into
>>>> mmap, you are doing it to be able to access it.
>>>
>>> The above is in conflict with what was discussed on 2025-06-26 IIUC.
>>>
>>> Shivank brought up the case of enabling mmap *only* to be able to set
>>> mempolicy using the VMAs, and Sean (IIUC) later agreed we should allow
>>> userspace to only enable mmap but still disable faults, so that userspace
>>> is given additional protection, such that even if a (compromised)
>>> userspace does a private-to-shared conversion, userspace is still not
>>> allowed to fault in the page.
>>
>> I don't think there's a conflict :)  What I think is this is outside
>> of the scope of this series for a few reasons:
>>
>> - This is prior to the mempolicy work (and is the base for it)
>> - If we need to, we can add a flag later to restrict mmap faulting
>> - Once we get in-place conversion, the mempolicy work could use the
>> ability to disallow mapping for private memory
>>
>> By actually implementing something now, we would be restricting the
>> mempolicy work, rather than helping it, since we would effectively be
>> deciding now how that work should proceed. By keeping this the way it
>> is now, the mempolicy work can explore various alternatives.
>>
>> I think we discussed this in the guest_memfd sync of 2025-06-12, and I
>> think this was roughly our conclusion.
>>
>>> Hence, if we want to support mmaping just for policy and continue to
>>> restrict faulting, then GUEST_MEMFD_FLAG_MMAP should not imply
>>> KVM_MEMSLOT_GMEM_ONLY.
>>>
>>>> To me, that seems like
>>>> something that merits its own flag, rather than mmap. Also, I recall
>>>> that we said that later on, with inplace conversion, that won't be
>>>> even necessary.
>>>
>>> On x86, as of now I believe we're going with an ioctl that does *not*
>>> check what the guest prefers and will go ahead to perform the
>>> private-to-shared conversion, which will go ahead to update
>>> shareability.
>>
>> Here I think you're making my case that we're dragging more complexity
>> from future work/series into this series, since now we're going into
>> the IOCTLs for the conversion series :)
>>
>>>> In other words, this would also be trying to solve a
>>>> problem that we haven't yet encountered and that we have a solution
>>>> for anyway.
>>>>
>>>
>>> So we don't have a solution for the use case where userspace wants to
>>> mmap but never fault for userspace's protection from stray
>>> private-to-shared conversions, unless we decouple GUEST_MEMFD_FLAG_MMAP
>>> and KVM_MEMSLOT_GMEM_ONLY.
>>>
>>>> I think that, unless anyone disagrees, is to go ahead with the names
>>>> we discussed in the last meeting. They seem to be the ones that make
>>>> the most sense for the upcoming use cases.
>>>>
>>>
>>> We could also discuss if we really want to support the use case where
>>> userspace wants to mmap but never fault for userspace's protection from
>>> stray private-to-shared conversions.
>>
>> I would really rather defer that work to when it's needed. It seems
>> that we should aim to land this series as soon as possible, since it's
>> the one blocking much of the future work. As far as I can tell,
>> nothing here precludes introducing the mechanism of supporting the
>> case where userspace wants to mmap but never fault, once it's needed.
>> This was I believe what we had agreed on in the sync on 2025-06-26.
> 
> I support this approach.

Agreed. Let's get this in with the changes requested by Sean applied.

How to use GUEST_MEMFD_FLAG_MMAP in combination with a CoCo VM with 
legacy mem attributes (-> all memory in guest_memfd private) could be 
added later on top, once really required.

As discussed, CoCo VMs that want to support GUEST_MEMFD_FLAG_MMAP will 
have to disable legacy mem attributes using a new capability in stage-2.

-- 
Cheers,

David / dhildenb