linux-kernel - Re: [RFC PATCH 09/12] KVM: TDX: Fold tdx_mem_page_record_premap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8e5cd292a95cb449e22f63661c54dbb86932159c.camel@intel.com>
Date: Thu, 28 Aug 2025 19:08:29 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "Zhao, Yan Y" <yan.y.zhao@...el.com>
CC: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com"
	<pbonzini@...hat.com>, "Annapurve, Vishal" <vannapurve@...gle.com>,
	"seanjc@...gle.com" <seanjc@...gle.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "michael.roth@....com"
	<michael.roth@....com>, "Weiny, Ira" <ira.weiny@...el.com>
Subject: Re: [RFC PATCH 09/12] KVM: TDX: Fold tdx_mem_page_record_premap_cnt()
 into its sole caller

On Thu, 2025-08-28 at 13:56 +0800, Yan Zhao wrote:
> > Reasons that tdh_mem_page_add() could get BUSY:
> > 1. If two vCPU's tried to tdh_mem_page_add() the same gpa at the same time 
> > they
> > could contend the SEPT entry lock
> > 2. If one vCPU tries to tdh_mem_page_add() while the other zaps (i.e.
> > tdh_mem_range_block()).
> Hmm, two tdh_mem_page_add()s can't contend as they are protected by both
> slot_lock and filemap lock.
> 
> With regard to the contention to tdh_mem_range_block(), please check my
> analysis at the above [1].

The analysis missed the tdh_mem_page_add() failure path

> 
> tdh_mem_page_add() could get BUSY though, when a misbehaved userspace invokes
> KVM_TDX_INIT_MEM_REGION on one vCPU while initializing another vCPU.
> 
> Please check more details at [2].
> 
> [2] https://lore.kernel.org/kvm/20250113021050.18828-1-yan.y.zhao@intel.com/

Ah, the TDR lock. I actually referred to an older version of your locking
analysis that didn't have that one. But this means the premap count could get
out of sync for that reason too.

> 
> 
> > I guess since we don't hold MMU lock while we tdh_mem_page_add(), 2 is a
> > possibility.
> 2 is possible only for paranoid zaps.
> See "case 3. Unexpected zaps" in [1].

Sean's lockdep assert handles half of those cases. Maybe we could also re-
consider a KVM_BUG_ON() in the invalid zap paths again if it comes to it.

> 
> 
> > > What reasonable use case is there for gracefully handling
> > > tdh_mem_page_add()
> > > failure?
> > > 
> > > If there is a need to handle failure, I gotta imagine it's only for the -
> > > EBUSY
> > > case.  And if it's only for -EBUSY, why can't that be handled by retrying
> > > in
> > > tdx_vcpu_init_mem_region()?  If tdx_vcpu_init_mem_region() guarantees that
> > > all
> > > pages mapped into the S-EPT are ADDed, then it can assert that there are
> > > no
> > > pending pages when it completes (even if it "fails"), and similarly
> > > tdx_td_finalize() can KVM_BUG_ON/WARN_ON the number of pending pages being
> > > non-zero.
> > 
> > Maybe we could take mmu write lock for the retry of tdh_mem_page_add(). Or
> > maybe
> > even for a single call of it, until someone wants to parallelize the
> > operation.
> Hmm. I prefer returning -BUSY directly as invoking KVM_TDX_INIT_MEM_REGION 
> before finishing initializing all vCPUs are uncommon.

I was looking guaranteeing its success when Sean posted his suggestion to return
to the original pattern. I'm in favor of that direction. If you agree we can
call this moot.