Re: [PATCH 1/2] mm/page_alloc: don't spin_trylock() in NMI on UP

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `!can_spin_trylock()` safety check in `__free_frozen_pages()` is bypassed for allocations with order > `pcp_allowed_order()` and for isolated pages, leaving them vulnerable to unsafe lock acquisition and potential memory corruption on UP systems or deadlocks on PREEMPT_RT.
--

--- Patch [1]: [PATCH 1/2] mm/page_alloc: don't spin_trylock() in NMI on UP ---
commit af22d8399ea9f942d20dfb24bc4e88e16f5fe2e1
Author: Brendan Jackman <[email protected]>

mm/page_alloc: don't spin_trylock() in NMI on UP
    
This commit updates the NMI and hardirq safety check in __free_frozen_pages()
to use can_spin_trylock(). It addresses an issue on UP systems where
spin_trylock() is unsafe in NMI context, which was missed on the free path.

Link: https://sashiko.dev/#/patchset/[email protected]

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9c97a86da2b9f..5fe1c11f919d7 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2979,8 +2979,7 @@ static void __free_frozen_pages(struct page *page, unsigned int order,
>  		migratetype = MIGRATE_MOVABLE;
>  	}
>  
> -	if (unlikely((fpi_flags & FPI_TRYLOCK) && IS_ENABLED(CONFIG_PREEMPT_RT)
> -		     && (in_nmi() || in_hardirq()))) {
> +	if (unlikely((fpi_flags & FPI_TRYLOCK) && !can_spin_trylock())) {

[Severity: High]
This isn't a bug introduced by this patch, but since the goal is to prevent
unsafe lock acquisition, does this check need to be moved earlier in
__free_frozen_pages() in mm/page_alloc.c?

There are early returns earlier in __free_frozen_pages() that bypass this
safety check completely. For example, for large orders:

    if (!pcp_allowed_order(order)) {
        __free_pages_ok(page, order, fpi_flags);
        return;
    }

And for isolated pages:

        if (unlikely(is_migrate_isolate(migratetype))) {
            free_one_page(zone, page, pfn, order, fpi_flags);
            return;
        }

Both of these paths eventually reach free_one_page() (around line 1561), which
unconditionally calls spin_trylock_irqsave() if FPI_TRYLOCK is set:

    if (unlikely(fpi_flags & FPI_TRYLOCK)) {
        if (!spin_trylock_irqsave(&zone->lock, flags)) {
            add_page_to_zone_llist(zone, page, order);
            return;
        }
    }

Could this lead to memory corruption on UP systems or deadlocks on PREEMPT_RT
if a high-order or isolated page is freed in NMI context via
free_frozen_pages_nolock() with FPI_TRYLOCK?

>  		add_page_to_zone_llist(zone, page, order);
>  		return;
>  	}

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