Re: [PATCH 3/4] mm/secretmem: zeroize secret pages before kdump

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Deadlock risk during kdump due to taking a spinlock in a panic notifier.
- [Low] Missing unlock of the `s_inode_list_lock` after successfully acquiring it.
--

--- Patch [3]: [PATCH 3/4] mm/secretmem: zeroize secret pages before kdump ---
commit e5fefbd7f9b60fca2237ceaf3f2f3cb2aa75df03
Author: Jan Sebastian Götte <linux@jaseg.de>

mm/secretmem: zeroize secret pages before kdump
    
Register a CRASH_ZEROIZE notifier that wipes secretmem folios. As a
result, when CONFIG_CRASH_ZEROIZE is set, secretmem areas will be
cleared before the kdump kernel is kexec'ed.

> diff --git a/mm/secretmem.c b/mm/secretmem.c
> index 5f57ac4720d32..07f3905f47cbd 100644
> --- a/mm/secretmem.c
> +++ b/mm/secretmem.c

[ ... ]

> @@ -187,6 +189,50 @@ static const struct inode_operations secretmem_iops = {
>  
>  static struct vfsmount *secretmem_mnt;
>  
> +#ifdef CONFIG_CRASH_ZEROIZE
> +/* Called far into vpanic from crash_core.c with other CPUs stopped and
> + * preemption disabled
> + */
> +static int secretmem_crash_zeroize(struct notifier_block *nb, unsigned long
> +		action, void *data)
> +{
> +	struct super_block *sb;
> +	struct inode *inode;
> +
> +	if (!secretmem_mnt)
> +		return NOTIFY_DONE;
> +	sb = secretmem_mnt->mnt_sb;
> +
> +	/* If the list was modified in the exact moment we panic'ed, it might be
> +	 * in an inconsistent state that would be unsafe to iterate. If we can't
> +	 * get the lock, too bad, that's all we can do here.
> +	 */
> +	if (!spin_trylock(&sb->s_inode_list_lock)) {
> +		pr_crit("crash_zeroize: can't acquire secretmem superblock lock.\n"
> +			 "crash_zeroize: skipping zeroizing secretmem.\n");
> +		return NOTIFY_DONE;
> +	}
> +
> +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> +		XA_STATE(xas, &inode->i_mapping->i_pages, 0);
> +		struct folio *folio;
> +
> +		/* no need for locks if we're burning down the house :) */
> +		xas_for_each(&xas, folio, ULONG_MAX) {
> +			if (xas_retry(&xas, folio) || xa_is_value(folio))
> +				continue;
> +			inode->i_mapping->a_ops->free_folio(folio);

[Severity: Critical]
Can this free_folio callback deadlock the panicking CPU?

The free_folio operation maps to secretmem_free_folio(), which invokes
set_direct_map_default_noflush(). On x86, this eventually calls
__change_page_attr_set_clr(), which unconditionally acquires cpa_lock
(when debug_pagealloc is not enabled):

arch/x86/mm/pat/set_memory.c:__change_page_attr_set_clr() {
    ...
    if (!debug_pagealloc_enabled())
        spin_lock(&cpa_lock);
    ...
}

Because this crash notifier runs after other CPUs have been forcibly halted
(e.g., via NMIs), if any of the stopped CPUs were interrupted while holding
cpa_lock (such as during module load or vmalloc), won't the panicking CPU
spin forever here and prevent the crash dump kernel from booting?

> +		}
> +	}
> +	/* off to kexec()! */
> +	return NOTIFY_DONE;

[Severity: Low]
Should we unlock sb->s_inode_list_lock before returning?

While this is the panic path and the system is about to kexec into the crash
kernel, leaving a successfully acquired spin_trylock unbalanced can trigger
warnings in static analysis tools or confuse readers.

> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731162739.158320-1-linux@jaseg.de?part=3