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.
