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

Sean Christopherson [off-list ref] writes:

[...snip...]

quoted

quoted

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.

quoted

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.

quoted

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.

I think even modulo memory failure the contract in mm for pages is that
transient refcounts are allowed to be taken.

quoted

quoted

quoted

quoted

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

So I think we can only specifically fix this case Yan reported.

quoted

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.

quoted

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.

quoted

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.

Sean seems confident enough that it's a small change to drop the
transient reference so I went ahead to put together the series [1] so we
can kick off the reviews.

I'll follow up soon with some testing and report on the other series
[1]. Yan, if you could provide a Tested-by on either series it'd be
great :)

Please send your reviews on [1] so we can make it for 7.4! ~7 weeks to
the soft-close of 7.3-rc5!

[1] https://lore.kernel.org/all/20260818-gmem-no-return-page-v1-0-4f8d939efdbc@google.com/ (local)