| Message ID | 20240802224031.154064-4-amoorthy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Set up KVM_EXIT_MEMORY_FAULTs when arm64/x86 stage-2 fault handlers fail | expand |
On Fri, Aug 02, 2024 at 10:40:31PM +0000, Anish Moorthy wrote: > Right now userspace just gets a bare EFAULT when the stage-2 fault > handler fails to fault in the relevant page. Set up a memory fault exit > when this happens, which at the very least eases debugging and might > also let userspace decide on/take some specific action other than > crashing the VM. There are several other 'bare' EFAULTs remaining (unexpected fault context, failed vma_lookup(), nested PTW), so the patch doesn't exactly match the shortlog. Is there a reason why those are unaddressed? In any case, it doesn't hurt to be unambiguous in the shortlog if we're only focused on this single error condition, e.g. KVM: arm64: Do a memory fault exit if __gfn_to_pfn_memslot() fails > Signed-off-by: Anish Moorthy <amoorthy@google.com> > --- > arch/arm64/kvm/mmu.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 6981b1bc0946..52b4f8e648fb 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -1568,8 +1568,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, > kvm_send_hwpoison_signal(hva, vma_shift); > return 0; > } > - if (is_error_noslot_pfn(pfn)) > + if (is_error_noslot_pfn(pfn)) { > + kvm_prepare_memory_fault_exit(vcpu, fault_ipa, vma_pagesize, > + write_fault, exec_fault, false); > return -EFAULT; > + } > > if (kvm_is_device_pfn(pfn)) { > /* > -- > 2.46.0.rc2.264.g509ed76dc8-goog >
On Mon, Aug 5, 2024 at 4:02 PM Oliver Upton <oliver.upton@linux.dev> wrote: > > On Fri, Aug 02, 2024 at 10:40:31PM +0000, Anish Moorthy wrote: > > Right now userspace just gets a bare EFAULT when the stage-2 fault > > handler fails to fault in the relevant page. Set up a memory fault exit > > when this happens, which at the very least eases debugging and might > > also let userspace decide on/take some specific action other than > > crashing the VM. > > There are several other 'bare' EFAULTs remaining (unexpected fault > context, failed vma_lookup(), nested PTW), so the patch doesn't exactly > match the shortlog. > > Is there a reason why those are unaddressed? In any case, it doesn't > hurt to be unambiguous in the shortlog if we're only focused on this single > error condition, e.g. > > KVM: arm64: Do a memory fault exit if __gfn_to_pfn_memslot() fails Ah, right- forgot to address this before I sent it out. Basically: those cases you mention (besides MTE, where it seems simple enough to add an annotation) happen before vma_pagesize is calculated, and it doesn't look trivial to just move logic around to do that calculation up at the top. Lmk if there's a good solution here, but in the meantime I'll just take your suggested shortlog [1] https://github.com/torvalds/linux/blob/master/arch/arm64/kvm/mmu.c#L1479-L1514
On Tue, Aug 06, 2024 at 11:44:39AM -0700, Anish Moorthy wrote: > On Mon, Aug 5, 2024 at 4:02 PM Oliver Upton <oliver.upton@linux.dev> wrote: > > > > On Fri, Aug 02, 2024 at 10:40:31PM +0000, Anish Moorthy wrote: > > > Right now userspace just gets a bare EFAULT when the stage-2 fault > > > handler fails to fault in the relevant page. Set up a memory fault exit > > > when this happens, which at the very least eases debugging and might > > > also let userspace decide on/take some specific action other than > > > crashing the VM. > > > > There are several other 'bare' EFAULTs remaining (unexpected fault > > context, failed vma_lookup(), nested PTW), so the patch doesn't exactly > > match the shortlog. > > > > Is there a reason why those are unaddressed? In any case, it doesn't > > hurt to be unambiguous in the shortlog if we're only focused on this single > > error condition, e.g. > > > > KVM: arm64: Do a memory fault exit if __gfn_to_pfn_memslot() fails > > Ah, right- forgot to address this before I sent it out. > > Basically: those cases you mention (besides MTE, where it seems simple > enough to add an annotation) happen before vma_pagesize is calculated, If the motivation is to add additional information for debugging unexpected KVM/VMM behavior then this really ought to be addressed. You could fall back to PAGE_SIZE, or better yet just don't report a size whatsoever (size = 0) if it cannot be reliably determined. Userspace probably only cares about logging @flags and @gpa before killing the VM.
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 6981b1bc0946..52b4f8e648fb 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -1568,8 +1568,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa, kvm_send_hwpoison_signal(hva, vma_shift); return 0; } - if (is_error_noslot_pfn(pfn)) + if (is_error_noslot_pfn(pfn)) { + kvm_prepare_memory_fault_exit(vcpu, fault_ipa, vma_pagesize, + write_fault, exec_fault, false); return -EFAULT; + } if (kvm_is_device_pfn(pfn)) { /*
Right now userspace just gets a bare EFAULT when the stage-2 fault handler fails to fault in the relevant page. Set up a memory fault exit when this happens, which at the very least eases debugging and might also let userspace decide on/take some specific action other than crashing the VM. Signed-off-by: Anish Moorthy <amoorthy@google.com> --- arch/arm64/kvm/mmu.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)