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

[Shivank Garg <shivankg@amd.com>]

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.
I think it's more strategic to land the mmap functionality first and then
iterate. We can address those advanced usecases in a separate series.

This follows the same pattern we agreed upon for the NUMA mempolicy support[1]
in 2025-06-12 sync: merge the initial feature rebased on stage-1, and handle
CoCo/SNP requirements in stage 2.

[1] https://lore.kernel.org/linux-mm/20250618112935.7629-1-shivankg@amd.com

Thanks,
Shivank