TLB and PTE coherency during munmap

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

I've noticed that with xtensa arch when I added a test before returning to userspace checking that TLB contents agrees with page tables of the current mm. This check reliably fires with the LTP test mtest05 that maps, unmaps and accesses memory from multiple threads.

Is there anything wrong in my description, maybe something specific to my arch, or this issue really exists?

Quoted 23 lines · Max Filippov

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

Quoted 7 lines

I've noticed that with xtensa arch when I added a test before returning to userspace checking that TLB contents agrees with page tables of the current mm. This check reliably fires with the LTP test mtest05 that maps, unmaps and accesses memory from multiple threads.

Is there anything wrong in my description, maybe something specific to my arch, or this issue really exists?

Hi,

I've made similar checking function for MIPS (because qemu is my only choice and it simulates MIPS TLB) and ran my tests on mips-malta machine in qemu. With MIPS I can also see this issue. I hope I did it right, the patch at the bottom is for the reference. The test I run and the diagnostic output are as follows:

To me it looks like the cond_resched in the zap_pmd_range is the root cause of this issue (let alone SMP case for now). It was introduced in the commit

commit 97a894136f29802da19a15541de3c019e1ca147e

Author: Peter Zijlstra <[email protected]>
Date:   Tue May 24 17:12:04 2011 -0700

mm: Remove i_mmap_lock lockbreak

Peter, Kamezawa, other reviewers of that commit, could you please comment?

Quoted 49 lines · Max Filippov

Quoted 23 lines · Max Filippov

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

Quoted 7 lines

I've noticed that with xtensa arch when I added a test before returning to userspace checking that TLB contents agrees with page tables of the current mm. This check reliably fires with the LTP test mtest05 that maps, unmaps and accesses memory from multiple threads.

Is there anything wrong in my description, maybe something specific to my arch, or this issue really exists?

Hi,

I've made similar checking function for MIPS (because qemu is my only choice and it simulates MIPS TLB) and ran my tests on mips-malta machine in qemu. With MIPS I can also see this issue. I hope I did it right, the patch at the bottom is for the reference. The test I run and the diagnostic output are as follows:

To me it looks like the cond_resched in the zap_pmd_range is the root cause of this issue (let alone SMP case for now). It was introduced in the commit

commit 97a894136f29802da19a15541de3c019e1ca147e

Author: Peter Zijlstra <[email protected]>
Date:   Tue May 24 17:12:04 2011 -0700

mm: Remove i_mmap_lock lockbreak

Quoted 1 line

Peter, Kamezawa, other reviewers of that commit, could you please comment?

Are you all running UP systems? I suppose the preemptible muck invalidated the assumption that UP systems are 'easy'.

If you make tlb_fast_mode() return an unconditional false, does it all work again?

It seems tlb_fast_mode() only affects the page free batching and won't affect the TLB flush themselves unless ofcourse the batching runs out of space.

FWIW, prior to your commit d16dfc550f5326 "mm: mmu_gather rework" tlb_finish_mmu() right before the need_resced() which would have handled the current situation. My proposal - please see my earlier email in thread is to reuse the force_flush logic in zap_pte_range() to do this.

mm/memory.c +4 -0

@@ -384,6 +384,21 @@ void tlb_remove_table(struct mmu_gather *tlb, void *table)
 
 #endif /* CONFIG_HAVE_RCU_TABLE_FREE */
 
+static inline void cond_resched_tlb(struct mmu_gather *tlb)
+{
+#ifndef CONFIG_PREEMPT
+       /*

Quoted 16 lines · Peter Zijlstra

mm/memory.c +10 -0

@@ -384,6 +384,21 @@ void tlb_remove_table(struct mmu_gather *tlb, void *table)
 
 #endif /* CONFIG_HAVE_RCU_TABLE_FREE */
 
+static inline void cond_resched_tlb(struct mmu_gather *tlb)
+{
+#ifndef CONFIG_PREEMPT
+       /*
+        * For full preempt kernels we must do regular batching like
+        * SMP, see tlb_fast_mode(). For !PREEMPT we can 'cheat' and
+        * do a flush before our voluntary 'yield'.
+        */
+       if (need_resched()) {
+               tlb_flush_mmu(tlb);

mm/memory.c void tlb_remove_table(struct mmu_gather *tlb, void *table) · line 400

Quoted 4 lines

mm/memory.c +4 -0

+               cond_resched();
+       }
+#endif
+}

Does it matter that in the CONFIG_PREEMPT case, you no longer call cond_resched()? I guess we can just rely on the kernel full preemption to reschedule as needed.

What about something like this?

---
 include/asm-generic/tlb.h | 11 ++++++++++-
 mm/memory.c               | 17 ++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

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

index b1b1fa6..651b1cf 100644
@@ -116,6 +116,7 @@ struct mmu_gather {
 
 static inline int tlb_fast_mode(struct mmu_gather *tlb)
 {
+#ifndef CONFIG_PREEMPT
 #ifdef CONFIG_SMP
 	return tlb->fast_mode;
 #else

@@ -124,7 +125,15 @@ static inline int tlb_fast_mode(struct mmu_gather *tlb)
 	 * and page free order so much..
 	 */
 	return 1;
-#endif

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

 void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm, bool fullmm);

mm/memory.c +0 -0

index 6dc1882..e915af2 100644
@@ -384,6 +384,21 @@ void tlb_remove_table(struct mmu_gather *tlb, void *table)
 
 #endif /* CONFIG_HAVE_RCU_TABLE_FREE */

mm/memory.c +6 -0

+		tlb_flush_mmu(tlb);
+		cond_resched();
+	}
+#endif
+}
+
 /*
  * If a p?d_bad entry is found while walking page tables, report
  * the error, before resetting entry to p?d_none.  Usually (but

Quoted 29 lines · Peter Zijlstra

What about something like this?

---
 include/asm-generic/tlb.h | 11 ++++++++++-
 mm/memory.c               | 17 ++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

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

index b1b1fa6..651b1cf 100644
@@ -116,6 +116,7 @@ struct mmu_gather {
 
 static inline int tlb_fast_mode(struct mmu_gather *tlb)
 {
+#ifndef CONFIG_PREEMPT
 #ifdef CONFIG_SMP
 	return tlb->fast_mode;
 #else

@@ -124,7 +125,15 @@ static inline int tlb_fast_mode(struct mmu_gather *tlb)
 	 * and page free order so much..
 	 */
 	return 1;
-#endif
+#endif /* CONFIG_SMP */
+#else  /* CONFIG_PREEMPT */
+	/*
+	 * Since mmu_gather is preemptible, preemptible kernels are like SMP
+	 * kernels, we must batch to make sure we invalidate TLBs before we

include/asm-generic/tlb.h static inline int tlb_fast_mode(struct mmu_gather *tlb) · line 137

Quoted 5 lines

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

+	 * free the pages.
+	 */
+	return 0;
+#endif /* CONFIG_PREEMPT */
 }

So this adds the page batching logic to small/simpler UP systems - but it's necessary evil :-(

Quoted 9 lines

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

 void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm, bool fullmm);

mm/memory.c +0 -0

index 6dc1882..e915af2 100644
@@ -384,6 +384,21 @@ void tlb_remove_table(struct mmu_gather *tlb, void *table)
 
 #endif /* CONFIG_HAVE_RCU_TABLE_FREE */
 

mm/memory.c void tlb_remove_table(struct mmu_gather *tlb, void *table) · line 395

Quoted 9 lines

mm/memory.c +9 -0

+static inline void cond_resched_tlb(struct mmu_gather *tlb)
+{
+#ifndef CONFIG_PREEMPT
+	/*
+	 * For full preempt kernels we must do regular batching like
+	 * SMP, see tlb_fast_mode(). For !PREEMPT we can 'cheat' and
+	 * do a flush before our voluntary 'yield'.
+	 */
+	if (need_resched()) {

This is really neat: w/o this check, a @fullmm flush (exit/execve) would have suffered multiple full TLB flushes in the loop, now you do that only if a scheduling was needed - meaning only in the case when we have the potential race condition which Max was seeing. Cool !

Quoted 10 lines

mm/memory.c +6 -0

+		tlb_flush_mmu(tlb);
+		cond_resched();
+	}
+#endif
+}
+
 /*
  * If a p?d_bad entry is found while walking page tables, report
  * the error, before resetting entry to p?d_none.  Usually (but

mm/memory.c static inline unsigned long zap_pmd_range(struct mmu_gather *tlb, · line 1285

Quoted 8 lines

mm/memory.c +1 -1

@@ -1264,7 +1279,7 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
 			goto next;
 		next = zap_pte_range(tlb, vma, pmd, addr, next, details);
 next:
-		cond_resched();
+		cond_resched_tlb(tlb);
 	} while (pmd++, addr = next, addr != end);
 
 	return addr;

BTW, since we are on the topic, it seems that we are missing tlb_fast_mode() in one spot - unless it is tied to rcu table free stuff.

-------------->
From: Vineet Gupta <[email protected]>
Date: Thu, 30 May 2013 10:25:30 +0530
Subject: [PATCH] mm: tlb_fast_mode check missing in tlb_finish_mmu()

Signed-off-by Vineet Gupta <[email protected]>

---
 mm/memory.c |    3 +++
 1 file changed, 3 insertions(+)

mm/memory.c +0 -0

index d9d5fd9..569ffe1 100644
@@ -269,6 +269,9 @@ void tlb_finish_mmu(struct mmu_gather *tlb, unsigned long

start, unsigned long e

     /* keep the page table cache within bounds */
     check_pgt_cache();

Quoted 24 lines · Vineet Gupta

On 05/29/2013 11:21 PM, Peter Zijlstra wrote:

BTW, since we are on the topic, it seems that we are missing tlb_fast_mode() in one spot - unless it is tied to rcu table free stuff.

-------------->
From: Vineet Gupta <[email protected]>
Date: Thu, 30 May 2013 10:25:30 +0530
Subject: [PATCH] mm: tlb_fast_mode check missing in tlb_finish_mmu()

Signed-off-by Vineet Gupta <[email protected]>

---
 mm/memory.c |    3 +++
 1 file changed, 3 insertions(+)

mm/memory.c +0 -0

index d9d5fd9..569ffe1 100644
@@ -269,6 +269,9 @@ void tlb_finish_mmu(struct mmu_gather *tlb, unsigned long

start, unsigned long e

     /* keep the page table cache within bounds */
     check_pgt_cache();

mm/memory.c

Quoted 6 lines

mm/memory.c +3 -0

+    if (tlb_fast_mode(tlb))
+        return;
+
     for (batch = tlb->local.next; batch; batch = next) {
         next = batch->next;
         free_pages((unsigned long)batch, 0);

Yes I think that is possible. It would shrink the code a little when fast_mode was unconditionally 1 -- ie. simple UP ;-).

Quoted 25 lines · Peter Zijlstra

Quoted 24 lines · Vineet Gupta

On 05/29/2013 11:21 PM, Peter Zijlstra wrote:

BTW, since we are on the topic, it seems that we are missing tlb_fast_mode() in one spot - unless it is tied to rcu table free stuff.

-------------->
From: Vineet Gupta <[email protected]>
Date: Thu, 30 May 2013 10:25:30 +0530
Subject: [PATCH] mm: tlb_fast_mode check missing in tlb_finish_mmu()

Signed-off-by Vineet Gupta <[email protected]>

---
 mm/memory.c |    3 +++
 1 file changed, 3 insertions(+)

mm/memory.c +0 -0

index d9d5fd9..569ffe1 100644
@@ -269,6 +269,9 @@ void tlb_finish_mmu(struct mmu_gather *tlb, unsigned long

start, unsigned long e

     /* keep the page table cache within bounds */
     check_pgt_cache();

Quoted 8 lines

mm/memory.c

Quoted 6 lines

mm/memory.c +3 -0

+    if (tlb_fast_mode(tlb))
+        return;
+
     for (batch = tlb->local.next; batch; batch = next) {
         next = batch->next;
         free_pages((unsigned long)batch, 0);

Yes I think that is possible. It would shrink the code a little when fast_mode was unconditionally 1 -- ie. simple UP ;-).

Exactly ! Can you please revert with a Reviewed-by/Acked-by so I can formally send it over to linux-mm list.

What about something like this?

---
 include/asm-generic/tlb.h | 11 ++++++++++-
 mm/memory.c               | 17 ++++++++++++++++-
 2 files changed, 26 insertions(+), 2 deletions(-)

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

index b1b1fa6..651b1cf 100644
@@ -116,6 +116,7 @@ struct mmu_gather {
 
 static inline int tlb_fast_mode(struct mmu_gather *tlb)
 {
+#ifndef CONFIG_PREEMPT
 #ifdef CONFIG_SMP
        return tlb->fast_mode;
 #else

@@ -124,7 +125,15 @@ static inline int tlb_fast_mode(struct mmu_gather *tlb)
         * and page free order so much..
         */
        return 1;
-#endif
+#endif /* CONFIG_SMP */
+#else  /* CONFIG_PREEMPT */
+       /*
+        * Since mmu_gather is preemptible, preemptible kernels are like SMP
+        * kernels, we must batch to make sure we invalidate TLBs before we
+        * free the pages.
+        */
+       return 0;
+#endif /* CONFIG_PREEMPT */
 }
 
 void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm, bool fullmm);

mm/memory.c +16 -1

index 6dc1882..e915af2 100644
@@ -384,6 +384,21 @@ void tlb_remove_table(struct mmu_gather *tlb, void *table)
 
 #endif /* CONFIG_HAVE_RCU_TABLE_FREE */
 
+static inline void cond_resched_tlb(struct mmu_gather *tlb)
+{
+#ifndef CONFIG_PREEMPT
+       /*
+        * For full preempt kernels we must do regular batching like
+        * SMP, see tlb_fast_mode(). For !PREEMPT we can 'cheat' and
+        * do a flush before our voluntary 'yield'.
+        */
+       if (need_resched()) {
+               tlb_flush_mmu(tlb);
+               cond_resched();
+       }
+#endif
+}
+
 /*
  * If a p?d_bad entry is found while walking page tables, report
  * the error, before resetting entry to p?d_none.  Usually (but

@@ -1264,7 +1279,7 @@ static inline unsigned long zap_pmd_range(struct mmu_gather *tlb,
                        goto next;
                next = zap_pte_range(tlb, vma, pmd, addr, next, details);
 next:
-               cond_resched();
+               cond_resched_tlb(tlb);
        } while (pmd++, addr = next, addr != end);
 
        return addr;

Hi Peter,

Quoted 1 line · Peter Zijlstra

What about something like this?

With that patch I still get mtest05 firing my TLB/PTE incoherency check in the UP PREEMPT_VOLUNTARY configuration. This happens after zap_pte_range completion in the end of unmap_region because of rescheduling called in the following call chain:

unmap_region
free_pgtables
unlink_anon_vmas
lock_anon_vma_root
down_write
might_sleep
might_resched
_cond_resched

Hi Peter,

Quoted 1 line · Peter Zijlstra

What about something like this?

With that patch I still get mtest05 firing my TLB/PTE incoherency check in the UP PREEMPT_VOLUNTARY configuration. This happens after zap_pte_range completion in the end of unmap_region because of rescheduling called in the following call chain:

Quoted 9 lines · Max Filippov

Hi Peter,

Quoted 1 line · Peter Zijlstra

What about something like this?

With that patch I still get mtest05 firing my TLB/PTE incoherency check in the UP PREEMPT_VOLUNTARY configuration. This happens after zap_pte_range completion in the end of unmap_region because of rescheduling called in the following call chain:

OK, so there two options; completely kill off fast-mode or something like the below where we add magic to the scheduler :/

I'm aware people might object to something like the below -- but since its a possibility I thought we ought to at least mention it.

For those new to the thread; the problem is that since the introduction of preemptible mmu_gather the traditional UP fast-mode is broken. Fast-mode is where we free the pages first and flush TLBs later. This is not a problem if there's no concurrency, but obviously if you can preempt there now is.

I think I prefer completely killing off fast-mode esp. since UP seems to go the way of the Dodo and it does away with an exception in the mmu_gather code.

Anyway; opinions? Linus, Thomas, Ingo?

Quoted 23 lines · Peter Zijlstra

Quoted 9 lines · Max Filippov

Hi Peter,

Quoted 1 line · Peter Zijlstra

What about something like this?

With that patch I still get mtest05 firing my TLB/PTE incoherency check in the UP PREEMPT_VOLUNTARY configuration. This happens after zap_pte_range completion in the end of unmap_region because of rescheduling called in the following call chain:

OK, so there two options; completely kill off fast-mode or something like the below where we add magic to the scheduler :/

I'm aware people might object to something like the below -- but since its a possibility I thought we ought to at least mention it.

For those new to the thread; the problem is that since the introduction of preemptible mmu_gather the traditional UP fast-mode is broken. Fast-mode is where we free the pages first and flush TLBs later. This is not a problem if there's no concurrency, but obviously if you can preempt there now is.

Quoted 5 lines

I think I prefer completely killing off fast-mode esp. since UP seems to go the way of the Dodo and it does away with an exception in the mmu_gather code.

Anyway; opinions? Linus, Thomas, Ingo?

Since UP kernels have not been packaged up by major distros for years, and since the live-patching of SMP kernels (the SMP alternative-instructions patching machinery) does away with a big chunk of the SMP cost, I guess UP kernels are slowly becoming like TINY_RCU: interesting but not really a primary design goal?

( Another reason for reducing SMP vs. UP complexity in this area would be
the fact that we had a few bad regressions lately - the TLB code is not getting simpler, and bugs are getting discovered and fixed slower. )

At least that's the x86 perspective. ARM might still see it differently?

On ARM there is a lot of ongoing work on single zImage for multiple SoCs and this implies SMP kernels. There is an SMP_ON_UP feature which does run-time code patching to optimise the UP case in a few places.

Regarding tlb_fast_mode(), the ARM-specific implementation is always 0 on ARMv7 even if UP because of speculative TLB loads (the MMU could pretty much act as a separate processor).

Quoted 7 lines · Catalin Marinas

On ARM there is a lot of ongoing work on single zImage for multiple SoCs and this implies SMP kernels. There is an SMP_ON_UP feature which does run-time code patching to optimise the UP case in a few places.

Regarding tlb_fast_mode(), the ARM-specific implementation is always 0 on ARMv7 even if UP because of speculative TLB loads (the MMU could pretty much act as a separate processor).

Oh right.. I should really refresh the mmu_gather unification patches so all archs are using the generic code.

Quoted 9 lines · Max Filippov

Hi Peter,

Quoted 1 line · Peter Zijlstra

What about something like this?

With that patch I still get mtest05 firing my TLB/PTE incoherency check in the UP PREEMPT_VOLUNTARY configuration. This happens after zap_pte_range completion in the end of unmap_region because of rescheduling called in the following call chain:

OK, so there two options; completely kill off fast-mode or something like the below where we add magic to the scheduler :/

I'm aware people might object to something like the below -- but since its a possibility I thought we ought to at least mention it.

For those new to the thread; the problem is that since the introduction of preemptible mmu_gather the traditional UP fast-mode is broken. Fast-mode is where we free the pages first and flush TLBs later. This is not a problem if there's no concurrency, but obviously if you can preempt there now is.

I think I prefer completely killing off fast-mode esp. since UP seems to go the way of the Dodo and it does away with an exception in the mmu_gather code.

Anyway; opinions? Linus, Thomas, Ingo?

And here's the patch that makes fast mode go *poof*..

Quoted 2 lines · Peter Zijlstra

And here's the patch that makes fast mode go *poof*..

Let's just do this. Mind sending a patch with proper changelog and sign-off?

Quoted 47 lines · Max Filippov

Quoted 23 lines · Max Filippov

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

Quoted 7 lines

I've noticed that with xtensa arch when I added a test before returning to userspace checking that TLB contents agrees with page tables of the current mm. This check reliably fires with the LTP test mtest05 that maps, unmaps and accesses memory from multiple threads.

Is there anything wrong in my description, maybe something specific to my arch, or this issue really exists?

Hi,

I've made similar checking function for MIPS (because qemu is my only choice and it simulates MIPS TLB) and ran my tests on mips-malta machine in qemu. With MIPS I can also see this issue. I hope I did it right, the patch at the bottom is for the reference. The test I run and the diagnostic output are as follows:

To me it looks like the cond_resched in the zap_pmd_range is the root cause of this issue (let alone SMP case for now). It was introduced in the commit

commit 97a894136f29802da19a15541de3c019e1ca147e

Author: Peter Zijlstra <[email protected]>
Date:   Tue May 24 17:12:04 2011 -0700

Quoted 3 lines

mm: Remove i_mmap_lock lockbreak

Peter, Kamezawa, other reviewers of that commit, could you please comment?

Are you all running UP systems? I suppose the preemptible muck

invalidated the assumption that UP systems are 'easy'.

If you make tlb_fast_mode() return an unconditional false, does it all work again?

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

I've noticed that with xtensa arch when I added a test before returning to userspace checking that TLB contents agrees with page tables of the current mm. This check reliably fires with the LTP test mtest05 that maps, unmaps and accesses memory from multiple threads.

Is there anything wrong in my description, maybe something specific to my arch, or this issue really exists?

I've also noticed that there are a lot of arches with default implementations of the involved functions, does that mean that any/all of them have this issue too?

Signature · 8 lines

Thanks.
-- Max

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Quoted 16 lines · Max Filippov

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 6 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

The idea behind the lazy MMU subsystem is that it does not need to flush the TLB all the time and allow one to do PTE manipulations in a "batch mode". Meaning there are stray entries - and one has to be diligient about not using them.

Here is the relvant comment from the Linux header:

/*
* A facility to provide lazy MMU batching. This allows PTE updates and
* page invalidations to be delayed until a call to leave lazy MMU mode
* is issued. Some architectures may benefit from doing this, and it is
* beneficial for both shadow and direct mode hypervisors, which may batch
* the PTE updates which happen during this window. Note that using this
* interface requires that read hazards be removed from the code. A read
* hazard could result in the direct mode hypervisor case, since the actual
* write to the page tables may not yet have taken place, so reads though
* a raw PTE pointer after it has been modified are not guaranteed to be
* up to date. This mode can only be entered and left under the protection of
* the page table locks for all page tables which may be modified. In the UP
* case, this is required so that preemption is disabled, and in the SMP case,
* it must synchronize the delayed page table writes properly on other CPUs. */

This means that eventually when arch_leave_lazy_mmu_mode or arch_flush_lazy_mmu_mode is called, the PTE updates _should_ be flushed (aka, TLB flush if needed on the altered PTE entries).

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages,

Quoted 14 lines · Max Filippov

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 3 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages,

The page is not freed at that time (at least not for the generic mmu_gather implementation). It is stored into mmu_gather and then freed along with the tlb flush in tlb_flush_mmu.

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

Max,

Quoted 4 lines · Max Filippov

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

If it happens, this would be a bug. It means that a process can access a physical page that has been allocated to something else, possibly kernel data.

Quoted 11 lines

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 6 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

It looks to me like cond_resched() here introduces a possible bug but it depends on the actual arch code, especially the __tlb_remove_tlb_entry() function. On ARM we record the range in tlb_remove_tlb_entry() and queue the pages to be removed in __tlb_remove_page(). It pretty much acts like tlb_fast_mode() == 0 even for the UP case (which is also needed for hardware speculative TLB loads). The tlb_finish_mmu() takes care of whatever pages are left to be freed.

With a dummy __tlb_remove_tlb_entry() and tlb_fast_mode() == 1, cond_resched() in zap_pmd_range() would cause problems.

I think possible workarounds:

1. tlb_fast_mode() always returning 0.
2. add a tlb_flush_mmu(tlb) before cond_resched() in zap_pmd_range().
3. implement __tlb_remove_tlb_entry() on xtensa to always flush the tlb (which is probably costly).
4. drop the cond_resched() (not sure about preemptible kernels though).

I would vote for 1 but let's see what the mm people say.

Quoted 35 lines · Catalin Marinas

Quoted 2 lines · Max Filippov

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

If it happens, this would be a bug. It means that a process can access a physical page that has been allocated to something else, possibly kernel data.

Quoted 11 lines

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 6 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

It looks to me like cond_resched() here introduces a possible bug but it depends on the actual arch code, especially the __tlb_remove_tlb_entry() function. On ARM we record the range in tlb_remove_tlb_entry() and queue the pages to be removed in __tlb_remove_page(). It pretty much acts like tlb_fast_mode() == 0 even for the UP case (which is also needed for hardware speculative TLB loads). The tlb_finish_mmu() takes care of whatever pages are left to be freed.

Quoted 2 lines

With a dummy __tlb_remove_tlb_entry() and tlb_fast_mode() == 1, cond_resched() in zap_pmd_range() would cause problems.

So, looks like most architectures in the UP configuration should have this issue (unless they flush TLB in the switch_mm, even when switching to the same mm):

Quoted 36 lines · Max Filippov

Quoted 35 lines · Catalin Marinas

Quoted 2 lines · Max Filippov

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

If it happens, this would be a bug. It means that a process can access a physical page that has been allocated to something else, possibly kernel data.

Quoted 11 lines

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 6 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

It looks to me like cond_resched() here introduces a possible bug but it depends on the actual arch code, especially the __tlb_remove_tlb_entry() function. On ARM we record the range in tlb_remove_tlb_entry() and queue the pages to be removed in __tlb_remove_page(). It pretty much acts like tlb_fast_mode() == 0 even for the UP case (which is also needed for hardware speculative TLB loads). The tlb_finish_mmu() takes care of whatever pages are left to be freed.

Quoted 6 lines

Quoted 2 lines

With a dummy __tlb_remove_tlb_entry() and tlb_fast_mode() == 1, cond_resched() in zap_pmd_range() would cause problems.

So, looks like most architectures in the UP configuration should have this issue (unless they flush TLB in the switch_mm, even when switching to the same mm):

switch_mm() wouldn't be called if switching to the same mm. You could do

it in switch_to() but it's not efficient (or before returning to user space on the same processor).

Do you happen to have a user-space test for this? Something like one

thread does an mmap(), writes some poison value, munmap(). The other thread keeps checking the poison value while trapping and ignoring any SIGSEGV. If it's working correctly, the second thread should either get a SIGSEGV or read the poison value.

Quoted 40 lines · Catalin Marinas

Quoted 39 lines · Max Filippov

Quoted 37 lines · Catalin Marinas

Quoted 2 lines · Max Filippov

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

If it happens, this would be a bug. It means that a process can access a physical page that has been allocated to something else, possibly kernel data.

Quoted 11 lines

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 6 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

It looks to me like cond_resched() here introduces a possible bug but it depends on the actual arch code, especially the __tlb_remove_tlb_entry() function. On ARM we record the range in tlb_remove_tlb_entry() and queue the pages to be removed in __tlb_remove_page(). It pretty much acts like tlb_fast_mode() == 0 even for the UP case (which is also needed for hardware speculative TLB loads). The tlb_finish_mmu() takes care of whatever pages are left to be freed.

With a dummy __tlb_remove_tlb_entry() and tlb_fast_mode() == 1, cond_resched() in zap_pmd_range() would cause problems.

Quoted 5 lines

Quoted 3 lines

So, looks like most architectures in the UP configuration should have this issue (unless they flush TLB in the switch_mm, even when switching to the same mm):

switch_mm() wouldn't be called if switching to the same mm. You could do

Hmm... Strange, but as far as I can tell from the context_switch it would.

Quoted 37 lines · Catalin Marinas

Quoted 36 lines · Max Filippov

Quoted 35 lines · Catalin Marinas

Quoted 2 lines · Max Filippov

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

If it happens, this would be a bug. It means that a process can access a physical page that has been allocated to something else, possibly kernel data.

Quoted 11 lines

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 6 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

It looks to me like cond_resched() here introduces a possible bug but it depends on the actual arch code, especially the __tlb_remove_tlb_entry() function. On ARM we record the range in tlb_remove_tlb_entry() and queue the pages to be removed in __tlb_remove_page(). It pretty much acts like tlb_fast_mode() == 0 even for the UP case (which is also needed for hardware speculative TLB loads). The tlb_finish_mmu() takes care of whatever pages are left to be freed.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

With a dummy __tlb_remove_tlb_entry() and tlb_fast_mode() == 1, cond_resched() in zap_pmd_range() would cause problems.

So, looks like most architectures in the UP configuration should have this issue (unless they flush TLB in the switch_mm, even when switching to the same mm):

switch_mm() wouldn't be called if switching to the same mm. You could do

Hmm... Strange, but as far as I can tell from the context_switch it would.

Quoted 4 lines

it in switch_to() but it's not efficient (or before returning to user space on the same processor).

Do you happen to have a user-space test for this? Something like one

I only had mtest05 from LTP that triggered TLB/PTE inconsistency, but not anything that would really try to peek at the freed page. I can make such test though.

Quoted 4 lines

thread does an mmap(), writes some poison value, munmap(). The other thread keeps checking the poison value while trapping and ignoring any SIGSEGV. If it's working correctly, the second thread should either get a SIGSEGV or read the poison value.

Max,

Quoted 4 lines · Max Filippov

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

If it happens, this would be a bug. It means that a process can access a physical page that has been allocated to something else, possibly kernel data.

Quoted 11 lines

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 6 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

It looks to me like cond_resched() here introduces a possible bug but it depends on the actual arch code, especially the __tlb_remove_tlb_entry() function. On ARM we record the range in tlb_remove_tlb_entry() and queue the pages to be removed in __tlb_remove_page(). It pretty much acts like tlb_fast_mode() == 0 even for the UP case (which is also needed for hardware speculative TLB loads). The tlb_finish_mmu() takes care of whatever pages are left to be freed.

With a dummy __tlb_remove_tlb_entry() and tlb_fast_mode() == 1, cond_resched() in zap_pmd_range() would cause problems.

I think possible workarounds:

1. tlb_fast_mode() always returning 0.

2. add a tlb_flush_mmu(tlb) before cond_resched() in zap_pmd_range().

Max,

Quoted 4 lines · Max Filippov

Hello arch and mm people.

Is it intentional that threads of a process that invoked munmap syscall can see TLB entries pointing to already freed pages, or it is a bug?

If it happens, this would be a bug. It means that a process can access a physical page that has been allocated to something else, possibly kernel data.

Quoted 11 lines

I'm talking about zap_pmd_range and zap_pte_range:

zap_pmd_range
zap_pte_range
arch_enter_lazy_mmu_mode
ptep_get_and_clear_full
tlb_remove_tlb_entry
__tlb_remove_page
arch_leave_lazy_mmu_mode
cond_resched

Quoted 6 lines

With the default arch_{enter,leave}_lazy_mmu_mode, tlb_remove_tlb_entry and __tlb_remove_page there is a loop in the zap_pte_range that clears PTEs and frees corresponding pages, but doesn't flush TLB, and surrounding loop in the zap_pmd_range that calls cond_resched. If a thread of the same process gets scheduled then it is able to see TLB entries pointing to already freed physical pages.

It looks to me like cond_resched() here introduces a possible bug but it depends on the actual arch code, especially the __tlb_remove_tlb_entry() function. On ARM we record the range in tlb_remove_tlb_entry() and queue the pages to be removed in __tlb_remove_page(). It pretty much acts like tlb_fast_mode() == 0 even for the UP case (which is also needed for hardware speculative TLB loads). The tlb_finish_mmu() takes care of whatever pages are left to be freed.

With a dummy __tlb_remove_tlb_entry() and tlb_fast_mode() == 1, cond_resched() in zap_pmd_range() would cause problems.

I think possible workarounds:

1. tlb_fast_mode() always returning 0.

2. add a tlb_flush_mmu(tlb) before cond_resched() in zap_pmd_range().