Re: [PATCH v2 25/27] KVM: x86/mmu: Drop @slot param from exported/external page-track APIs

[Yan Zhao <yan.y.zhao@intel.com>]

On Thu, May 04, 2023 at 10:17:20AM +0800, Yan Zhao wrote:
> On Wed, May 03, 2023 at 04:16:10PM -0700, Sean Christopherson wrote:
> > Finally getting back to this series...
> > 
> > On Thu, Mar 23, 2023, Yan Zhao wrote:
> > > On Fri, Mar 17, 2023 at 04:28:56PM +0800, Yan Zhao wrote:
> > > > On Fri, Mar 10, 2023 at 04:22:56PM -0800, Sean Christopherson wrote:
> > > > ...
> > > > > +int kvm_write_track_add_gfn(struct kvm *kvm, gfn_t gfn)
> > > > > +{
> > > > > +	struct kvm_memory_slot *slot;
> > > > > +	int idx;
> > > > > +
> > > > > +	idx = srcu_read_lock(&kvm->srcu);
> > > > > +
> > > > > +	slot = gfn_to_memslot(kvm, gfn);
> > > > > +	if (!slot) {
> > > > > +		srcu_read_unlock(&kvm->srcu, idx);
> > > > > +		return -EINVAL;
> > > > > +	}
> > > > > +
> > > > Also fail if slot->flags & KVM_MEMSLOT_INVALID is true?
> > > > There should exist a window for external users to see an invalid slot
> > > > when a slot is about to get deleted/moved.
> > > > (It happens before MOVE is rejected in kvm_arch_prepare_memory_region()).
> > > 
> > > Or using
> > >         if (!kvm_is_visible_memslot(slot)) {
> > > 		srcu_read_unlock(&kvm->srcu, idx);
> > > 		return -EINVAL;
> > > 	}
> > 
Hi Sean,
After more thoughts, do you think checking KVM internal memslot is necessary?

slot = gfn_to_memslot(kvm, gfn);
if (!slot || slot->id >= KVM_USER_MEM_SLOTS) {
		srcu_read_unlock(&kvm->srcu, idx);
		return -EINVAL;
}

Do we allow write tracking to APIC access page when APIC-write VM exit
is not desired?

Thanks
Yan

> > Hrm.  If the DELETE/MOVE succeeds, then the funky accounting is ok (by the end
> > of the series) as the tracking disappears on DELETE, KVMGT will reject MOVE, and
> > KVM proper zaps SPTEs and resets accounting on MOVE (account_shadowed() runs under
> > mmu_lock and thus ensures all previous SPTEs are zapped before the "flush" from
> > kvm_arch_flush_shadow_memslot() can run).
> > 
> > If kvm_prepare_memory_region() fails though...
> > 
> > Ah, KVM itself is safe because of the aforementioned kvm_arch_flush_shadow_memslot().
> > Any accounting done on a temporarily invalid memslot will be unwound when the SPTEs
> > are zapped.  So for KVM, ignoring invalid memslots is correct _and necessary_.
> > We could clean that up by having accounted_shadowed() use the @slot from the fault,
> > which would close the window where the fault starts with a valid memslot but then
> > sees an invalid memslot when accounting a new shadow page.  But I don't think there
> > is a bug there.
> > 
> > Right, and DELETE can't actually fail in the current code base, and we've established
> > that MOVE can't possibly work.  So even if this is problematic in theory, there are
> > no _unknown_ bugs, and the known bugs are fixed by the end of the series.
> > 
> > And at the end of the series, KVMGT drops its tracking only when the DELETE is
> > committed.  So I _think_ allowing external trackers to add and remove gfns for
> > write-tracking in an invalid slot is actually desirable/correct.  I'm pretty sure
> > removal should be allowed as that can lead to dangling write-protection in a
> > rollback scenario.   And I can't think of anything that will break (in the kernel)
> > if write-tracking a gfn in an invalid slot is allowed, so I don't see any harm in
> > allowing the extremely theoretical case of KVMGT shadowing a gfn in a to-be-deleted
> > memslot _and_ the deletion being rolled back.
> Yes, you are right!
> I previously thought that
> invalid_slot->arch.gfn_write_track and old->arch.gfn_write_track are
> pointing to different places. But I'm wrong.
> Yes, allowing INVALID slot here is more desired for deletion rolling-back.
>