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

[Fuad Tabba <tabba@google.com>]

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.

Cheers,
/fuad