Re: [PATCH v9 14/17] KVM: arm64: Enable mapping guest_memfd in arm64

[Fuad Tabba <tabba@google.com>]

Hi David,

On Wed, 21 May 2025 at 13:44, David Hildenbrand <david@redhat.com> wrote:
>
> On 21.05.25 12:29, Fuad Tabba wrote:
> > On Wed, 21 May 2025 at 11:26, David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 21.05.25 12:12, Fuad Tabba wrote:
> >>> Hi David,
> >>>
> >>> On Wed, 21 May 2025 at 09:05, David Hildenbrand <david@redhat.com> wrote:
> >>>>
> >>>> On 13.05.25 18:34, Fuad Tabba wrote:
> >>>>> Enable mapping guest_memfd in arm64. For now, it applies to all
> >>>>> VMs in arm64 that use guest_memfd. In the future, new VM types
> >>>>> can restrict this via kvm_arch_gmem_supports_shared_mem().
> >>>>>
> >>>>> Signed-off-by: Fuad Tabba <tabba@google.com>
> >>>>> ---
> >>>>>     arch/arm64/include/asm/kvm_host.h | 10 ++++++++++
> >>>>>     arch/arm64/kvm/Kconfig            |  1 +
> >>>>>     2 files changed, 11 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> >>>>> index 08ba91e6fb03..2514779f5131 100644
> >>>>> --- a/arch/arm64/include/asm/kvm_host.h
> >>>>> +++ b/arch/arm64/include/asm/kvm_host.h
> >>>>> @@ -1593,4 +1593,14 @@ static inline bool kvm_arch_has_irq_bypass(void)
> >>>>>         return true;
> >>>>>     }
> >>>>>
> >>>>> +static inline bool kvm_arch_supports_gmem(struct kvm *kvm)
> >>>>> +{
> >>>>> +     return IS_ENABLED(CONFIG_KVM_GMEM);
> >>>>> +}
> >>>>> +
> >>>>> +static inline bool kvm_arch_vm_supports_gmem_shared_mem(struct kvm *kvm)
> >>>>> +{
> >>>>> +     return IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM);
> >>>>> +}
> >>>>> +
> >>>>>     #endif /* __ARM64_KVM_HOST_H__ */
> >>>>> diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
> >>>>> index 096e45acadb2..8c1e1964b46a 100644
> >>>>> --- a/arch/arm64/kvm/Kconfig
> >>>>> +++ b/arch/arm64/kvm/Kconfig
> >>>>> @@ -38,6 +38,7 @@ menuconfig KVM
> >>>>>         select HAVE_KVM_VCPU_RUN_PID_CHANGE
> >>>>>         select SCHED_INFO
> >>>>>         select GUEST_PERF_EVENTS if PERF_EVENTS
> >>>>> +     select KVM_GMEM_SHARED_MEM
> >>>>>         help
> >>>>>           Support hosting virtualized guest machines.
> >>>>>
> >>>>
> >>>> Do we have to reject somewhere if we are given a guest_memfd that was
> >>>> *not* created using the SHARED flag? Or will existing checks already
> >>>> reject that?
> >>>
> >>> We don't reject, but I don't think we need to. A user can create a
> >>> guest_memfd that's private in arm64, it would just be useless.
> >>
> >> But the arm64 fault routine would not be able to handle that properly, no?
> >
> > Actually it would. The function user_mem_abort() doesn't care whether
> > it's private or shared. It would fault it into the guest correctly
> > regardless.
>
>
> I think what I meant is that: if it's !shared (private only), shared
> accesses (IOW all access without CoCo) should be taken from the user
> space mapping.
>
> But user_mem_abort() would blindly go to kvm_gmem_get_pfn() because
> "is_gmem = kvm_slot_has_gmem(memslot) = true".

Yes, since it is a gmem-backed slot.

> In other words, arm64 would have to *ignore* guest_memfd that does not
> support shared?
>
> That's why I was wondering whether we should just immediately refuse
> such guest_memfds.

My thinking is that if a user deliberately creates a
guest_memfd-backed slot without designating it as being sharable, then
either they would find out when they try to map that memory to the
host userspace (mapping it would fail), or it could be that they
deliberately want to set up a VM with memslots that not mappable at
all by the host. Perhaps to add some layer of security (although a
very flimsy one, since it's not a confidential guest).

I'm happy to a check to prevent this. The question is, how to do it
exactly (I assume it would be in kvm_gmem_create())? Would it be
arch-specific, i.e., prevent arm64 from creating non-shared
guest_memfd backed memslots? Or do it by VM type? Even if we do it by
VM-type it would need to be arch-specific, since we allow private
guest_memfd slots for the default VM in x86, but we wouldn't for
arm64.

We could add another function, along the lines of
kvm_arch_supports_gmem_only_shared_mem(), but considering that it
actually works, and (arguably) would behave as intended, I'm not sure
if it's worth the complexity.

What do you think?

Cheers,
/fuad

>
> --
> Cheers,
>
> David / dhildenb
>