Re: [PATCH v2 11/13] mshv: unlock SNP pages on panic for crashdump collection
From: sashiko-bot@kernel.org
Date: 2026-08-10 13:10:02
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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/20260810124527.1409634-1-weh@linux.microsoft.com?part=11