On Tue, May 09, 2023, David Matlack wrote: > On Wed, Apr 12, 2023 at 09:34:48PM +0000, Anish Moorthy wrote: > > Upon receiving an annotated EFAULT, userspace may take appropriate > > action to resolve the failed access. For instance, this might involve a > > UFFDIO_CONTINUE or MADV_POPULATE_WRITE in the context of uffd-based live > > migration postcopy. > > As implemented, I think it will be prohibitively expensive if not > impossible for userspace to determine why KVM is returning EFAULT when > KVM_CAP_ABSENT_MAPPING_FAULT is enabled, which means userspace can't > decide the correct action to take (try to resolve or bail). > > Consider the direct_map() case in patch in PATCH 15. The only way to hit > that condition is a logic bug in KVM or data corruption. There isn't > really anything userspace can do to handle this situation, and it has no > way to distinguish that from faults to due absent mappings. > > We could end up hitting cases where userspace loops forever doing > KVM_RUN, EFAULT, UFFDIO_CONTINUE/MADV_POPULATE_WRITE, KVM_RUN, EFAULT... > > Maybe we should just change direct_map() to use KVM_BUG() and return > something other than EFAULT. But the general problem still exists and > even if we have confidence in all the current EFAULT sites, we don't have > much protection against someone adding an EFAULT in the future that > userspace can't handle. Yeah, when I speed read the series, several of the conversions stood out as being "wrong". My (potentially unstated) idea was that KVM would only signal KVM_EXIT_MEMORY_FAULT when the -EFAULT could be traced back to a user access, i.e. when the fault _might_ be resolvable by userspace. If we want to populate KVM_EXIT_MEMORY_FAULT even on kernel bugs, and anything else that userspace can't possibly resolve, then the easiest thing would be to add a flag to signal that the fault is fatal, i.e. that userspace shouldn't retry. Adding a flag may be more robust in the long term as it will force developers to think about whether or not a fault is fatal, versus relying on documentation to say "don't signal KVM_EXIT_MEMORY_FAULT for fatal EFAULT conditions". Side topic, KVM x86 really should have a version of KVM_SYNC_X86_REGS that stores registers for userspace, but doesn't load registers. That would allow userspace to detect many infinite loops with minimal overhead, e.g. (1) set KVM_STORE_X86_REGS during demand paging, (2) check RIP on every exit to see if the vCPU is making forward progress, (3) escalate to checking all registers if RIP hasn't changed for N exits, and finally (4) take action if the guest is well and truly stuck after N more exits. KVM could even store RIP on every exit if userspace wanted to avoid the overhead of storing registers until userspace actually wants all registers.
