Re: [PATCH] mm/sparse: Fix race on mem_section->usage in pfn walkers

[Muchun Song <muchun.song@linux.dev>]

> On Apr 21, 2026, at 19:21, David Hildenbrand (Arm) <david@kernel.org> wrote:
> 
> On 4/15/26 11:20, Muchun Song wrote:
>> 
>> 
>>> On Apr 15, 2026, at 16:04, David Hildenbrand (Arm) <david@kernel.org> wrote:
>>> 
>>> On 4/15/26 04:23, Muchun Song wrote:
>>>> When memory is hot-removed, section_deactivate() can tear down
>>>> mem_section->usage while concurrent pfn walkers still inspect the
>>>> subsection map via pfn_section_valid() or pfn_section_first_valid().
>>> 
>>> I'll note that it's all racy either way: someone checking pfn_valid() /
>>> pfn_to_online_page() can race with concurrent unplug.
>> 
>> Agree. When I first saw the commit message for 5ec8e8ea8b77, I was curious
>> because the goal of this commit was to fix an access issue with ms->usage.
>> Looking at the race diagram, I realized that while this only addresses the
>> ->usage access, subsequent accesses to struct page will still be problematic.
>> It's just that the former issue happened to be triggered first in this specific
>> commit.
>> 
>>> 
>>> We've known that for years; it's hard to fix; it never ever triggers :)
>> 
>> Glad to know my analysis wasn't off! It seems I've just stumbled upon a
>> 'well-known secret' within the community. :)
> 
> Heh, yes.
> 
>> 
>>> 
>>> So is this really worth it, when we should in fact, work on protecting
>>> the users of pfn_valid() / pfn_to_online_page() with rcu or similar?
>> 
>> I am not sure if it is worth fixing, especially since I just realized the
>> community has been aware of this issue for many years. If we do decide to
>> fix it, I think the most straightforward approach would be to protect it
>> using RCU, something like:
>> 
>> 	# the user side of pfn_to_online_page():
>> 	rcu_read_lock(); 
>> 	page = pfn_to_online_page();
>> 	if (!get_page_unless_zero(page))
>>    		goto out_unlock;
>> 	rcu_read_unlock();
> 
> 
> Right, but we'd have to protect against the sections being marked as
> offline as well here, though. So against a pure concurrent offline_pages().

Right.

> 
> If you're looking for a project, this is really one worth doing! :)
> 

Initially, I wasn't sure if this issue was worth fixing, but it seems
we are moving in the right direction. I'll give it some more thought
in my spare time.

> 
> pfn_to_online_page() is more in need for protection than pfn_valid() I
> think.

Agree.

Muchun,
Thanks.

> 
> -- 
> Cheers,
> 
> David