Re: [PATCH] mm: Fix the race between collapse and PT_RECLAIM under per-vma lock

[David Hildenbrand <david@redhat.com>]

On 05.08.25 11:30, Qi Zheng wrote:
> 
> 
> On 8/5/25 4:56 PM, Baolin Wang wrote:
>>
>>
>> On 2025/8/5 16:17, Qi Zheng wrote:
>>> Hi Baolin,
>>>
>>> On 8/5/25 3:53 PM, Baolin Wang wrote:
>>>>
>>>>
>>>> On 2025/8/5 14:42, Qi Zheng wrote:
>>>>> Hi Barry,
>>>>>
>>>>> On 8/5/25 11:54 AM, Barry Song wrote:
>>>>>> From: Barry Song <v-songbaohua@oppo.com>
>>>>>>
>>>>>> The check_pmd_still_valid() call during collapse is currently only
>>>>>> protected by the mmap_lock in write mode, which was sufficient when
>>>>>> pt_reclaim always ran under mmap_lock in read mode. However, since
>>>>>> madvise_dontneed can now execute under a per-VMA lock, this assumption
>>>>>> is no longer valid. As a result, a race condition can occur between
>>>>>> collapse and PT_RECLAIM, potentially leading to a kernel panic.
>>>>>
>>>>> There is indeed a race condition here. And after applying this patch, I
>>>>> can no longer reproduce the problem locally (I was able to reproduce it
>>>>> stably locally last night).
>>>>>
>>>>> But I still can't figure out how this race condtion causes the
>>>>> following panic:
>>>>>
>>>>> exit_mmap
>>>>> --> mmap_read_lock()
>>>>>       unmap_vmas()
>>>>>       --> pte_offset_map_lock
>>>>>           --> rcu_read_lock()
>>>>>               check if the pmd entry is a PTE page
>>>>>               ptl = pte_lockptr(mm, &pmdval)  <-- ptl is NULL
>>>>>               spin_lock(ptl)                  <-- PANIC!!
>>>>>
>>>>> If this PTE page is freed by pt_reclaim (via RCU), then the ptl can
>>>>> not be NULL.
>>>>>
>>>>> The collapse holds mmap write lock, so it is impossible to be
>>>>> concurrent
>>>>> with exit_mmap().
>>>>>
>>>>> Confusing. :(
>>>>
>>>> IIUC, the issue is not caused by the concurrency between exit_mmap
>>>> and collapse, but rather by the concurrency between pt_reclaim and
>>>> collapse.
>>>>
>>>> Before this patch, khugepaged might incorrectly restore a PTE
>>>> pagetable that had already been freed.
>>>>
>>>> pt_reclaim has cleared the pmd entry and freed the PTE page table.
>>>> However, due to the race condition, check_pmd_still_valid() still
>>>> passes and continues to attempt the collapse:
>>>>
>>>> _pmd = pmdp_collapse_flush(vma, address, pmd); ---> returns a none
>>>> pmd entry (the original pmd entry has been cleared)
>>>>
>>>> pte = pte_offset_map_lock(mm, &_pmd, address, &pte_ptl); ---> returns
>>>> pte == NULL
>>>>
>>>> Then khugepaged will restore the old PTE pagetable with an invalid
>>>> pmd entry:
>>>>
>>>> pmd_populate(mm, pmd, pmd_pgtable(_pmd));
>>>>
>>>> So when the process exits and trys to free the mapping of the
>>>> process, traversing the invalid pmd table will lead to a crash.
>>>
>>> CPU0                         CPU1
>>> ====                         ====
>>>
>>> collapse
>>> --> pmd_populate(mm, pmd, pmd_pgtable(_pmd));
>>>       mmap_write_unlock
>>>                                exit_mmap
>>>                                --> hold mmap lock
>>>                                    __pte_offset_map_lock
>>>                                    --> pte = __pte_offset_map(pmd,
>>> addr, &pmdval);
>>>                                        if (unlikely(!pte))
>>>                                            return pte;   <-- will return
>>
>> __pte_offset_map() might not return NULL? Because the 'pmd_populate(mm,
>> pmd, pmd_pgtable(_pmd))' could populate a valid page (although the
>> '_pmd' entry is NONE), but it is not the original pagetable page.
> 
> CPU0                          CPU1
> ====                          ====
> 
> collapse
> --> check_pmd_still_valid
>                                 vma read lock
>                                 pt_reclaim clear the pmd entry and will
> free the PTE page (via RCU)
>                                 vma read unlock
> 
>       vma write lock
>       _pmd = pmdp_collapse_flush(vma, address, pmd) <-- pmd_none(_pmd)
>       pte = pte_offset_map_lock(mm, &_pmd, address, &pte_ptl); <-- pte is
> NULL
>       pmd_populate(mm, pmd, pmd_pgtable(_pmd)); <-- populate a valid page?
>       vma write unlock
> 
> The above is the concurrent scenario you mentioned, right?
> 
> What types of this 'valid page' could be? If __pte_offset_map() returns
> non-NULL, then it is a PTE page. Even if it is not the original one, it
> should not cause panic. Did I miss some key information? :(

Wasn't the original issue all about a NULL-pointer de-reference while 
*locking*?

Note that in that kernel config [1] we have CONFIG_DEBUG_SPINLOCK=y, so 
likely we will have ALLOC_SPLIT_PTLOCKS set.

[1] 
https://github.com/laifryiee/syzkaller_logs/blob/main/250803_193026___pte_offset_map_lock/.config

-- 
Cheers,

David / dhildenb