Re: [PATCH v4 18/18] mm: factor out can_spin_trylock()

[Brendan Jackman]

On Thu Jul 2, 2026 at 12:28 PM UTC, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] The refactoring to deduplicate spin_trylock safety checks into can_spin_trylock() missed __free_frozen_pages(), leaving a pre-existing UP NMI corruption bug unpatched.
> --
>
> Note: The format of this report has been altered to provide a free-form
> summary of the findings due to recitation restrictions.
>
> commit a99a7a468132ab177e1233c7e2f5644523ace0a7
> Author: Brendan Jackman <jackmanb@xxxxxxxxxx>
> Subject: mm: factor out can_spin_trylock()
>
> This commit introduces can_spin_trylock() to deduplicate checks for whether 
> the current context is safe for spin_trylock(). It updates 
> alloc_nolock_allowed() in mm/page_alloc.c and __kmalloc_nolock_noprof() in 
> mm/slub.c to use this newly factored helper.
>
> Link: https://lore.kernel.org/all/397859cb-b127-4cc6-9c71-044afc99bf0c@xxxxxxxxxx/
>
> [Severity: High]
>
> This isn't a bug introduced by this patch, but the deduplication refactoring 
> appears to have missed an open-coded check in __free_frozen_pages() in 
> mm/page_alloc.c. 
>
> Could this open-coded check also be updated to use the new 
> can_spin_trylock() helper to resolve a pre-existing UP NMI safety issue?
>
> When __free_frozen_pages() handles FPI_TRYLOCK, it explicitly checks for
> CONFIG_PREEMPT_RT and NMI/hardirq context. On UP systems where CONFIG_SMP 
> is disabled, spin_trylock() and its variants are defined to unconditionally 
> succeed because there is no actual lock state tracked.
>
> If a caller invokes free_pages_nolock() from an NMI context on a UP system, 
> the current RT-specific check fails. The execution then falls back to 
> free_one_page(), where spin_trylock_irqsave() falsely succeeds even if the 
> NMI interrupted another task that was actively modifying the free list under 
> the zone lock. 
>
> This allows concurrent modification of the free list, which can lead to 
> corruption and kernel panics.
>
> Updating the check in __free_frozen_pages() to use !can_spin_trylock() 
> would catch the UP NMI case and safely route the allocation to the lockless 
> add_page_to_zone_llist() path instead.

I think Sashiko is right? Based on the unfixed kprobe bugs described in
[0] should we just disable nolock on UP? If that has a user, who knows
what they are up to, but I would wager that we'd prefer to break their 
probabilistic tracing than corrupt their freelists.

Not a strongly held opinion though I can also send a patch to "fix it"
if people prefer; I'll keep functional changes separate from this series
though.

[0]: https://lore.kernel.org/all/20260427-nolock-api-fix-v2-0-a6b83a92d9a4@xxxxxxxxxx/