Re: [PATCH v2 11/13] mshv: unlock SNP pages on panic for crashdump collection

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Panic notifier fails to set crash_kexec_post_notifiers globally, bypassing memory unmap in default configurations.
- [High] Acquiring spinlock_t via spin_trylock in a panic notifier is structurally unsafe and violates PREEMPT_RT wait-context rules.
- [Medium] Panic path memory unlocking aborts on the first chunk error, failing to execute best-effort vmcore recovery.
- [Low] Missing rcu_read_lock() during RCU hash table traversal.
- [Low] Misleading success message is logged unconditionally even if SNP unlocking completely aborts.
--

--- Patch [11]: [PATCH v2 11/13] mshv: unlock SNP pages on panic for crashdump collection ---
commit b8503cbf714b3ffa7932540f4a80cc94da615bdc
Author: Wei Hu <[email protected]>

mshv: unlock SNP pages on panic for crashdump collection

Add an SNP panic-path page-unlock feature so a root partition running
confidential (SEV-SNP) guests can still collect a Linux root vmcore via
kexec after a crash. On panic, guest pages of every encrypted partition
are unmapped and shared back to the host so kexec can read them.

> diff --git a/drivers/hv/mshv_root_main.c b/drivers/hv/mshv_root_main.c
> index 0dbf509351bb0..6b38a45ad43cc 100644
> --- a/drivers/hv/mshv_root_main.c
> +++ b/drivers/hv/mshv_root_main.c
> @@ -2941,6 +2941,81 @@ static int __init mshv_init_vmm_caps(struct device *dev)
>  	return 0;
>  }
>  
> +#if defined(CONFIG_X86_64) && IS_ENABLED(CONFIG_CRASH_DUMP)
> +static void mshv_panic_unlock_snp(struct mshv_partition *vm)
> +{
> +	struct mshv_mem_region *memreg;
> +	int ret;
> +
> +	if (!spin_trylock(&vm->pt_mem_regions_lock)) {
> +		pt_err(vm, "Cannot lock SNP memory regions during panic\n");
> +		return;
> +	}

[Severity: High]
Is it safe to acquire a spinlock_t via spin_trylock() in a panic notifier?

Calling spin_trylock() on a spinlock_t from a hardirq or NMI context violates
PREEMPT_RT wait-context rules. 

Additionally, if the CPU that panicked happens to hold this lock, the trylock
will fail and bypass the memory unmap entirely, breaking the kexec crashdump.

> +
> +	hlist_for_each_entry(memreg, &vm->pt_mem_regions, hnode) {
> +		mshv_region_unmap(memreg);
> +		ret = mshv_region_share(memreg);

[Severity: Medium]
Will this abort prematurely if an error occurs on a single chunk?

The return value of mshv_region_unmap() is ignored, and both underlying
functions call mshv_region_process_range() which aborts immediately on the
first error.

Could a best-effort approach be used here to continue attempting to unlock
remaining regions instead of halting on the first hypercall failure?

> +		if (ret)
> +			pt_err(vm, "Unlock snp failed. ret:0x%x gfn:%llx numpfns:%lld\n",
> +			       ret, memreg->start_gfn, memreg->nr_pages);
> +	}
> +
> +	spin_unlock(&vm->pt_mem_regions_lock);
> +}
> +
> +static int mshv_root_panic_cb(struct notifier_block *this, unsigned long event,
> +			      void *ptr)
> +{
> +	int i, done = 0;
> +	struct mshv_partition *pt;
> +	struct device *dev = NULL;
> +
> +	hash_for_each_rcu(mshv_root.pt_htable, i, pt, pt_hnode) {

[Severity: Low]
Should this RCU hash table traversal be protected by rcu_read_lock()?

While preemption is stopped in the panic path, RCU subsystem semantics still
dictate explicitly delimiting the critical section to prevent static checker
warnings.

> +		if (!mshv_partition_encrypted(pt))
> +			continue;
> +
> +		done = 1;
> +		mshv_panic_unlock_snp(pt);
> +		dev = pt->pt_module_dev;
> +	}
> +	if (done && dev)
> +		dev_info(dev, "SNP pages are unlocked for panic\n");

[Severity: Low]
Could this emit a misleading success message?

The done flag is set to 1 before mshv_panic_unlock_snp() is called. If the
spin_trylock() inside mshv_panic_unlock_snp() fails and returns early, or if
an error occurs during unmapping, this will still unconditionally log that SNP
pages are unlocked.

> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block mshv_root_panic_blk = {
> +	.notifier_call = mshv_root_panic_cb,
> +};
> +
> +/*
> + * If mshv devirt setup failed during boot, or the feature itself is not
> + * available, allow the system to at least collect linux root vmcore. For
> + * that, snp guest pages must be made readable in the panic path so kexec can
> + * collect them.
> + */
> +static void mshv_crashdump_init(void)
> +{
> +	if (hv_crash_enabled)
> +		return;
> +
> +	atomic_notifier_chain_register(&panic_notifier_list,
> +				       &mshv_root_panic_blk);

[Severity: High]
Does this setup guarantee that the notifier will run before kexec boots?

By default, panic() calls __crash_kexec() before evaluating
panic_notifier_list.

Without explicitly setting crash_kexec_post_notifiers to true here, the
kdump kernel will boot immediately upon panic on standard configurations,
skipping the notifier list and leaving SNP memory locked.

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