Re: [PATCH v4 03/16] KVM: Add KVM_CAP_MEMORY_FAULT_INFO

[Sean Christopherson <seanjc@google.com>]

On Tue, Jul 11, 2023, Kautuk Consul wrote:
> > > That said, I agree that there's a risk that KVM could clobber vcpu->run_run by
> > > hitting an -EFAULT without the vCPU loaded, but that's a solvable problem, e.g.
> > > the helper to fill KVM_EXIT_MEMORY_FAULT could be hardened to yell if called
> > > without the target vCPU being loaded:
> > >
> > >     int kvm_handle_efault(struct kvm_vcpu *vcpu, ...)
> > >     {
> > >         preempt_disable();
> > >         if (WARN_ON_ONCE(vcpu != __this_cpu_read(kvm_running_vcpu)))
> > >             goto out;
> > >
> > >         vcpu->run->exit_reason = KVM_EXIT_MEMORY_FAULT;
> > >         ...
> > >     out:
> > >         preempt_enable();
> > >         return -EFAULT;
> > >     }
> > 
> > Ancient history aside, let's figure out what's really needed here.
> > 
> > > Why use WARN_ON_ONCE when there is a clear possiblity of preemption
> > > kicking in (with the possibility of vcpu_load/vcpu_put being called
> > > in the new task) before preempt_disable() is called in this function ?
> > > I think you should use WARN_ON_ONCE only where there is some impossible
> > > or unhandled situation happening, not when there is a possibility of that
> > > situation clearly happening as per the kernel code.
> > 
> > I did some mucking around to try and understand the kvm_running_vcpu
> > variable, and I don't think preemption/rescheduling actually trips the
> > WARN here? From my (limited) understanding, it seems that the
> > thread being preempted will cause a vcpu_put() via kvm_sched_out().
> > But when the thread is eventually scheduled back in onto whatever
> > core, it'll vcpu_load() via kvm_sched_in(), and the docstring for
> > kvm_get_running_vcpu() seems to imply the thing that vcpu_load()
> > stores into the per-cpu "kvm_running_vcpu" variable will be the same
> > thing which would have been observed before preemption.
> > 
> > All that's to say: I wouldn't expect the value of
> > "__this_cpu_read(kvm_running_vcpu)" to change in any given thread. If
> > that's true, then the things I would expect this WARN to catch are (a)
> > bugs where somehow the thread gets scheduled without calling
> > vcpu_load() or (b) bizarre situations (probably all bugs?) where some
> > vcpu thread has a hold of some _other_ kvm_vcpu* and is trying to do
> > something with it.
> Oh I completely missed the scheduling path for KVM.
> But since vcpu_put and vcpu_load are exported symbols, I wonder what'll
> happen when there are calls to these functions from places other
> than kvm_sched_in() and kvm_sched_out() ? Just thinking out loud.

Invoking this helper without the target vCPU loaded on the current task would be
considered a bug.  kvm.ko exports a rather disgusting number of symbols purely for
use by vendor modules, e.g. kvm-intel.ko and kvm-amd.ko on x86.  The exports are
not at all intended to be used by non-KVM code, i.e. any such misuse would also be
considered a bug.