Re: [WIP Patch v2 00/14] Avoiding slow get-user-pages via memory fault exit

[Sean Christopherson <seanjc@google.com>]

On Fri, Mar 17, 2023, Sean Christopherson wrote:
> On Fri, Mar 17, 2023, Anish Moorthy wrote:
> > On Fri, Mar 17, 2023 at 12:00 PM David Matlack <dmatlack@google.com> wrote:
> > > > The low-level accessors are common across architectures and can be called from
> > > > other contexts besides a vCPU. Is it possible for the caller to catch -EFAULT
> > > > and convert that into an exit?
> > >
> > > Ya, as things stand today, the conversions _must_ be performed at the caller, as
> > > there are (sadly) far too many flows where KVM squashes the error.  E.g. almost
> > > all of x86's paravirt code just suppresses user memory faults :-(
> > >
> > > Anish, when we discussed this off-list, what I meant by limiting the intial support
> > > to existing -EFAULT cases was limiting support to existing cases where KVM directly
> > > returns -EFAULT to userspace, not to all existing cases where -EFAULT is ever
> > > returned _within KVM_ while handling KVM_RUN.  My apologies if I didn't make that clear.
> > 
> > Don't worry, we eventually got there off-list :)
> > 
> > This brings us back to my original set of questions. As has already
> > been pointed out, I'll have to revisit my "Confident that needs
> > conversion" changes and tweak them so that the vCPU exit is populated
> > only for the call sites where the -EFAULT makes it to userspace. I
> > still want feedback on if I've mis-identified any of the functions in
> > my "EFAULT does not propagate to userspace" list and whether there are
> > functions/callers in the "Still unsure if needs conversion" which do
> > have return paths to KVM_RUN.
> 
> As you've probably gathered from the type of feedback you're receiving, identifying
> the conversion touchpoints isn't going to be the long pole of this series.  Correctly
> identifying all of the touchpoints may not be easy, but fixing any cases we get wrong
> will likely be straightforward.  And realistically, no matter how many eyeballs look
> at the code, odds are good we'll miss at least one case.  In other words, don't worry
> too much about getting all the touchpoints correct on the first version.  Getting the
> uAPI right is much more important.
> 
> And rather than rely on code review to get things right, we should be able to
> detect issues programmatically.  E.g. use fault injection to make gup() and/or
> uaccess fail (might even be wired up already?), and hack in a WARN in the KVM_RUN
> path to assert that KVM_EXIT_MEMORY_FAULT is filled if the return code is -EFAULT
> (assuming we go don't try to get KVM to return 0 everywhere), e.g. something like
> the below would at least flag the "misses", although debug could still prove to be
> annoying.
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 67b890e54cf1..cccae0ad1436 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4100,6 +4100,8 @@ static long kvm_vcpu_ioctl(struct file *filp,
>                 }
>                 r = kvm_arch_vcpu_ioctl_run(vcpu);
>                 trace_kvm_userspace_exit(vcpu->run->exit_reason, r);
> +               WARN_ON(r == -EFAULT &&
> +                       vcpu->run->exit_reason == KVM_EXIT_MEMORY_FAULT);

Gah, I inverted the second check, this should be 

		WARN_ON(r == -EFAULT &&
			vcpu->run->exit_reason != KVM_EXIT_MEMORY_FAULT);
		
>                 break;
>         }
>         case KVM_GET_REGS: {
>