Re: [RFC PATCH v2 05/18] KVM: TDX: Drop superfluous page pinning in S-EPT management

[Edgecombe, Rick P]

On Thu, 2025-08-28 at 17:06 -0700, Sean Christopherson wrote:
> From: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> 
> Don't explicitly pin pages when mapping pages into the S-EPT, guest_memfd
> doesn't support page migration in any capacity, i.e. there are no migrate
> callbacks because guest_memfd pages *can't* be migrated.  See the WARN in
> kvm_gmem_migrate_folio().
> 
> Eliminating TDX's explicit pinning will also enable guest_memfd to support
> in-place conversion between shared and private memory[1][2].  Because KVM
> cannot distinguish between speculative/transient refcounts and the
> intentional refcount for TDX on private pages[3], failing to release
> private page refcount in TDX could cause guest_memfd to indefinitely wait
> on decreasing the refcount for the splitting.
> 
> Under normal conditions, not holding an extra page refcount in TDX is safe
> because guest_memfd ensures pages are retained until its invalidation
> notification to KVM MMU is completed. However, if there're bugs in KVM/TDX
> module, not holding an extra refcount when a page is mapped in S-EPT could
> result in a page being released from guest_memfd while still mapped in the
> S-EPT.  But, doing work to make a fatal error slightly less fatal is a net
> negative when that extra work adds complexity and confusion.
> 
> Several approaches were considered to address the refcount issue, including
>   - Attempting to modify the KVM unmap operation to return a failure,
>     which was deemed too complex and potentially incorrect[4].
>  - Increasing the folio reference count only upon S-EPT zapping failure[5].
>  - Use page flags or page_ext to indicate a page is still used by TDX[6],
>    which does not work for HVO (HugeTLB Vmemmap Optimization).
>   - Setting HWPOISON bit or leveraging folio_set_hugetlb_hwpoison()[7].
> 
> Due to the complexity or inappropriateness of these approaches, and the
> fact that S-EPT zapping failure is currently only possible when there are
> bugs in the KVM or TDX module, which is very rare in a production kernel,
> a straightforward approach of simply not holding the page reference count
> in TDX was chosen[8].
> 
> When S-EPT zapping errors occur, KVM_BUG_ON() is invoked to kick off all
> vCPUs and mark the VM as dead. Although there is a potential window that a
> private page mapped in the S-EPT could be reallocated and used outside the
> VM, the loud warning from KVM_BUG_ON() should provide sufficient debug
> information.
> 

Yea, in the case of a bug, there could be a use-after-free. This logic applies
to all code that has allocations including the entire KVM MMU. But in this case,
we can actually catch the use-after-free scenario under scrutiny and not have it
happen silently, which does not apply to all code. But the special case here is
that the use-after-free depends on TDX module logic which is not part of the
kernel.

Yan, can you clarify what you mean by "there could be a small window"? I'm
thinking this is a hypothetical window around vm_dead races? Or more concrete? I
*don't* want to re-open the debate on whether to go with this approach, but I
think this is a good teaching edge case to settle on how we want to treat
similar issues. So I just want to make sure we have the justification right.

>  To be robust against bugs, the user can enable panic_on_warn
> as normal.
> 
> Link: https://lore.kernel.org/all/cover.1747264138.git.ackerleytng@xxxxxxxxxx [1]
> Link: https://youtu.be/UnBKahkAon4 [2]
> Link: https://lore.kernel.org/all/CAGtprH_ypohFy9TOJ8Emm_roT4XbQUtLKZNFcM6Fr+fhTFkE0Q@xxxxxxxxxxxxxx [3]
> Link: https://lore.kernel.org/all/aEEEJbTzlncbRaRA@xxxxxxxxxxxxxxxxxxxxxxxxx [4]
> Link: https://lore.kernel.org/all/aE%2Fq9VKkmaCcuwpU@xxxxxxxxxxxxxxxxxxxxxxxxx [5]
> Link: https://lore.kernel.org/all/aFkeBtuNBN1RrDAJ@xxxxxxxxxxxxxxxxxxxxxxxxx [6]
> Link: https://lore.kernel.org/all/diqzy0tikran.fsf@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx [7]
> Link: https://lore.kernel.org/all/53ea5239f8ef9d8df9af593647243c10435fd219.camel@xxxxxxxxx [8]
> Suggested-by: Vishal Annapurve <vannapurve@xxxxxxxxxx>
> Suggested-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>
> Suggested-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>
> Signed-off-by: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> Reviewed-by: Ira Weiny <ira.weiny@xxxxxxxxx>
> Reviewed-by: Kai Huang <kai.huang@xxxxxxxxx>
> [sean: extract out of hugepage series, massage changelog accordingly]
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---

Discussion aside, Reviewed-by: Rick Edgecombe <rick.p.edgecombe@xxxxxxxxx>