linux-kernel - Re: [PATCH v3] KVM: PPC: Book3S HV: Mask off LPCR_MER for a vCPU before running it to avoid spurious interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87ttcjwudz.fsf@gmail.com>
Date: Thu, 07 Nov 2024 09:46:40 +0530
From: "Ritesh Harjani (IBM)" <ritesh.list@...il.com>
To: Gautam Menghani <gautam@...ux.ibm.com>, mpe@...erman.id.au, npiggin@...il.com, christophe.leroy@...roup.eu, naveen@...nel.org, maddy@...ux.ibm.com
Cc: Gautam Menghani <gautam@...ux.ibm.com>, linuxppc-dev@...ts.ozlabs.org, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] KVM: PPC: Book3S HV: Mask off LPCR_MER for a vCPU before running it to avoid spurious interrupts

Gautam Menghani <gautam@...ux.ibm.com> writes:

> Running a L2 vCPU (see [1] for terminology) with LPCR_MER bit set and no
> pending interrupts results in that L2 vCPU getting an infinite flood of
> spurious interrupts. The 'if check' in kvmhv_run_single_vcpu() sets the
> LPCR_MER bit if there are pending interrupts.
>
> The spurious flood problem can be observed in 2 cases:
> 1. Crashing the guest while interrupt heavy workload is running
>   a. Start a L2 guest and run an interrupt heavy workload (eg: ipistorm)
>   b. While the workload is running, crash the guest (make sure kdump
>      is configured)
>   c. Any one of the vCPUs of the guest will start getting an infinite
>      flood of spurious interrupts.
>
> 2. Running LTP stress tests in multiple guests at the same time
>    a. Start 4 L2 guests.
>    b. Start running LTP stress tests on all 4 guests at same time.
>    c. In some time, any one/more of the vCPUs of any of the guests will
>       start getting an infinite flood of spurious interrupts.
>
> The root cause of both the above issues is the same:
> 1. A NMI is sent to a running vCPU that has LPCR_MER bit set.
> 2. In the NMI path, all registers are refreshed, i.e, H_GUEST_GET_STATE
>    is called for all the registers.
> 3. When H_GUEST_GET_STATE is called for LPCR, the vcpu->arch.vcore->lpcr
>    of that vCPU at L1 level gets updated with LPCR_MER set to 1, and this
>    new value is always used whenever that vCPU runs, regardless of whether
>    there was a pending interrupt.
> 4. Since LPCR_MER is set, the vCPU in L2 always jumps to the external
>    interrupt handler, and this cycle never ends.
>
> Fix the spurious flood by masking off the LPCR_MER bit before running a
> L2 vCPU to ensure that it is not set if there are no pending interrupts.
>
> [1] Terminology:
> 1. L0 : PAPR hypervisor running in HV mode
> 2. L1 : Linux guest (logical partition) running on top of L0
> 3. L2 : KVM guest running on top of L1
>
> Fixes: ec0f6639fa88 ("KVM: PPC: Book3S HV nestedv2: Ensure LPCR_MER bit is passed to the L0")
> Cc: stable@...r.kernel.org # v6.8+
> Signed-off-by: Gautam Menghani <gautam@...ux.ibm.com>
> ---
> v1 -> v2:
> 1. Removed the macro which was silently clearing LPCR_MER bit from vcore->lpcr
> and instead just masked it off while sending it to kvmhv_run_single_vcpu().
> Added an inline comment describing the reason to avoid anyone tipping
> it over. (Suggested by Ritesh in an internal review)
>
> v2 -> v3:
> 1. Moved the masking of LPCR_MER from kvmppc_vcpu_run_hv() to
> kvmhv_run_single_vcpu() (Suggested by Michael Ellerman)
>
>  arch/powerpc/kvm/book3s_hv.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
> index 8f7d7e37bc8c..0ed5c5c7a350 100644
> --- a/arch/powerpc/kvm/book3s_hv.c
> +++ b/arch/powerpc/kvm/book3s_hv.c
> @@ -4892,6 +4892,18 @@ int kvmhv_run_single_vcpu(struct kvm_vcpu *vcpu, u64 time_limit,
>  							   BOOK3S_INTERRUPT_EXTERNAL, 0);
>  			else
>  				lpcr |= LPCR_MER;
> +		} else {
> +			/*
> +			 * L1's copy of L2's LPCR (vcpu->arch.vcore->lpcr) can get its MER bit
> +			 * unexpectedly set - for e.g. during NMI handling when all register
> +			 * states are synchronized from L0 to L1. L1 needs to inform L0 about
> +			 * MER=1 only when there are pending external interrupts.
> +			 * In the above if check, MER bit is set if there are pending
> +			 * external interrupts. Hence, explicity mask off MER bit
> +			 * here as otherwise it may generate spurious interrupts in L2 KVM
> +			 * causing an endless loop, which results in L2 guest getting hung.
> +			 */
> +			lpcr &= ~LPCR_MER;
>  		}

I think we had enough discussions on v1 internally and v2 on mailing
list. So I am comfortable giving... 

LGTM. Please feel free to add - 

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>

>  	} else if (vcpu->arch.pending_exceptions ||
>  		   vcpu->arch.doorbell_request ||
> -- 
> 2.47.0