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;
> > }
>
> 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.
