munmap() vs unmap_mapping_range()

Scattered across the archs are 3 basic forms of tlb_{start,end}_vma(). Provide two new MMU_GATHER_knobs to enumerate them and remove the per arch tlb_{start,end}_vma() implementations.

- MMU_GATHER_NO_FLUSH_CACHE indicates the arch has flush_cache_range() but does *NOT* want to call it for each VMA.

- MMU_GATHER_MERGE_VMAS indicates the arch wants to merge the invalidate across multiple VMAs if possible.

With these it is possible to capture the three forms:

1) empty stubs;
select MMU_GATHER_NO_FLUSH_CACHE and MMU_GATHER_MERGE_VMAS

2) start: flush_cache_range(), end: empty;
select MMU_GATHER_MERGE_VMAS

3) start: flush_cache_range(), end: flush_tlb_range(); default

Obviously, if the architecture does not have flush_cache_range() then it also doesn't need to select MMU_GATHER_NO_FLUSH_CACHE.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/Kconfig                     |    7 +++++++
 arch/csky/include/asm/tlb.h      |   13 -------------
 arch/loongarch/Kconfig           |    1 +
 arch/loongarch/include/asm/tlb.h |   10 ----------
 arch/powerpc/Kconfig             |    1 +
 arch/powerpc/include/asm/tlb.h   |    2 --
 arch/s390/Kconfig                |    1 +
 arch/s390/include/asm/tlb.h      |    3 ---
 arch/sparc/Kconfig               |    2 ++
 arch/sparc/include/asm/tlb_64.h  |    2 --
 arch/x86/Kconfig                 |    1 +
 arch/x86/include/asm/tlb.h       |    3 ---
 include/asm-generic/tlb.h        |   21 +++++++++++++++++++--
 13 files changed, 32 insertions(+), 35 deletions(-)

The previous patch removed the tlb_flush_end() implementation which used tlb_flush_range(). This means:

- csky did double invalidates, a range invalidate per vma and a full invalidate at the end

- csky actually has range invalidates and as such the generic tlb_flush implementation is more efficient for it.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/csky/include/asm/tlb.h |    2 --
 1 file changed, 2 deletions(-)

Now that architectures are no longer allowed to override tlb_{start,end}_vma() re-arrange code so that there is only one implementation for each of these functions.

This much simplifies trying to figure out what they actually do.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/asm-generic/tlb.h |   15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

include/asm-generic/tlb.h +2 -12

@@ -346,8 +346,8 @@ static inline void __tlb_reset_range(str
 
 #ifdef CONFIG_MMU_GATHER_NO_RANGE
 
-#if defined(tlb_flush) || defined(tlb_start_vma) || defined(tlb_end_vma)
-#error MMU_GATHER_NO_RANGE relies on default tlb_flush(), tlb_start_vma() and tlb_end_vma()
+#if defined(tlb_flush)
+#error MMU_GATHER_NO_RANGE relies on default tlb_flush()
 #endif
 
 /*

@@ -367,17 +367,10 @@ static inline void tlb_flush(struct mmu_
 static inline void
 tlb_update_vma_flags(struct mmu_gather *tlb, struct vm_area_struct *vma) { }
 
-#define tlb_end_vma tlb_end_vma
-static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma) { }
-
 #else /* CONFIG_MMU_GATHER_NO_RANGE */
 
 #ifndef tlb_flush
 
-#if defined(tlb_start_vma) || defined(tlb_end_vma)
-#error Default tlb_flush() relies on default tlb_start_vma() and tlb_end_vma()
-#endif
-
 /*
  * When an architecture does not provide its own tlb_flush() implementation
  * but does have a reasonably efficient flush_vma_range() implementation

@@ -498,7 +491,6 @@ static inline unsigned long tlb_get_unma
  * case where we're doing a full MM flush.  When we're doing a munmap,
  * the vmas are adjusted to only cover the region to be torn down.
  */
-#ifndef tlb_start_vma
 static inline void tlb_start_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 {
 	if (tlb->fullmm)

@@ -509,9 +501,7 @@ static inline void tlb_start_vma(struct
 	flush_cache_range(vma, vma->vm_start, vma->vm_end);
 #endif
 }
-#endif
 
-#ifndef tlb_end_vma
 static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 {
 	if (tlb->fullmm || IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS))

Jann reported a race between munmap() and unmap_mapping_range(), where unmap_mapping_range() will no-op once unmap_vmas() has unlinked the VMA; however munmap() will not yet have invalidated the TLBs.

Therefore unmap_mapping_range() will complete while there are still (stale) TLB entries for the specified range.

Mitigate this by force flushing TLBs for VM_PFNMAP ranges.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/asm-generic/tlb.h |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

include/asm-generic/tlb.h +9 -15

@@ -303,6 +303,7 @@ struct mmu_gather {
 	 */
 	unsigned int		vma_exec : 1;
 	unsigned int		vma_huge : 1;
+	unsigned int		vma_pfn  : 1;
 
 	unsigned int		batch_count;
 

@@ -373,7 +374,6 @@ tlb_update_vma_flags(struct mmu_gather *
 #else /* CONFIG_MMU_GATHER_NO_RANGE */
 
 #ifndef tlb_flush
-
 /*
  * When an architecture does not provide its own tlb_flush() implementation
  * but does have a reasonably efficient flush_vma_range() implementation

@@ -393,6 +393,9 @@ static inline void tlb_flush(struct mmu_
 		flush_tlb_range(&vma, tlb->start, tlb->end);
 	}
 }
+#endif
+
+#endif /* CONFIG_MMU_GATHER_NO_RANGE */
 
 static inline void
 tlb_update_vma_flags(struct mmu_gather *tlb, struct vm_area_struct *vma)

@@ -410,17 +413,9 @@ tlb_update_vma_flags(struct mmu_gather *
 	 */
 	tlb->vma_huge = is_vm_hugetlb_page(vma);
 	tlb->vma_exec = !!(vma->vm_flags & VM_EXEC);
+	tlb->vma_pfn  = !!(vma->vm_flags & VM_PFNMAP);
 }
 
-#else
-
-static inline void
-tlb_update_vma_flags(struct mmu_gather *tlb, struct vm_area_struct *vma) { }
-
-#endif
-
-#endif /* CONFIG_MMU_GATHER_NO_RANGE */
-
 static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb)
 {
 	/*

@@ -507,16 +502,22 @@ static inline void tlb_start_vma(struct
 
 static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 {
-	if (tlb->fullmm || IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS))
+	if (tlb->fullmm)
 		return;
 
 	/*
-	 * Do a TLB flush and reset the range at VMA boundaries; this avoids
-	 * the ranges growing with the unused space between consecutive VMAs,
-	 * but also the mmu_gather::vma_* flags from tlb_start_vma() rely on
-	 * this.
+	 * VM_PFNMAP is more fragile because the core mm will not track the
+	 * page mapcount -- there might not be page-frames for these PFNs after
+	 * all. Force flush TLBs for such ranges to avoid munmap() vs

Quoted 22 lines · Peter Zijlstra

(quoted diff) +8 -6

@@ -507,16 +502,22 @@ static inline void tlb_start_vma(struct
 
 static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 {
-	if (tlb->fullmm || IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS))
+	if (tlb->fullmm)
 		return;
 
 	/*
-	 * Do a TLB flush and reset the range at VMA boundaries; this avoids
-	 * the ranges growing with the unused space between consecutive VMAs,
-	 * but also the mmu_gather::vma_* flags from tlb_start_vma() rely on
-	 * this.
+	 * VM_PFNMAP is more fragile because the core mm will not track the
+	 * page mapcount -- there might not be page-frames for these PFNs after
+	 * all. Force flush TLBs for such ranges to avoid munmap() vs
+	 * unmap_mapping_range() races.
 	 */
-	tlb_flush_mmu_tlbonly(tlb);
+	if (tlb->vma_pfn || !IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS)) {
+		/*
+		 * Do a TLB flush and reset the range at VMA boundaries; this avoids

Review static inline void tlb_start_vma(struct · line 520

Quoted 4 lines

(quoted diff) +4 -0

+		 * the ranges growing with the unused space between consecutive VMAs.
+		 */
+		tlb_flush_mmu_tlbonly(tlb);
+	}

We already have the vma here, so I'm not sure how much the new 'vma_pfn' field really buys us over checking the 'vm_flags', but perhaps that's cleanup for another day.

Jann reported a race between munmap() and unmap_mapping_range(), where unmap_mapping_range() will no-op once unmap_vmas() has unlinked the VMA; however munmap() will not yet have invalidated the TLBs.

Therefore unmap_mapping_range() will complete while there are still (stale) TLB entries for the specified range.

Mitigate this by force flushing TLBs for VM_PFNMAP ranges.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/asm-generic/tlb.h |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

include/asm-generic/tlb.h +4 -1

@@ -303,6 +303,7 @@ struct mmu_gather {
         */
        unsigned int            vma_exec : 1;
        unsigned int            vma_huge : 1;
+       unsigned int            vma_pfn  : 1;
 
        unsigned int            batch_count;
 

@@ -373,7 +374,6 @@ tlb_update_vma_flags(struct mmu_gather *
 #else /* CONFIG_MMU_GATHER_NO_RANGE */
 
 #ifndef tlb_flush
-
 /*
  * When an architecture does not provide its own tlb_flush() implementation
  * but does have a reasonably efficient flush_vma_range() implementation

@@ -393,6 +393,9 @@ static inline void tlb_flush(struct mmu_
                flush_tlb_range(&vma, tlb->start, tlb->end);
        }
 }
+#endif
+
+#endif /* CONFIG_MMU_GATHER_NO_RANGE */
 
 static inline void
 tlb_update_vma_flags(struct mmu_gather *tlb, struct vm_area_struct *vma)

include/asm-generic/tlb.h +0 -9

 }
 
-#else
-
-static inline void
-tlb_update_vma_flags(struct mmu_gather *tlb, struct vm_area_struct *vma) { }
-
-#endif
-
-#endif /* CONFIG_MMU_GATHER_NO_RANGE */
-
 static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb)
 {
        /*

include/asm-generic/tlb.h +7 -1

         */
-       tlb_flush_mmu_tlbonly(tlb);
+       if (tlb->vma_pfn || !IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS)) {
+               /*
+                * Do a TLB flush and reset the range at VMA boundaries; this avoids
+                * the ranges growing with the unused space between consecutive VMAs.
+                */
+               tlb_flush_mmu_tlbonly(tlb);
+       }
 }
 
 /*
 

Quoted 40 lines · Peter Zijlstra

Jann reported a race between munmap() and unmap_mapping_range(), where unmap_mapping_range() will no-op once unmap_vmas() has unlinked the VMA; however munmap() will not yet have invalidated the TLBs.

Therefore unmap_mapping_range() will complete while there are still (stale) TLB entries for the specified range.

Mitigate this by force flushing TLBs for VM_PFNMAP ranges.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 include/asm-generic/tlb.h |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

include/asm-generic/tlb.h +4 -1

@@ -303,6 +303,7 @@ struct mmu_gather {
         */
        unsigned int            vma_exec : 1;
        unsigned int            vma_huge : 1;
+       unsigned int            vma_pfn  : 1;
 
        unsigned int            batch_count;
 

@@ -373,7 +374,6 @@ tlb_update_vma_flags(struct mmu_gather *
 #else /* CONFIG_MMU_GATHER_NO_RANGE */
 
 #ifndef tlb_flush
-
 /*
  * When an architecture does not provide its own tlb_flush() implementation
  * but does have a reasonably efficient flush_vma_range() implementation

@@ -393,6 +393,9 @@ static inline void tlb_flush(struct mmu_
                flush_tlb_range(&vma, tlb->start, tlb->end);
        }
 }
+#endif
+
+#endif /* CONFIG_MMU_GATHER_NO_RANGE */
 

include/asm-generic/tlb.h tlb_update_vma_flags(struct mmu_gather * · line 416

Quoted 7 lines

include/asm-generic/tlb.h +1 -0

 static inline void
 tlb_update_vma_flags(struct mmu_gather *tlb, struct vm_area_struct *vma)

@@ -410,17 +413,9 @@ tlb_update_vma_flags(struct mmu_gather *
         */
        tlb->vma_huge = is_vm_hugetlb_page(vma);
        tlb->vma_exec = !!(vma->vm_flags & VM_EXEC);
+       tlb->vma_pfn  = !!(vma->vm_flags & VM_PFNMAP);

We should probably handle VM_MIXEDMAP the same way as VM_PFNMAP here, I think? Conceptually I think the same issue can happen with device-owned pages that aren't managed by the kernel's page allocator, and for those, VM_MIXEDMAP is the same as VM_PFNMAP.

Quoted 16 lines

include/asm-generic/tlb.h +0 -9

 }
 
-#else
-
-static inline void
-tlb_update_vma_flags(struct mmu_gather *tlb, struct vm_area_struct *vma) { }
-
-#endif
-
-#endif /* CONFIG_MMU_GATHER_NO_RANGE */
-
 static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb)
 {
        /*

@@ -507,16 +502,22 @@ static inline void tlb_start_vma(struct
 

include/asm-generic/tlb.h static inline void tlb_start_vma(struct · line 506

Quoted 5 lines

include/asm-generic/tlb.h +1 -1

 static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 {
-       if (tlb->fullmm || IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS))
+       if (tlb->fullmm)
                return;

Is this correct, or would there still be a race between MM teardown (which sets ->fullmm, see exit_mmap()->tlb_gather_mmu_fullmm()) and unmap_mapping_range()? My understanding is that ->fullmm only guarantees a flush at tlb_finish_mmu(), but here we're trying to ensure a flush before unlink_file_vma().

include/asm-generic/tlb.h static inline void tlb_start_vma(struct · line 511

Quoted 9 lines

include/asm-generic/tlb.h +4 -4

        /*
-        * Do a TLB flush and reset the range at VMA boundaries; this avoids
-        * the ranges growing with the unused space between consecutive VMAs,
-        * but also the mmu_gather::vma_* flags from tlb_start_vma() rely on
-        * this.
+        * VM_PFNMAP is more fragile because the core mm will not track the
+        * page mapcount -- there might not be page-frames for these PFNs after
+        * all. Force flush TLBs for such ranges to avoid munmap() vs
+        * unmap_mapping_range() races.

Maybe add: "We do *not* guarantee that after munmap() has passed

through tlb_end_vma(), there are no more stale TLB entries for this VMA; there could be a parallel PTE-zapping operation that has zapped PTEs before we looked at them but hasn't done the corresponding TLB flush yet. However, such a parallel zap can't be done through the mm_struct (we've unlinked the VMA), so it would have to be done under the ->i_mmap_sem in read mode, which we synchronize against in unlink_file_vma()."

I'm not convinced it's particularly nice to do a flush in tlb_end_vma() when we can't make guarantees about the TLB state wrt parallel invalidations, and when we only really care about having a flush between unmap_vmas() and free_pgtables(), but I guess it works?

Quoted 14 lines · Jann Horn

include/asm-generic/tlb.h static inline void tlb_start_vma(struct · line 506

Quoted 7 lines · Peter Zijlstra

include/asm-generic/tlb.h +1 -1

@@ -507,16 +502,22 @@ static inline void tlb_start_vma(struct
 
 static inline void tlb_end_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
 {
-       if (tlb->fullmm || IS_ENABLED(CONFIG_MMU_GATHER_MERGE_VMAS))
+       if (tlb->fullmm)
                return;

Is this correct, or would there still be a race between MM teardown (which sets ->fullmm, see exit_mmap()->tlb_gather_mmu_fullmm()) and unmap_mapping_range()? My understanding is that ->fullmm only guarantees a flush at tlb_finish_mmu(), but here we're trying to ensure a flush before unlink_file_vma().

fullmm is when the last user of the mm goes away, there should not be

any races on the address space at that time. Also see the comment with tlb_gather_mmu_fullmm() and its users.

Subject: mmu_gather: Force TLB-flush VM_PFNMAP|VM_MIXEDMAP vmas
From: Peter Zijlstra <[email protected]>
Date: Thu Jul 7 11:51:16 CEST 2022

Jann reported a race between munmap() and unmap_mapping_range(), where unmap_mapping_range() will no-op once unmap_vmas() has unlinked the VMA; however munmap() will not yet have invalidated the TLBs.

Therefore unmap_mapping_range() will complete while there are still (stale) TLB entries for the specified range.

Mitigate this by force flushing TLBs for VM_PFNMAP ranges.

Reported-by Jann Horn <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

Hi,

These 4 patches implement the suggestion by Linus.

The first patch nukes all architecture implementations of tlb_{start,end}_vma() by adding two additional MMU_GATHER config knobs.

DaveM, please clarify the sparc64 case, it seems a little odd to have flush_cache_range() but to explicitly not call it at unmap time. It would be nice to remove this one special case.

The second patch isn't strictly needed for this series but since I was looking at all this, I figured C-SKY didn't make a whole lot of sense. Guo, please have a look.

The third patch cleans up the #ifdeffery and provides a single tlb_{start,end}_vma() implementation for everyone.

The fourth, and final, patch does the force TLB-flush on VM_PFNMAP thing.

I've had the first three patches exposed to the robots and they've (so far) not reported build (or other) fail.

What are we doing with these patches? Andrew will you pick them up, or will I smuggle them in x86/mm or something ?

I'm picking up other random things right now anyway as people have emailed me, so I'll just do that with this series too.