Re: [PATCH v9 07/17] KVM: guest_memfd: Allow host to map guest_memfd() pages

[Fuad Tabba <tabba@google.com>]

Hi Gavin,

On Fri, 16 May 2025 at 13:12, Gavin Shan <gshan@redhat.com> wrote:
>
> Hi Fuad,
>
> On 5/16/25 5:56 PM, Fuad Tabba wrote:
> > On Fri, 16 May 2025 at 08:09, Gavin Shan <gshan@redhat.com> wrote:
> >> On 5/14/25 2:34 AM, Fuad Tabba wrote:
> >>> This patch enables support for shared memory in guest_memfd, including
> >>> mapping that memory at the host userspace. This support is gated by the
> >>> configuration option KVM_GMEM_SHARED_MEM, and toggled by the guest_memfd
> >>> flag GUEST_MEMFD_FLAG_SUPPORT_SHARED, which can be set when creating a
> >>> guest_memfd instance.
> >>>
> >>> Co-developed-by: Ackerley Tng <ackerleytng@google.com>
> >>> Signed-off-by: Ackerley Tng <ackerleytng@google.com>
> >>> Signed-off-by: Fuad Tabba <tabba@google.com>
> >>> ---
> >>>    arch/x86/include/asm/kvm_host.h | 10 ++++
> >>>    include/linux/kvm_host.h        | 13 +++++
> >>>    include/uapi/linux/kvm.h        |  1 +
> >>>    virt/kvm/Kconfig                |  5 ++
> >>>    virt/kvm/guest_memfd.c          | 88 +++++++++++++++++++++++++++++++++
> >>>    5 files changed, 117 insertions(+)
> >>>
> >>
> >> [...]
> >>
> >>> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> >>> index 6db515833f61..8e6d1866b55e 100644
> >>> --- a/virt/kvm/guest_memfd.c
> >>> +++ b/virt/kvm/guest_memfd.c
> >>> @@ -312,7 +312,88 @@ static pgoff_t kvm_gmem_get_index(struct kvm_memory_slot *slot, gfn_t gfn)
> >>>        return gfn - slot->base_gfn + slot->gmem.pgoff;
> >>>    }
> >>>
> >>> +#ifdef CONFIG_KVM_GMEM_SHARED_MEM
> >>> +
> >>> +static bool kvm_gmem_supports_shared(struct inode *inode)
> >>> +{
> >>> +     uint64_t flags = (uint64_t)inode->i_private;
> >>> +
> >>> +     return flags & GUEST_MEMFD_FLAG_SUPPORT_SHARED;
> >>> +}
> >>> +
> >>> +static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
> >>> +{
> >>> +     struct inode *inode = file_inode(vmf->vma->vm_file);
> >>> +     struct folio *folio;
> >>> +     vm_fault_t ret = VM_FAULT_LOCKED;
> >>> +
> >>> +     filemap_invalidate_lock_shared(inode->i_mapping);
> >>> +
> >>> +     folio = kvm_gmem_get_folio(inode, vmf->pgoff);
> >>> +     if (IS_ERR(folio)) {
> >>> +             int err = PTR_ERR(folio);
> >>> +
> >>> +             if (err == -EAGAIN)
> >>> +                     ret = VM_FAULT_RETRY;
> >>> +             else
> >>> +                     ret = vmf_error(err);
> >>> +
> >>> +             goto out_filemap;
> >>> +     }
> >>> +
> >>> +     if (folio_test_hwpoison(folio)) {
> >>> +             ret = VM_FAULT_HWPOISON;
> >>> +             goto out_folio;
> >>> +     }
> >>> +
> >>> +     if (WARN_ON_ONCE(folio_test_large(folio))) {
> >>> +             ret = VM_FAULT_SIGBUS;
> >>> +             goto out_folio;
> >>> +     }
> >>> +
> >>
> >> I don't think there is a large folio involved since the max/min folio order
> >> (stored in struct address_space::flags) should have been set to 0, meaning
> >> only order-0 is possible when the folio (page) is allocated and added to the
> >> page-cache. More details can be referred to AS_FOLIO_ORDER_MASK. It's unnecessary
> >> check but not harmful. Maybe a comment is needed to mention large folio isn't
> >> around yet, but double confirm.
> >
> > The idea is to document the lack of hugepage support in code, but if
> > you think it's necessary, I could add a comment.
> >
>
> Ok, I was actually nit-picky since we're at v9, which is close to integration,
> I guess. If another respin is needed, a comment wouldn't be harmful, but it's
> also perfectly fine without it :)
>
> >
> >>
> >>> +     if (!folio_test_uptodate(folio)) {
> >>> +             clear_highpage(folio_page(folio, 0));
> >>> +             kvm_gmem_mark_prepared(folio);
> >>> +     }
> >>> +
> >>
> >> I must be missing some thing here. This chunk of code is out of sync to kvm_gmem_get_pfn(),
> >> where kvm_gmem_prepare_folio() and kvm_arch_gmem_prepare() are executed, and then
> >> PG_uptodate is set after that. In the latest ARM CCA series, kvm_arch_gmem_prepare()
> >> isn't used, but it would delegate the folio (page) with the prerequisite that
> >> the folio belongs to the private address space.
> >>
> >> I guess that kvm_arch_gmem_prepare() is skipped here because we have the assumption that
> >> the folio belongs to the shared address space? However, this assumption isn't always
> >> true. We probably need to ensure the folio range is really belonging to the shared
> >> address space by poking kvm->mem_attr_array, which can be modified by VMM through
> >> ioctl KVM_SET_MEMORY_ATTRIBUTES.
> >
> > This series only supports shared memory, and the idea is not to use
> > the attributes to check. We ensure that only certain VM types can set
> > the flag (e.g., VM_TYPE_DEFAULT and KVM_X86_SW_PROTECTED_VM).
> >
> > In the patch series that builds on it, with in-place conversion
> > between private and shared, we do add a check that the memory faulted
> > in is in-fact shared.
> >
>
> Ok, thanks for your clarification. I plan to review that series, but not
> getting a chance yet. Right, it's sensible to limit the capability of modifying
> page's attribute (private vs shared) to the particular machine types since
> the whole feature (restricted mmap and in-place conversion) is applicable
> to particular machine types. I can understand KVM_X86_SW_PROTECTED_VM
> (similar to pKVM) needs the feature, but I don't understand why VM_TYPE_DEFAULT
> needs the feature. I guess we may want to use guest-memfd as to tmpfs or
> shmem, meaning all the address space associated with a guest-memfd is shared,
> but without the corresponding private space pointed by struct kvm_userspace_memory_region2
> ::userspace_addr. Instead, the 'userspace_addr' will be mmap(guest-memfd) from
> VMM's perspective if I'm correct.

There are two reasons for why we're adding this feature for
VM_TYPE_DEFAULT. The first is for VMMs like Firecracker to be able to
run guests backed completely by guest_memfd [1]. Combined with
Patrick's series for direct map removal in guest_memfd [2], this would
allow running VMs that offer additional hardening against Spectre-like
transient execution attacks. The other one is that, in the long term,
the hope is for guest_memfd to become the main way for backing guests,
regardless of the type of guest they represent.

If you're interested to find out more, we had a discussion about this
a couple of weeks ago during the bi-weekly guest_memfd upstream call
(May 1) [3].

Cheers,
/fuad

[1] https://github.com/firecracker-microvm/firecracker/tree/feature/secret-hiding
[2] https://lore.kernel.org/all/20250221160728.1584559-1-roypat@amazon.co.uk/
[3] https://docs.google.com/document/d/1M6766BzdY1Lhk7LiR5IqVR8B8mG3cr-cxTxOrAosPOk/edit?tab=t.0#heading=h.jwwteecellpo





> Thanks,
> Gavin
>
> > Thanks,
> > /fuad
> >
> >>> +     vmf->page = folio_file_page(folio, vmf->pgoff);
> >>> +
> >>> +out_folio:
> >>> +     if (ret != VM_FAULT_LOCKED) {
> >>> +             folio_unlock(folio);
> >>> +             folio_put(folio);
> >>> +     }
> >>> +
> >>> +out_filemap:
> >>> +     filemap_invalidate_unlock_shared(inode->i_mapping);
> >>> +
> >>> +     return ret;
> >>> +}
> >>> +
> >>
> >> Thanks,
> >> Gavin
> >>
> >
>