Sean Christopherson <[email protected]> writes: > > [...snip...] > >> > 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. >
I think even modulo memory failure the contract in mm for pages is that transient refcounts are allowed to be taken. >> >> > 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. >> 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. 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/[email protected]/
