Sean Christopherson <[email protected]> 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.
