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

[Harry Yoo]

On 7/13/26 11:31 PM, Brendan Jackman wrote:
> 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.

Ouch, do we allow alloc_pages() -> free_pages_nolock()?
Didn't notice.

>>> 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.

Just noting, it is quite niche because it requires UP, and tracing
something that can be called in NMI on UP is even nicher.

>> 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.

Hmm but alloc_order_allowed() would return true for !pcp_allowed_order()
when spinning is allowed, then pages can be freed via
free_pages_nolock()?

-- 
Cheers,
Harry / Hyeonggon

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature