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