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

[Brendan Jackman]

On Sat Jul 11, 2026 at 12:03 AM UTC, Andrew Morton wrote:
> On Fri, 10 Jul 2026 10:42:20 +0000 Brendan Jackman <jackmanb@xxxxxxxxxx> wrote:
>
>> As noted in can_spin_trylock(), using this is unsafe in this context.
>> commit 620b46ed6ae17 ("mm/page_alloc: return NULL early from
>> alloc_frozen_pages_nolock() in NMI on UP") fixed this on the alloc side
>> but missed the free side.
>> 
>> Reported-by: sashiko-bot@xxxxxxxxxx
>> Link: https://sashiko.dev/#/patchset/20260703-alloc-trylock-v5-0-c87b714e19d3@xxxxxxxxxx
>> Cc: stable@xxxxxxxxxxxxxxx
>> Fixes: d7242af86434 ("mm: Introduce alloc_frozen_pages_nolock()")
>
> Is this correct?  I'm not seeing anything in that commit which could
> have caused this?

Oh yeah I guess it should be:

Fixes: 8c57b687e8331 ("mm, bpf: Introduce free_pages_nolock()")

This is confusing coz we have:

A: commit d7242af86434 ("mm: Introduce alloc_frozen_pages_nolock()")
B: commit 8c57b687e8331 ("mm, bpf: Introduce free_pages_nolock()")
...
X: commit 620b46ed6ae17 ("mm/page_alloc: return NULL early from alloc_frozen_pages_nolock() in NMI on UP") 

X is marked as Fixing A, but it was an incomplete fix. I just copy
pasted the Fixes tag. But actually I'm now changing the free path that
was only introduced in B. 

>> --- 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())) {
>>  		add_page_to_zone_llist(zone, page, order);
>>  		return;
>>  	}
>
> It would be nice to include a description of the userspace impact.  I'm
> suspecting that's "none known", but some speculation on what might
> happen to someone is appropriate.

Ack. I think if you trigger this bug by accident it will probably crash
your machine in extremely confusing ways. If you can trigger it
deliberately from unpriv (depends on the rest of the host setup, e.g.
what tracing is being used) you can probably use it to get root/ring0.
Can mention this in the commit message.

> Also, please let's not combine a cc:stable bugfix with a minor macro
> renaming.  They're very different things and will take quite different
> paths into mainline and -stable kernels.

Ack. I assume by "combine" you mean put them in the same series - let
me know if I misunderstood that. Will separate them for v2.

> Also, Sashiko might have found yet more pre-existing issues:
> 	https://sashiko.dev/#/patchset/20260710-spin-trylock-followup-v1-0-affb5fe5ed00@xxxxxxxxxx

There are 2 cases here:

1: !pcp_allowed_order() -> This is forbidden by alloc_order_allowed(),
   quite a bad miss from Sashiko IMO. Usually I expect AI to do better
   at spotting this kind of thing, makes me wonder if I'm the dumb one
   here.

2. pageblock is isolated -> Yeah this looks broken to me.