On Wed, Jan 12, 2022 at 4:29 PM Sean Christopherson seanjc@google.com wrote:
On Wed, Jan 12, 2022, David Matlack wrote:
On Wed, Jan 12, 2022 at 3:14 PM Sean Christopherson seanjc@google.com wrote:
On Wed, Jan 12, 2022, David Matlack wrote:
When the TDP MMU is write-protection GFNs for page table protection (as opposed to for dirty logging, or due to the HVA not being writable), it checks if the SPTE is already write-protected and if so skips modifying the SPTE and the TLB flush.
This behavior is incorrect because the SPTE may be write-protected for dirty logging. This implies that the SPTE could be locklessly be made writable on the next write access, and that vCPUs could still be running with writable SPTEs cached in their TLB.
Fix this by unconditionally setting the SPTE and only skipping the TLB flush if the SPTE was already marked !MMU-writable or !Host-writable, which guarantees the SPTE cannot be locklessly be made writable and no vCPUs are running the writable SPTEs cached in their TLBs.
Technically it would be safe to skip setting the SPTE as well since:
(a) If MMU-writable is set then Host-writable must be cleared and the only way to set Host-writable is to fault the SPTE back in entirely (at which point any unsynced shadow pages reachable by the new SPTE will be synced and MMU-writable can be safetly be set again).
and
(b) MMU-writable is never consulted on its own.
And in fact this is what the shadow MMU does when write-protecting guest page tables. However setting the SPTE unconditionally is much easier to reason about and does not require a huge comment explaining why it is safe.
I disagree. I looked at the code+comment before reading the full changelog and typed up a response saying the code should be:
if (!is_writable_pte(iter.old_spte) && !spte_can_locklessly_be_made_writable(spte)) break;Then I went read the changelog and here we are :-)
I find that much more easier to grok, e.g. in plain English: "if the SPTE isn't writable and can't be made writable, there's nothing to do".
Oh interesting. I actually find that confusing because it can easily lead to the MMU-writable bit staying set. Here we are protecting GFNs and we're opting to leave the MMU-writable bit set. It takes a lot of digging to figure out that this is safe because if MMU-writable is set and the SPTE cannot be locklessly be made writable then it implies Host-writable is clear, and Host-writable can't be reset without syncing the all shadow pages reachable by the MMU. Oh and the MMU-writable bit is never consulted on its own (e.g. We never iterate through all SPTEs to find the ones that are !MMU-writable).
Ah, you've missed the other wrinkle: MMU-writable can bet set iff Host-writable is set. In other words, the MMU-writable bit is never left set because it can't be set if spte_can_locklessly_be_made_writable() returns false.
Ohhh I did miss that and yes that explains it. I'll send another version of this patch that skips setting the SPTE unnecessarily.
To reduce confusion, we can and probably should do:
diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h index a4af2a42695c..bc691ff72cab 100644 --- a/arch/x86/kvm/mmu/spte.h +++ b/arch/x86/kvm/mmu/spte.h @@ -316,8 +316,7 @@ static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
static inline bool spte_can_locklessly_be_made_writable(u64 spte) {
return (spte & shadow_host_writable_mask) &&(spte & shadow_mmu_writable_mask);
return (spte & shadow_mmu_writable_mask);}
static inline u64 get_mmio_spte_generation(u64 spte)
Though it'd be nice to have a WARN somewhere to enforce that MMU-Writable isn't set without Host-writable.
We could also rename the helper to is_mmu_writable_spte(), though I'm not sure that's actually better.
Yet another option would be to invert the flag and make it shadow_mmu_pt_protected_mask or something, i.e. make it more explicitly a flag that says "this thing is write-protected for shadowing a page table".