On Fri, Mar 17, 2023, Anish Moorthy wrote:
> On Fri, Mar 17, 2023 at 12:00 PM David Matlack <dmatlack@xxxxxxxxxx> 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);
break;
}
case KVM_GET_REGS: {
