Re: [PATCH v2] KVM: arm64: Drop %pB on nVHE panic when stage-2 is active

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix for host stage-2 deadlock in nVHE hyp panic handler.
--

--- Patch [1]: [PATCH v2] KVM: arm64: Drop %pB on nVHE panic when stage-2 is active ---
commit 6bd2e7dd3552f4e44146bf3e1065ca1511de7411
Author: Vincent Donnefort <[email protected]>

KVM: arm64: Drop %pB on nVHE panic when stage-2 is active

This patch introduces nvhe_hyp_panic_host_s2_disabled() to skip %pB
formatting during a hyp panic when host stage-2 is active. It aims to prevent
deadlocks that occur if the host stage-2 lock is held and a fault happens
when accessing lazily mapped kallsyms data.

> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index 54aedf93c78b6..88dcb8cb36f3c 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
[ ... ]
> @@ -538,8 +548,7 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
>  		unsigned int line = 0;
>  
>  		/* All hyp bugs, including warnings, are treated as fatal. */
> -		if (!is_protected_kvm_enabled() ||
> -		    IS_ENABLED(CONFIG_PKVM_DISABLE_STAGE2_ON_PANIC)) {
> +		if (nvhe_hyp_panic_host_s2_disabled()) {
>  			struct bug_entry *bug = find_bug(elr_in_kimg);
>  
>  			if (bug)

[Severity: High]
This is a pre-existing issue, but does this fully resolve the host stage-2
deadlock?

Further down in this same function, the backtrace and instruction dumpers
are called unconditionally:

	/* Dump the nVHE hypervisor backtrace */
	kvm_nvhe_dump_backtrace(hyp_offset);

	/* Dump the faulting instruction */
	dump_kernel_instr(panic_addr + kaslr_offset());

kvm_nvhe_dump_backtrace() iterates over stack frames and calls
kvm_nvhe_dump_backtrace_entry(), which uses the exact same %pB formatter
to print the frames:

arch/arm64/kvm/stacktrace.c:kvm_nvhe_dump_backtrace_entry() {
    ...
	kvm_err(" [<%016lx>] %pB\n", where, (void *)(where + kaslr_offset()));
    ...
}

Similarly, dump_kernel_instr() reads the faulting instruction directly from
kernel memory, which could trigger a stage-2 data abort if the page is not
present in the lazy host stage-2 page tables:

arch/arm64/kernel/traps.c:dump_kernel_instr() {
    ...
		bad = aarch64_insn_read(&((u32 *)kaddr)[i], &val);
    ...
}

If a panic occurs while the host stage-2 lock is held, won't these
unconditional calls still trigger the stage-2 fault and deadlock that
this patch is trying to avoid?

Should these operations also be gated by nvhe_hyp_panic_host_s2_disabled()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1