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

[Brendan Jackman]

On Mon Jul 13, 2026 at 4:15 PM UTC, Harry Yoo wrote:
>
>
> 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.

We don't explicitly disallow that but I'd say it's "forbidden by
default"...

But I think that's unrelated? It doesn't mean you can't use
free_pages_nolock() from NMI, right? (Would be weird to alloc from
another context and then free in NMI, but I don't think it's "forbidden
by default" in the way that using unmatched APIs is).

>>>> 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()?

As noted above I think that's forbidden. Now I think about it, I'll
include a patch to document this.