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

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Deadlock in panic path due to `free_folio` attempting to acquire blocking locks.
- [High] Zeroization loop silently fails because it iterates over an empty `sb->s_inodes` list.
- [Low] Missing `spin_unlock` for `s_inode_list_lock` on the success path.
--

--- Patch [3]: [PATCH 3/4] mm/secretmem: zeroize secret pages before kdump ---
commit 2960931b04ec8dddbd17c954634f6a26336377a1
Author: Jan Sebastian Götte <[email protected]>

mm/secretmem: zeroize secret pages before kdump

This commit adds a crash notifier to zeroize secretmem folios before a kdump
kernel is kexec'ed. It iterates over the page cache without taking the i_pages
lock, as zeroization runs after other CPUs have been stopped.

> diff --git a/mm/secretmem.c b/mm/secretmem.c
> --- 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) {

[Severity: High]
Does this loop actually iterate over any secretmem files?

In secretmem_file_create(), the inode is allocated via
anon_inode_make_secure_inode(). That function uses new_inode_pseudo(), which
intentionally skips adding the inode to the superblock's s_inodes list.

Because of this, won't sb->s_inodes be empty (or contain only the root inode),
causing the zeroization process to silently skip all secretmem folios?

> +		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: High]
Can calling free_folio() here cause a deadlock during a kernel panic?

The free_folio callback points to secretmem_free_folio(), which eventually
calls set_direct_map_default_noflush() on the folio's head page.

On architectures like x86, set_direct_map_default_noflush() resolves to
__set_pages_p(), which acquires cpa_lock (via spin_lock(&cpa_lock)). If a
secondary CPU was stopped by an NMI while holding cpa_lock, won't this cause
the panicking CPU to spin indefinitely, preventing the crash kernel from
loading?

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

[Severity: Low]
Is there a missing spin_unlock(&sb->s_inode_list_lock) before returning on
this success path?

Even though the system is in a terminal state, leaving the lock acquired via
spin_trylock() permanently held could unbalance preemption or confuse lock
debugging if it runs after this point.

> +}
> +
> +static struct notifier_block secretmem_zeroize_nb = {
> +	.notifier_call = secretmem_crash_zeroize
> +};
> +#endif /* CONFIG_CRASH_ZEROIZE */

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