Re: [PATCH v2] mm: memory: fix truncation removing mapped folios

["David Hildenbrand (Arm)" <david@kernel.org>]

On 10/16/26 03:47, Baolin Wang wrote:
> 
> 
> On 10/16/26 3:00 AM, David Hildenbrand (Arm) wrote:
>> On 10/15/26 08:41, Baolin Wang wrote:
>>>
>>>
>>>
>>> Yes, I've considered that. However, implicit memory barriers give me a headache,
>>> so I ended up going with explicit memory barriers (suggested by Pedro).
>>>
>>> If you don't like memory barriers, let me re-check whether the implicit memory
>>> barriers here can ensure race safety with sync_with_folio_pmd_zap(). A
>>> simplified logic is as follows:
>>>
>>> CPU 0 (truncation)                        CPU 1 (zapping)
>>> down_read() ->Acquire A1.                pte spin_lock() ->Acquire A4
>>>                                           clear mapcount
>>>
>>>                                           pmd spin_lock() ->Acquire A5
>>>                                           clear pmd
>>>                                           pmd spin_unlock() ->Release R5
>>>
>>> if (pmd_none()) { //read pmd
>>>                                           pte spin_unlock() ->Release R4
>>>      pmd spin_lock() ->Acquire A2
>>>      pmd spin_unlock() ->Release R2
>>> }
>>> up_read() -> Release R1
>>>
>>> spin_lock(y) ->Acquire A3
>>> folio_mapped() // read mapcount
>>> spin_unlock(y) ->Release R3
>>>
>>> CPU 0 constraints:
>>> 1. R2 is a release, so "read pmd" completes before R2.
>>> 2. A2 completes before A3 (ACQUIRE vs ACQUIRE).
>>> 3. "read mapcount" completes after A3, hence after A2.
>>>
>>> CPU 1 constraints:
>>> 1. R5 is a release. Both "clear mapcount" and "clear pmd" complete before R5.
>>>
>>> According to above constraints, if CPU0 sees pmd_none() caused by CPU1's clear
>>> pmd, and CPU0 then acquires the same pmd lock released by CPU1 at R5, then
>>> CPU1's clear mapcount happens before CPU0's later folio_mapped().
>>>
>>> Therefore folio_mapped() is guaranteed not to read the old mapcount in this
>>> case. So I think sync_with_folio_pmd_zap() can work, but needs a small change to
>>> avoid the double pmd_none() check.
>>
>>
>> I'd be okay with some (follow-up?) memory barrier handling inside
>> sync_with_folio_pmd_zap(), for example, based on the folio size and with proper
>> documentation why we need the PT lock in one case and why the memory barrier is
>> ok in the other case.
> 
> I'm afraid that we can't fit all the memory barriers into
> sync_with_folio_pmd_zap(). As the current patch does, we need memory barriers on
> both the writer and reader sides.

Obviously only the reader-side one would go in there.

[...]

>>>                  if (pmd_none(*pmd)) {
>>> +                       if (details && details->single_folio)
>>> +                               sync_with_folio_pmd_zap(tlb->mm, pmd);
>>>                          addr = next;
>>>                          continue;
>>>                  }
>>
>> We do have another sync_with_folio_pmd_zap() user in page_vma_mapped_walk(),
>> does that one need care as well?
> 
> Good point. But I don't want to add page_vma_mapped_walk() changes into this fix
> patch. I need to analyze all callers of page_vma_mapped_walk() and the potential
> races. I'll look into whether it should be changed in a follow-up patch.

Yes, please have a look. I am 99.9% sure that we face the exact same issue there
as well.

-- 
Cheers,

David