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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 10/27/26 6:55 PM, David Hildenbrand (Arm) wrote:
> On 10/27/26 11:35, Baolin Wang wrote:
>>
>>
>> On 10/27/26 5:33 PM, David Hildenbrand (Arm) wrote:
>>> On 10/27/26 10:03, Baolin Wang wrote:
>>>>
>>>>
>>>>
>>>> No, keeping two pmd_none() checks cannot fix the race, as I replied before. [1]
>>>>
>>>
>>> Best to wait to submit new versions if there are open discussions? Because now
>>> we have the same discussion in two places.
>>
>> Sure.
>>
>>>
>>> I think you're right about the race, the issue is that we check for
>>> pmd_is_huge(), not pmd_present() which confused me.
>>>
>>> Clearest to me would be to not drop the second pmd_none() check but move it
>>> where it belongs: if splitting zapped the PMD.
>>>
>>> @@ -2101,13 +2104,15 @@ static inline unsigned long zap_pmd_range(struct
>>> mmu_gather *tlb,
>>>                                   addr = next;
>>>                                   continue;
>>>                           }
>>> +                       /* We already synchronized with concurrent zapping. */
>>> +                       if (pmd_none(*pmd)) {
>>> +                               addr = next;
>>> +                               continue;
>>> +                       }
>>>                           /* fall through */
>>> -               } else if (details && details->single_folio &&
>>> -                          folio_test_pmd_mappable(details->single_folio) &&
>>> -                          next - addr == HPAGE_PMD_SIZE && pmd_none(*pmd)) {
>>> -                       sync_with_folio_pmd_zap(tlb->mm, pmd);
>>> -               }
>>> -               if (pmd_none(*pmd)) {
>>> +               } else if (pmd_none(*pmd)) {
>>> +                       if (details && details->single_folio)
>>> +                               sync_with_folio_pmd_zap(tlb->mm, pmd);
>>>                           addr = next;
>>>                           continue;
>>>                   }
>>
>> IMHO, I'm not a big fan of adding two pmd_none() checks just to avoid an
>> uncommon concurrent sync.
> 
> It's what we already had. We had to: one for handling pmd_none() directly and
> one for handling zapping while splitting (file THPs).
> 
> We just managed to let the second also handle the final piece of the former.
> 
> Anyhow, let's get this fixed.
> 
> If you think the code is clearer your way (where I disagree)

Fair enough. Though I have a different taste, I'd still like to make the 
maintainer happy. I'll send out v6 tomorrow following your suggestion.

But could you help check the following whole diff to avoid any 
unnecessary misunderstandings? (I don't want to give Andrew more trouble 
:)). Thanks.

diff --git a/mm/internal.h b/mm/internal.h
index b1b0f29d2216..ffb8eaec90e3 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -584,14 +584,16 @@ 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() may have cleared the PMD but not yet updated the folio
+ * mapcount, and zap_empty_pte_table() clears the PMD only after the rmaps
+ * for the table have been removed, but nothing orders the pmd_none() read
+ * against later mapcount reads. 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 e911e63ba1ed..e0fef107a022 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2036,6 +2036,15 @@ static unsigned long zap_pte_range(struct 
mmu_gather *tlb,
                 }
         } while (pte += nr, addr += PAGE_SIZE * nr, addr != end);

+       add_mm_rss_vec(mm, rss);
+       lazy_mmu_mode_disable();
+
+       /* Do the actual TLB flush before dropping ptl */
+       if (force_flush) {
+               tlb_flush_mmu_tlbonly(tlb);
+               tlb_flush_rmaps(tlb, vma);
+       }
+
         /*
          * Fast path: try to hold the pmd lock and unmap the PTE page.
          *
@@ -2043,18 +2052,12 @@ static unsigned long zap_pte_range(struct 
mmu_gather *tlb,
          * to hold the pmd lock failed, then we need to recheck all pte 
entries
          * to ensure they are still none, thereby preventing the pte 
entries
          * from being repopulated by another thread.
+        *
+        * Clear the PMD after the rmaps for the table have been 
removed, in case
+        * an incorrect mapcount is observed after unmapping the folio.
          */
         if (can_reclaim_pt && direct_reclaim && addr == end)
                 direct_reclaim = zap_empty_pte_table(mm, pmd, ptl, 
&pmdval);
-
-       add_mm_rss_vec(mm, rss);
-       lazy_mmu_mode_disable();
-
-       /* Do the actual TLB flush before dropping ptl */
-       if (force_flush) {
-               tlb_flush_mmu_tlbonly(tlb);
-               tlb_flush_rmaps(tlb, vma);
-       }
         pte_unmap_unlock(start_pte, ptl);

         /*
@@ -2101,13 +2104,15 @@ static inline unsigned long zap_pmd_range(struct 
mmu_gather *tlb,
                                 addr = next;
                                 continue;
                         }
+                       /* We already synchronized with concurrent 
zapping. */
+                       if (pmd_none(*pmd)) {
+                               addr = next;
+                               continue;
+                       }
                         /* fall through */
-               } else if (details && details->single_folio &&
-                          folio_test_pmd_mappable(details->single_folio) &&
-                          next - addr == HPAGE_PMD_SIZE && 
pmd_none(*pmd)) {
-                       sync_with_folio_pmd_zap(tlb->mm, pmd);
-               }
-               if (pmd_none(*pmd)) {
+               } else if (pmd_none(*pmd)) {
+                       if (details && details->single_folio)
+                               sync_with_folio_pmd_zap(tlb->mm, pmd);
                         addr = next;
                         continue;
                 }