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

[David Hildenbrand <david@redhat.com>]

On 21.05.25 15:15, Fuad Tabba wrote:
> 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. 

Hm. But that would meant that we interpret "private" memory as a concept 
that is not understood by the VM. Because the VM does not know what 
"private" memory is ...

> Perhaps to add some layer of security (although a
> very flimsy one, since it's not a confidential guest).

Exactly my point. If you don't want to mmap it then ... don't mmap it :)

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

My thinking was to either block this at slot creation time or at 
guest_memfd creation time. And we should probably block that for other 
VM types as well that do not support private memory?

I mean, creating guest_memfd for private memory when there is no concept 
of private memory for the VM is ... weird, no? :)

-- 
Cheers,

David / dhildenb