Re: [PATCH v10 11/41] KVM: guest_memfd: Ensure pages are not in use before conversion

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> On Mon, Aug 17, 2026, Yan Zhao wrote:
>> On Thu, Aug 13, 2026 at 04:20:05PM -0700, Sean Christopherson wrote:
>> > On Thu, Aug 13, 2026, Rick P Edgecombe wrote:
>> > > > > nicely so we actually just run an old branch's TDX selftests against newer
>> > > > > kernels. So the branch is a bit of a pile, and not really suitable for sharing.
>> > > > > We plan to clean it and upstream it when the path clears. So it would really
>> > > > > help to get those basic ones upstream. We remain happy to help, so please let us
>> > > > > know.
>> > > >
>> > > > I guess at this point I'm hoping y'all and Sean are okay that this
>> > > > conversions series merges, and we let this stress test failure be
>> > > > handled later. I'll be around to fix things :)
>> > > >
>> > > > I'd say the line of sight to fixing this would be when the KVM MMU only
>> > > > gets PFNs (and no pages at all) from guest_memfd.
>> > >
>> > > Hmm, I think we shouldn't upstream a uABI that we don't have line of sight to
>> > > making robust. So it would be good to settle this thread at least.
>> >
>> > This isn't uABI.  You're talking about hitting a race condition between one task
>> Hmm. Perhaps it is not a uABI issue, since users are allowed to retry. However,
>> it is hard to convince me that it makes sense to require users to retry a
>> private-to-shared conversion before a GFN has ever been mapped, given that a
>> retry is not required when the GFN is currently in use by the guest.
>>
>> > converting a page and another faulting in the same page.  An NMI, SMI, or IRQ at
>> > just the right/wrong time, especially on a preemptible kernel, could lead to the
>> > same test failures, even if KVM drops the refcount "immediately".
>> Could you elaborate on how an NMI, SMI, or IRQ at just the right/wrong time
>> could lead to the same test failures?
>
> Ah, sorry, my bad.  I was speed reading and missed that the key to your suggested
> "*page = NULL" change was that the reference was put _before_
> filemap_invalidate_unlock_shared(), i.e. before dropping
> the invalidate lock and thus before __kvm_gmem_set_attributes() will walk the
> folios to look for outstanding references.  I was thinking that putting the
> reference right away was just shrinking the timing window, but putting the
> reference while still holding the invalidate lock closes the window entirely.
>
> So, I take back what I said about this not being ABI, and about this not blocking
> in-place conversion.  It most definitely affects ABI, and so needs to be addressed
> before merging in-place conversion.

I thought back then when David suggested that conversion can return
-EAGAIN, one of the core ABI benefits is that this leaves the door open
for things to gradually improve. If we can improve stuff within the
kernel, then the the kernel would just return fewer errors. This retains
backward compatibility, since extra userspace code that handles errors
can continue to exist, it just won't be used.

> The only question is if we want to commit to
> guaranteeing that conversion will succeed in this scenario, or if we want to take
> the easy way out and formally document that conversion can fail with EAGAIN at any
> time, even if userspace has never mmap()'d the memory in question.
>

I don't really think there's a need to commit to this, IIUC in
principle, ignoring that on many paths of those guest_memfd may be
excluded, refcounts can be taken even if there are no host userspace
mappings. For one, memory failure handling doesn't care if there are
mappings, the refcount will be taken for a short while and could cause
this conversion failure.

Here's the relevant part of the documentation added for conversions:

  If this ioctl returns -EAGAIN, the offset of the page with unexpected
  refcounts will be returned in `error_offset`. This can occur if there
  are transient refcounts on the pages, taken by other parts of the
  kernel.

  Userspace is expected to figure out how to remove all known refcounts
  on the shared pages, such as refcounts taken by get_user_pages(), and
  try the ioctl again. A possible source of these long term refcounts is
  if the guest_memfd memory was pinned in IOMMU page tables.

> I'm leaning pretty strongly towards guaranteeing conversion will succeed.  We'll
> still need to document the EAGAIN behavior, but IMO there's a massive difference
> between conversion failing if there's a lingering reference acquired via a VMA,
> conversion failing because a vCPU page fault raced with conversion.  E.g. being
> able to assert success in a very curated test, as the stress test presumably does,
> would be extremely valuable for helping detect/prevent edge case bugs.
>
> The argument against guaranteeing success is that we might make our future lives
> harder, e.g. if it turns out there are legitimate, hard-to-solve edge cases.  But
> I'm ok with that risk, as it seems highly unlikely to be problematic in practice,
> and there is real benefit to guaranteeing success.
>

Is there really a need to commit to anything? This is already documented
as "can fail", and it's orthogonal to whether the memory was mapped. The
transient nature of refcounts on pages in general makes it hard to
guarantee, and this stretches outside of KVM. I mean, anything could
take a refcount on a page in future and we can't be auditing the entire
kernel for no refcounts on guest_memfd pages ever.

>> > As for in-place conversion, this is not a blocker.
>> Sorry. I didn't intend to block in-place conversion.
>
> LOL, what we intend and what happens aren't always the same. :-)
>

I don't think we're ready to guarantee conversion success when
guest_memfd pages are not mapped to userspace without dragging this out
way further.

I'm all for KVM not taking any references on guest_memfd, but I think
eliminating KVM itself as a source of transient refcounts can be a
series in itself. KVM not taking any references on guest_memfd memory is
definitely welcome, it'll pave the way to using non-struct-page memory
in guest_memfd.

It'll come, can we not block on this please? If we find a way to
strengthen the guarantee, wouldn't that be an iterative improvement?

>> I encountered this issue during testing, so reported it.
>
> Thanks for doing so!  I'd *much* rather sort these issues out *before* merging
> code, even if it means delaying the merge by a bit.