[PATCH] KVM: arm/arm64: Signal SIGBUS when stage2 discovers hwpoison memory

[Marc Zyngier]

On 27/03/17 14:31, Punit Agrawal wrote:
> Christoffer Dall <cdall at linaro.org> writes:
> 
>> On Mon, Mar 27, 2017 at 01:00:56PM +0100, James Morse wrote:
>>> Hi guys,
>>>
>>> On 27/03/17 12:20, Punit Agrawal wrote:
>>>> Christoffer Dall <cdall at linaro.org> writes:
>>>>> On Wed, Mar 15, 2017 at 04:07:27PM +0000, James Morse wrote:
>>>>>> Once we enable ARCH_SUPPORTS_MEMORY_FAILURE on arm64[0], notifications for
>>>>>> broken memory can call memory_failure() in mm/memory-failure.c to deliver
>>>>>> SIGBUS to any user space process using the page, and notify all the
>>>>>> in-kernel users.
>>>>>>
>>>>>> If the page corresponded with guest memory, KVM will unmap this page
>>>>>> from its stage2 page tables. The user space process that allocated
>>>>>> this memory may have never touched this page in which case it may not
>>>>>> be mapped meaning SIGBUS won't be delivered.
>>>>>>
>>>>>> When this happens KVM discovers pfn == KVM_PFN_ERR_HWPOISON when it
>>>>>> comes to process the stage2 fault.
>>>>>>
>>>>>> Do as x86 does, and deliver the SIGBUS when we discover
>>>>>> KVM_PFN_ERR_HWPOISON. Use the stage2 mapping size as the si_addr_lsb
>>>>>> as this matches the user space mapping size.
>>>
>>>>>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>>>>>> index 962616fd4ddd..9d1aa294e88f 100644
>>>>>> --- a/arch/arm/kvm/mmu.c
>>>>>> +++ b/arch/arm/kvm/mmu.c
>>>>>> @@ -20,8 +20,10 @@
>>>>>>  #include <linux/kvm_host.h>
>>>>>>  #include <linux/io.h>
>>>>>>  #include <linux/hugetlb.h>
>>>>>> +#include <linux/sched/signal.h>
>>>>>>  #include <trace/events/kvm.h>
>>>>>>  #include <asm/pgalloc.h>
>>>>>> +#include <asm/siginfo.h>
>>>>>>  #include <asm/cacheflush.h>
>>>>>>  #include <asm/kvm_arm.h>
>>>>>>  #include <asm/kvm_mmu.h>
>>>>>> @@ -1237,6 +1239,23 @@ static void coherent_cache_guest_page(struct kvm_vcpu *vcpu, kvm_pfn_t pfn,
>>>>>>  	__coherent_cache_guest_page(vcpu, pfn, size);
>>>>>>  }
>>>>>>  
>>>>>> +static void kvm_send_hwpoison_signal(unsigned long address, bool hugetlb)
>>>>>> +{
>>>>>> +	siginfo_t info;
>>>>>> +
>>>>>> +	info.si_signo   = SIGBUS;
>>>>>> +	info.si_errno   = 0;
>>>>>> +	info.si_code    = BUS_MCEERR_AR;
>>>>>> +	info.si_addr    = (void __user *)address;
>>>>>> +
>>>>>> +	if (hugetlb)
>>>>>> +		info.si_addr_lsb = PMD_SHIFT;
>>>>>> +	else
>>>>>> +		info.si_addr_lsb = PAGE_SHIFT;
>>>>>> +
>>>>>> +	send_sig_info(SIGBUS, &info, current);
>>>>>> +}
>>>>>> +
>>>>>>  static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>>>  			  struct kvm_memory_slot *memslot, unsigned long hva,
>>>>>>  			  unsigned long fault_status)
>>>>>> @@ -1306,6 +1325,10 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>>>>>>  	smp_rmb();
>>>>>>  
>>>>>>  	pfn = gfn_to_pfn_prot(kvm, gfn, write_fault, &writable);
>>>>>> +	if (pfn == KVM_PFN_ERR_HWPOISON) {
>>>>>> +		kvm_send_hwpoison_signal(hva, hugetlb);
>>>>>
>>>>> The way this is called means that we'll only notify userspace of a huge
>>>>> mapping if userspace is mapping hugetlbfs, and not because the stage2
>>>>> mapping may or may not have used transparent huge pages when the error
>>>>> was discovered.  Is this the desired semantics?
>>>
>>> No,
>>>
>>>
>>>> I think so.
>>>>
>>>> AFAIUI, transparent hugepages are split before being poisoned while all
>>>> the underlying pages of a hugepage are poisoned together, i.e., no
>>>> splitting.
>>>
>>> In which case I need to look into this some more!
>>>
>>> My thinking was we should report the size that was knocked out of the stage2 to
>>> avoid the guest repeatedly faulting until it has touched every guest-page-size
>>> in the stage2 hole.
>>
>> By signaling something at the fault path, I think it's going to be very
>> hard to backtrack how the stage 2 page tables looked like when faults
>> started happening, because I think these are completely decoupled events
>> (the mmu notifier and the later fault).
>>
>>>
>>> Reading the code in that kvm/mmu.c it looked like the mapping sizes would always
>>> be the same as those used by userspace.
>>
>> I think the mapping sizes should be the same between userspace and KVM,
>> but the mapping size of a particular page (and associated pages) may
>> vary over time.
> 
> Stage 1 and Stage 2 support different hugepage sizes. A larger size
> stage 1 page maps to multiple stage 2 page table entries. For stage 1,
> we support PUD_SIZE, CONT_PMD_SIZE, PMD_SIZE and CONT_PTE_SIZE while
> only PMD_SIZE is supported for Stage 2.

What is stage-1 doing here? We have no idea about what stage-1 is doing
(not under KVM's control). Or do you mean userspace instead?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...