Re: [PATCH 1/6] KVM: guest_memfd: Add DEFAULT_SHARED flag, reject user page faults if not set

Vishal Annapurve
2025-10-03T20:30:16Z

On Fri, Oct 3, 2025 at 9:13 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Oct 02, 2025, Vishal Annapurve wrote:
> > On Thu, Oct 2, 2025, 5:12 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > > >
> > > > > If the _only_ user-visible asset that is added is a KVM_CREATE_GUEST_MEMFD flag,
> > > > > a CAP is gross overkill. Even if there are other assets that accompany the new
> > > > > flag, there's no reason we couldn't say "this feature exist if XYZ flag is
> > > > > supported".
> > > > >
> > > > > E.g. it's functionally no different than KVM_CAP_VM_TYPES reporting support for
> > > > > KVM_X86_TDX_VM also effectively reporting support for a _huge_ number of things
> > > > > far beyond being able to create a VM of type KVM_X86_TDX_VM.
> > > > >
> > > >
> > > > What's your opinion about having KVM_CAP_GUEST_MEMFD_MMAP part of
> > > > KVM_CAP_GUEST_MEMFD_CAPS i.e. having a KVM cap covering all features
> > > > of guest_memfd?
> > >
> > > I'd much prefer to have both. Describing flags for an ioctl via a bitmask that
> > > doesn't *exactly* match the flags is asking for problems. At best, it will be
> > > confusing. E.g. we'll probably end up with code like this:
> > >
> > > gmem_caps = kvm_check_cap(KVM_CAP_GUEST_MEMFD_CAPS);
> > >
> > > if (gmem_caps & KVM_CAP_GUEST_MEMFD_MMAP)
> > > gmem_flags |= GUEST_MEMFD_FLAG_MMAP;
> > > if (gmem_caps & KVM_CAP_GUEST_MEMFD_INIT_SHARED)
> > > gmem_flags |= KVM_CAP_GUEST_MEMFD_INIT_SHARED;
> > >
> >
> > No, I actually meant the userspace can just rely on the cap to assume
> > right flags to be available (not necessarily the same flags as cap
> > bits).
> >
> > i.e. Userspace will do something like:
> > gmem_caps = kvm_check_cap(KVM_CAP_GUEST_MEMFD_CAPS);
> >
> > if (gmem_caps & KVM_CAP_GUEST_MEMFD_MMAP)
> > gmem_flags |= GUEST_MEMFD_FLAG_MMAP;
> > if (gmem_caps & KVM_CAP_GUEST_MEMFD_HUGETLB)
> > gmem_flags |= GUEST_MEMFD_FLAG_HUGETLB | GUEST_MEMFD_FLAG_HUGETLB_2MB;
>
> Yes, that's exactly what I said. But I goofed when copy+pasted and failed to
> do s/KVM_CAP_GUEST_MEMFD_INIT_SHARED/GUEST_MEMFD_FLAG_INIT_SHARED, which is the
> type of bug that ideally just can't happen.
>
> Side topic, I'm not at all convinced that this is what we want for KVM's uAPI:
>
> if (gmem_caps & KVM_CAP_GUEST_MEMFD_HUGETLB)
> gmem_flags |= GUEST_MEMFD_FLAG_HUGETLB | GUEST_MEMFD_FLAG_HUGETLB_2MB;
>
> See https://lore.kernel.org/all/aN_fJEZXo6wkcHOh@google.com.

Ack, that makes sense to me.

>
> > Userspace has to anyways assume flag values, userspace just needs to
> > know if a particular feature is available.
>
> I don't understand what you mean by "assume flag values".

Ok, I think you covered the explanation of why you would prefer to
have KVM_CAP_GUEST_MEMFD_FLAGS around and I misinterpreted some of it.

One more example with KVM_CAP_GUEST_MEMFD_FLAGS around:

gmem_caps = kvm_check_cap(KVM_CAP_GUEST_MEMFD_CAPS);
valid_flags = kvm_check_cap(KVM_CAP_GUEST_MEMFD_FLAGS);

if (gmem_caps & KVM_CAP_GUEST_MEMFD_CONVERSION) {
// Use single memory backing paths for 4K backing
if (valid_flags & GUEST_MEMFD_FLAG_MMAP)
gmem_flags |= GUEST_MEMFD_FLAG_MMAP;
else
// error out;
}
if (gmem_caps & KVM_CAP_GUEST_MEMFD_HUGETLB_CONVERSION) {
// Use single memory backing paths for hugetlb memory backing
if (valid_flags & GUEST_MEMFD_FLAG_HUGETLB) {
gmem_flags |= GUEST_MEMFD_FLAG_HUGETLB;
kvm_create_guest_memfd.huge_page_size_log2 = ...;
} else
// error out;
}

Userspace will have to rely on a combination of flags and caps to
decide it's control flow instead of just caps. Thinking more about
this, I don't have a strong preference between two scenarios i.e. with
or without KVM_CAP_GUEST_MEMFD_FLAGS.