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

Muchun Song
2026-04-21T12:56:09Z

> 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