Re: [PATCH v2 1/6] KVM: x86/mmu: Fix wrong gfn range of tlb flushing in validate_direct_spte()

From: Sean Christopherson <seanjc@google.com>
To: Robert Hoo <robert.hu@linux.intel.com>
Cc: David Matlack <dmatlack@google.com>,
	Hou Wenlong <houwenlong.hwl@antgroup.com>,
	kvm list <kvm@vger.kernel.org>,
	Paolo Bonzini <pbonzini@redhat.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	X86 ML <x86@kernel.org>, "H. Peter Anvin" <hpa@zytor.com>,
	Lan Tianyu <Tianyu.Lan@microsoft.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/6] KVM: x86/mmu: Fix wrong gfn range of tlb flushing in validate_direct_spte()
Date: Tue, 27 Sep 2022 16:44:10 +0000	[thread overview]
Message-ID: <YzMoWghqNJdYBlaE@google.com> (raw)
In-Reply-To: <d55ccf1c085d4adadc8dbbbae6443059a94eaf90.camel@linux.intel.com>

On Tue, Sep 27, 2022, Robert Hoo wrote:
> On Tue, 2022-09-20 at 11:44 -0700, David Matlack wrote:
> > That being said, we might as well replace the WARN_ON_ONCE() with
> > KVM_BUG_ON(). That will still do a WARN_ON_ONCE() but has the added
> > benefit of terminating the VM.
> 
> Yeah, here was my point, WARN_ON_ONCE() might not be warning obviously
> enough, as it usually for recoverable cases. But terminating VM is also
> over action I think.

Agreed, from the helper's perspective, killing the VM is unnecessary, e.g. it can
simply flush the entire TLB as a fallback.

	if (WARN_ON_ONCE(!sp->role.direct)) {
		kvm_flush_remote_tlbs(kvm);
		return;
	}

But looking at the series as a whole, I think the better option is to just not
introduce this helper.  There's exactly one user, and if that path encounters an
indirect shadow page then KVM is deep in the weeds.  But that's a moot point,
because unless I'm misreading the flow, there's no need for the unique helper.
kvm_flush_remote_tlbs_sptep() will do the right thing even if the target SP happens
to be indirect.

If we really want to assert that the child is a direct shadow page, then
validate_direct_spte() would be the right location for such an assert.  IMO that's
unnecessary paranoia.  The odds of KVM reaching this point with a completely messed
up shadow paging tree, and without already having hosed the guest and/or yelled
loudly are very, very low.

In other words, IMO we're getting too fancy for this one and we should instead
simply do:

		child = to_shadow_page(*sptep & SPTE_BASE_ADDR_MASK);
		if (child->role.access == direct_access)
			return;

		drop_parent_pte(child, sptep);
		kvm_flush_remote_tlbs_sptep(kvm, sptep);

That has the added benefit of flushing the same "thing" that is zapped, i.e. both
operate on the parent SPTE, not the child.

Note, kvm_flush_remote_tlbs_sptep() operates on the _parent_, where the above
snippets retrieves the child.  This becomes much more obvious once spte_to_child_sp()
comes along: https://lore.kernel.org/all/20220805230513.148869-8-seanjc@google.com.