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

On Mon, Aug 17, 2026, Ackerley Tng wrote:
> Sean Christopherson <[email protected]> writes:
> > On Mon, Aug 17, 2026, Yan Zhao wrote:
> >> > 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.

Ya, that's definitely one of my hesitations to trying to guarantee success in
the kernel.

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

Yes, but the above docs also say "it's userspace's problem".  Which I generally
agree with, but that's not a very good story when it comes to KVM itself taking
transient references, because then the answer becomes "Stop running all vCPUs",
which I don't like.  E.g. in a very pathological scenario, it's theoretically
possible that conversion may never succeed.  That's what gives me pause.

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

True, but at the same time, if there were never any VMAs then I would expect there
to never be transient refcounts, modulo memory failure.  And it'd be easy enough
to document the memory failure angle.

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

Yeah, that was too strong of wording on my part.  The needle I was trying to
thread was "conversion for this specific scenario, in a controlled environment,
is guaranteed to succeed".

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

Yes, it would definitely be a separate mini-series.

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

FWIW, it doesn't have to block initial merge, just the final release.  E.g. even
if we decide that this is a blocking issue, we can still land the in-place
conversion series, so long as it's not exposed to userspace in the final release
of 7.4 (or whatever kernel) without fixing the transient refcount issue.

> If we find a way to strengthen the guarantee, wouldn't that be an iterative
> improvement?

Yes, but we do need to draw a line in the sand.  E.g. if conversion failed 99%
of the time because KVM was taking spurious references, I think we'd all agree
that needs to be fixed before the code is released.

I'm still leaning towards saying this one has to be fixed, because it would give
us a solid baseline from which to start, and a way to enforce it going forward
(Yan's stress test).  I could certinaly be convinced otherwise, though dropping
the transiest reference seems straightforward enough that hopefully it's a moot
point, i.e. we land both in 7.4 and don't actually have to make a decision.