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

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

On 10/14/26 16:05, Pedro Falcato wrote:
> On Wed, Oct 14, 2026 at 03:25:33PM +0200, David Hildenbrand (Arm) wrote:
>> On 10/13/26 11:36, Baolin Wang wrote:
>>>
>>>
>>>
>>> I had considered this approach before, but it is not feasible. IIUC, the
>>> sync_with_folio_pmd_zap() is used to address the race with zap_huge_pmd(), and
>>> both the PMD clear and the folio mapcount decrement are performed under the PMD
>>> lock in zap_huge_pmd().
>>
>> Just to be clear, I am thinking about something like the following on the
>> zap_pmd_range() side:
>>
>> diff --git a/mm/internal.h b/mm/internal.h
>> index 38b1165212c94..a8820ad7e2f21 100644
>> --- a/mm/internal.h
>> +++ b/mm/internal.h
>> @@ -584,14 +584,15 @@ void free_pgtables(struct mmu_gather *tlb, struct
>> unmap_desc *desc);
>>  void pmd_install(struct mm_struct *mm, pmd_t *pmd, pgtable_t *pte);
>>
>>  /**
>> - * sync_with_folio_pmd_zap - sync with concurrent zapping of a folio PMD
>> + * sync_with_folio_pmd_zap - sync with concurrent zapping of folios
>>   * @mm: The mm_struct.
>>   * @pmdp: Pointer to the pmd that was found to be pmd_none().
>>   *
>>   * When we find a pmd_none() while unmapping a folio without holding the PTL,
>> - * zap_huge_pmd() may have cleared the PMD but not yet modified the folio to
>> - * indicate that it's unmapped. Skipping the PMD without synchronization could
>> - * make folio unmapping code assume that unmapping failed.
>> + * zap_huge_pmd() or zap_empty_pte_table() may have cleared the PMD but not
>> + * yet modified the mapped folios to indicate they are unmapped. Skipping the
>> + * PMD without synchronization could make folio unmapping code assume that
>> + * unmapping failed.
>>   *
>>   * Wait for concurrent zapping to complete by grabbing the PTL.
>>   */
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 8b0c2c735d3de..c585f5eb255c9 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -2139,9 +2139,7 @@ static inline unsigned long zap_pmd_range(struct
>> mmu_gather *tlb,
>>                                 continue;
>>                         }
>>                         /* fall through */
>> -               } else if (details && details->single_folio &&
>> -                          folio_test_pmd_mappable(details->single_folio) &&
>> -                          next - addr == HPAGE_PMD_SIZE && pmd_none(*pmd)) {
>> +               } else if (details && details->single_folio && pmd_none(*pmd)) {
>>                         sync_with_folio_pmd_zap(tlb->mm, pmd);
>>                 }
>>                 if (pmd_none(*pmd)) {
>>
>>>
>>> However, the race here is different: tlb_flush_rmaps() is under the PTE lock,
>>> while zap_empty_pte_table() is under the PMD lock, and reordering between the
>>> two functions is also possible. That is, the folio mapcount operation could
>>> execute after the acquire barrier (spin_lock()) of zap_empty_pte_table(),
>>> allowing a reader on CPU 0 to incorrectly observe that the PMD is none while
>>> mapcount > 0.
>>
>> I think we really just have to make sure to flush rmaps before zapping the PMD.
>> Then it's really just the same thing we had before with sync_with_folio_pmd_zap().
> 
> Hmm, could we viably replace sync_with_folio_pmd_zap() with the memory barriers
> instead? Seems lighter-weight and functionally equivalent?

For the ordinary PMD zap case it's really:

1) Clear PMD

2) Drop refcount+mapcount

So I don't think it's a direct replacement. If we had a proper way to invalidate
+ adjust + clear it could work. Like

1) Invalidate

2) Drop refcount+mapcount

3) Clear


But in general I think:

a) Likely all places where we have existing sync_with_folio_pmd_zap() have this
   problem (2).

b) I don't want us to throw around memory barriers that way. This all belongs
   into a helper (like sync_with_folio_pmd_zap()) which we only call when
   required (e.g., details && details->single_folio) and that properly documents
   all the nasty details.

c) Maybe there is some smart way to decide whether we need a memory barrier or a
   proper lock for now inside sync_with_folio_pmd_zap(). Based on the folio size
   should do (PMD vs. < PMD). But I assume grabbing the PMD lock briefly is not
   really a concern: if there would be something mapped we would have to lock
   it, and if it would be a PTE table we would lock that instead.

[insert rant about our PMD table locking]

-- 
Cheers,

David