Fixes for SW PAN

update_saved_ttbr0 mandates that mm->pgd is not swapper, since swapper contains kernel mappings and should never be installed into ttbr0. However, this means that callers must avoid passing the init_mm to update_saved_ttbr0 which in turn can cause the saved ttbr0 value to be out-of-date in the context of the idle thread. For example, EFI runtime services may leave the saved ttbr0 pointing at the EFI page table, and kernel threads may end up with stale references to freed page tables.

This patch changes update_saved_ttbr0 so that the init_mm points the saved ttbr0 value to the empty zero page, which always exists and never contains valid translations. EFI and switch can then call into update_saved_ttbr0 unconditionally.

Cc Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Vinayak Menon <[email protected]>

Reported-by Vinayak Menon <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/efi.h         |  4 +---
 arch/arm64/include/asm/mmu_context.h | 22 +++++++++++++---------
 2 files changed, 14 insertions(+), 12 deletions(-)

arch/arm64/include/asm/efi.h +1 -3

index 650344d01124..c4cd5081d78b 100644
@@ -132,11 +132,9 @@ static inline void efi_set_pgd(struct mm_struct *mm)
 			 * Defer the switch to the current thread's TTBR0_EL1
 			 * until uaccess_enable(). Restore the current
 			 * thread's saved ttbr0 corresponding to its active_mm
-			 * (if different from init_mm).
 			 */
 			cpu_set_reserved_ttbr0();
-			if (current->active_mm != &init_mm)
-				update_saved_ttbr0(current, current->active_mm);
+			update_saved_ttbr0(current, current->active_mm);
 		}
 	}
 }

arch/arm64/include/asm/mmu_context.h +13 -9

index 3257895a9b5e..f7773f90546e 100644
@@ -174,11 +174,17 @@ enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
 static inline void update_saved_ttbr0(struct task_struct *tsk,
 				      struct mm_struct *mm)
 {
-	if (system_uses_ttbr0_pan()) {
-		BUG_ON(mm->pgd == swapper_pg_dir);
-		task_thread_info(tsk)->ttbr0 =
-			virt_to_phys(mm->pgd) | ASID(mm) << 48;
-	}
+	u64 ttbr;
+
+	if (!system_uses_ttbr0_pan())
+		return;
+
+	if (mm == &init_mm)
+		ttbr = __pa_symbol(empty_zero_page);
+	else
+		ttbr = virt_to_phys(mm->pgd) | ASID(mm) << 48;
+
+	task_thread_info(tsk)->ttbr0 = ttbr;
 }
 #else
 static inline void update_saved_ttbr0(struct task_struct *tsk,

@@ -214,11 +220,9 @@ switch_mm(struct mm_struct *prev, struct mm_struct *next,
 	 * Update the saved TTBR0_EL1 of the scheduled-in task as the previous
 	 * value may have not been initialised yet (activate_mm caller) or the
 	 * ASID has changed since the last run (following the context switch
-	 * of another thread of the same process). Avoid setting the reserved
-	 * TTBR0_EL1 to swapper_pg_dir (init_mm; e.g. via idle_task_exit).
+	 * of another thread of the same process).
 	 */
-	if (next != &init_mm)
-		update_saved_ttbr0(tsk, next);
+	update_saved_ttbr0(tsk, next);
 }
 
 #define deactivate_mm(tsk,mm)	do { } while (0)

Signature · 1 line

2.1.4

update_saved_ttbr0 mandates that mm->pgd is not swapper, since swapper contains kernel mappings and should never be installed into ttbr0. However, this means that callers must avoid passing the init_mm to update_saved_ttbr0 which in turn can cause the saved ttbr0 value to be out-of-date in the context of the idle thread. For example, EFI runtime services may leave the saved ttbr0 pointing at the EFI page table, and kernel threads may end up with stale references to freed page tables.

This patch changes update_saved_ttbr0 so that the init_mm points the saved ttbr0 value to the empty zero page, which always exists and never contains valid translations. EFI and switch can then call into update_saved_ttbr0 unconditionally.

Cc Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Vinayak Menon <[email protected]>

Reported-by Vinayak Menon <[email protected]>

Signed-off-by Will Deacon <[email protected]>

enter_lazy_tlb is called when a kernel thread rides on the back of another mm, due to a context switch or an explicit call to unuse_mm where a call to switch_mm is elided.

In these cases, it's important to keep the saved ttbr value up to date with the active mm, otherwise we can end up with a stale value which points to a potentially freed page table.

This patch implements enter_lazy_tlb for arm64, so that the saved ttbr0 is kept up-to-date with the active mm for kernel threads.

Cc Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Vinayak Menon <[email protected]>

Reported-by Vinayak Menon <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/mmu_context.h | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

arch/arm64/include/asm/mmu_context.h +10 -14

index f7773f90546e..9d155fa9a507 100644
@@ -156,20 +156,6 @@ void check_and_switch_context(struct mm_struct *mm, unsigned int cpu);
 
 #define init_new_context(tsk,mm)	({ atomic64_set(&(mm)->context.id, 0); 0; })
 
-/*
- * This is called when "tsk" is about to enter lazy TLB mode.
- *
- * mm:  describes the currently active mm context
- * tsk: task which is entering lazy tlb
- * cpu: cpu number which is entering lazy tlb
- *
- * tsk->mm will be NULL
- */
-static inline void
-enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
-{
-}
-
 #ifdef CONFIG_ARM64_SW_TTBR0_PAN
 static inline void update_saved_ttbr0(struct task_struct *tsk,
 				      struct mm_struct *mm)

@@ -193,6 +179,16 @@ static inline void update_saved_ttbr0(struct task_struct *tsk,
 }
 #endif
 
+static inline void
+enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
+{
+	/*
+	 * We don't actually care about the ttbr0 mapping, so point it at the
+	 * zero page.
+	 */
+	update_saved_ttbr0(tsk, &init_mm);
+}
+
 static inline void __switch_mm(struct mm_struct *next)
 {
 	unsigned int cpu = smp_processor_id();

Signature · 1 line

2.1.4

enter_lazy_tlb is called when a kernel thread rides on the back of another mm, due to a context switch or an explicit call to unuse_mm where a call to switch_mm is elided.

In these cases, it's important to keep the saved ttbr value up to date with the active mm, otherwise we can end up with a stale value which points to a potentially freed page table.

This patch implements enter_lazy_tlb for arm64, so that the saved ttbr0 is kept up-to-date with the active mm for kernel threads.

Cc Mark Rutland <[email protected]>

Cc Ard Biesheuvel <[email protected]>

Cc Vinayak Menon <[email protected]>

Reported-by Vinayak Menon <[email protected]>

Signed-off-by Will Deacon <[email protected]>

Hi all,

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Quoted 9 lines · Will Deacon

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

Quoted 6 lines

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Both patches look good to me, per my understanding of the problem, so FWIW, for the series:

Reviewed-by Mark Rutland <[email protected]>

However, I think we have another issue in this area. We have sequences where we update the HW TTBR0 before the shadow, e.g. in efi_set_pgd(), where we do:

	cpu_set_reserved_ttbr0();
	update_saved_ttbr0(current, current->active_mm);

... so if an exception comes in between after HW TTBR0 update but before the shadow update, the stale shadow value will be restored upon the exception return, leaving the HW TTBR0 stale.

That's easy enough to fix in the efi code, e.g.

	update_saved_ttbr0(current, current->active_mm);
	barrier();
	cpu_set_reserved_ttbr0();

... but I haven't yet figured out a nice way to sort out switch_mm(), __switch_mm(), and check_and_switch_context().

Hi all,

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Cheers,

Will

--->8

Will Deacon (2):
arm64: SW PAN: Point saved ttbr0 at the zero page when switching to
init_mm
arm64: SW PAN: Update saved ttbr0 value on enter_lazy_tlb

 arch/arm64/include/asm/efi.h         |  4 +---
 arch/arm64/include/asm/mmu_context.h | 46 ++++++++++++++++++------------------
 2 files changed, 24 insertions(+), 26 deletions(-)

Quoted 11 lines · Will Deacon

Hi all,

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

Quoted 6 lines

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Thanks Will. So these 2 patches fix the case of kthreads having a stale saved ttbr0. The callstack I had shared in the original issue description was not of a kthread (its user task with PF_KTHREAD not set. The tsk->mm was set to NULL by exit_mm I think). So do you think this could be a different problem ?
I had a look at the dumps again and what I see is that, the PA part of the saved ttbr0
(from thread_info) is not the same as the pa(tsk->active_mm->pgd). The PA derived from saved ttbr0 actually points to a page which is "now" owned by slab.

Quoted 17 lines · Vinayak Menon

Quoted 9 lines · Will Deacon

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

Quoted 6 lines

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Quoted 6 lines

Thanks Will. So these 2 patches fix the case of kthreads having a stale saved ttbr0. The callstack I had shared in the original issue description was not of a kthread (its user task with PF_KTHREAD not set. The tsk->mm was set to NULL by exit_mm I think). So do you think this could be a different problem ?
I had a look at the dumps again and what I see is that, the PA part of the saved ttbr0
(from thread_info) is not the same as the pa(tsk->active_mm->pgd). The PA derived from saved ttbr0 actually points to a page which is "now" owned by slab.

Having not been able to reproduce the failure you described, I can't give you a good answer to this.

Quoted 25 lines · Will Deacon

Quoted 17 lines · Vinayak Menon

Quoted 9 lines · Will Deacon

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

Quoted 6 lines

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Quoted 6 lines

Thanks Will. So these 2 patches fix the case of kthreads having a stale saved ttbr0. The callstack I had shared in the original issue description was not of a kthread (its user task with PF_KTHREAD not set. The tsk->mm was set to NULL by exit_mm I think). So do you think this could be a different problem ?
I had a look at the dumps again and what I see is that, the PA part of the saved ttbr0
(from thread_info) is not the same as the pa(tsk->active_mm->pgd). The PA derived from saved ttbr0 actually points to a page which is "now" owned by slab.

Quoted 2 lines

Having not been able to reproduce the failure you described, I can't give you a good answer to this.

While these fixes make sense for a stable backport, I could go back to per-cpu saved_ttbr0 as in the first version of this patchset:

http://lkml.kernel.org/r/1471015666-23125-4-git-send-email-catalin.marinas at arm.com

(changed in v2 for some marginally shorter asm code)

Quoted 32 lines · Catalin Marinas

Quoted 25 lines · Will Deacon

Quoted 17 lines · Vinayak Menon

Quoted 9 lines · Will Deacon

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

Quoted 6 lines

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Quoted 6 lines

Thanks Will. So these 2 patches fix the case of kthreads having a stale saved ttbr0. The callstack I had shared in the original issue description was not of a kthread (its user task with PF_KTHREAD not set. The tsk->mm was set to NULL by exit_mm I think). So do you think this could be a different problem ?
I had a look at the dumps again and what I see is that, the PA part of the saved ttbr0
(from thread_info) is not the same as the pa(tsk->active_mm->pgd). The PA derived from saved ttbr0 actually points to a page which is "now" owned by slab.

Quoted 2 lines

Having not been able to reproduce the failure you described, I can't give you a good answer to this.

While these fixes make sense for a stable backport, I could go back to per-cpu saved_ttbr0 as in the first version of this patchset:

Quoted 3 lines

http://lkml.kernel.org/r/1471015666-23125-4-git-send-email-catalin.marinas at arm.com

(changed in v2 for some marginally shorter asm code)

To be honest, if we're going to consider changes as fundamental as that, I'd much prefer for us to use the active_mm->pgd directly, setting the zero page if it's either NULL or init_mm. This would need some hacking for EFI...

Quoted 27 lines · Will Deacon

Quoted 26 lines · Catalin Marinas

Quoted 25 lines · Will Deacon

Quoted 23 lines · Vinayak Menon

Quoted 9 lines · Will Deacon

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

Quoted 6 lines

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Thanks Will. So these 2 patches fix the case of kthreads having a stale saved ttbr0. The callstack I had shared in the original issue description was not of a kthread (its user task with PF_KTHREAD not set. The tsk->mm was set to NULL by exit_mm I think). So do you think this could be a different problem ?
I had a look at the dumps again and what I see is that, the PA part of the saved ttbr0
(from thread_info) is not the same as the pa(tsk->active_mm->pgd). The PA derived from saved ttbr0 actually points to a page which is "now" owned by slab.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Having not been able to reproduce the failure you described, I can't give you a good answer to this.

Quoted 5 lines

Quoted 5 lines

While these fixes make sense for a stable backport, I could go back to per-cpu saved_ttbr0 as in the first version of this patchset:

http://lkml.kernel.org/r/1471015666-23125-4-git-send-email-catalin.marinas at arm.com

Quoted 5 lines

Quoted 1 line

(changed in v2 for some marginally shorter asm code)

To be honest, if we're going to consider changes as fundamental as that, I'd much prefer for us to use the active_mm->pgd directly, setting the zero page if it's either NULL or init_mm. This would need some hacking for EFI...

The problem is the __pa(active_mm->pgd) and doing it in assembly may be pretty unreadable.

Yet another option is to move save_ttbr0 in mm_context_t.

Quoted 26 lines · Catalin Marinas

Quoted 25 lines · Will Deacon

Quoted 24 lines · Catalin Marinas

Quoted 23 lines · Will Deacon

Quoted 22 lines · Vinayak Menon

Quoted 9 lines · Will Deacon

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

Quoted 6 lines

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Thanks Will. So these 2 patches fix the case of kthreads having a stale saved ttbr0. The callstack I had shared in the original issue description was not of a kthread (its user task with PF_KTHREAD not set. The tsk->mm was set to NULL by exit_mm I think). So do you think this could be a different problem ?
I had a look at the dumps again and what I see is that, the PA part of the saved ttbr0
(from thread_info) is not the same as the pa(tsk->active_mm->pgd). The PA derived from saved ttbr0 actually points to a page which is "now" owned by slab.

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Having not been able to reproduce the failure you described, I can't give you a good answer to this.

Looking at the code (again), if we context switch in do_exit after exit_mm, then the thread behaves an awful lot like a kernel thread: current->mm is NULL and we're in lazy TLB mode.

Furthermore, that context switch will drop
the last reference to the old mm and the pgd will finally be freed.

So I think my patches will solve your case too because we'll call enter_lazy_tlb again when getting scheduled back in. If you have any way to test them, that would be great.

Quoted 27 lines · Will Deacon

Quoted 26 lines · Catalin Marinas

Quoted 25 lines · Will Deacon

Quoted 24 lines · Catalin Marinas

Quoted 23 lines · Will Deacon

Quoted 22 lines · Vinayak Menon

Quoted 9 lines · Will Deacon

After lots of collective head scratching in response to Vinayak's mail here:

http://lists.infradead.org/pipermail/linux-arm-kernel/2017-December/545641.html

It turns out that we have a problem with SW PAN and kernel threads, where the saved ttbr0 value for a kernel thread can be stale and subsequently inherited by other kernel threads over a fork.

Quoted 6 lines

These two patches attempt to fix that. We've not be able to reproduce the exact failure reported above, but I added some assertions to the uaccess routines to check for discrepancies between the active_mm pgd and the saved ttbr0 value (ignoring the zero page) and these no longer fire with these changes, but do fire without them if EFI runtime services are enabled on my Seattle board.

Thanks Will. So these 2 patches fix the case of kthreads having a stale saved ttbr0. The callstack I had shared in the original issue description was not of a kthread (its user task with PF_KTHREAD not set. The tsk->mm was set to NULL by exit_mm I think). So do you think this could be a different problem ?
I had a look at the dumps again and what I see is that, the PA part of the saved ttbr0
(from thread_info) is not the same as the pa(tsk->active_mm->pgd). The PA derived from saved ttbr0 actually points to a page which is "now" owned by slab.

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Having not been able to reproduce the failure you described, I can't give you a good answer to this.

Looking at the code (again), if we context switch in do_exit after exit_mm, then the thread behaves an awful lot like a kernel thread: current->mm is NULL and we're in lazy TLB mode.

Yes, that could be the case.
I am going to try out these 2 patches and see if the issue gets resolved. It usually takes more than a day to reproduce the problem. Will update you as soon as I get the results.

Quoted 6 lines

Furthermore, that context switch will drop
the last reference to the old mm and the pgd will finally be freed.

So I think my patches will solve your case too because we'll call enter_lazy_tlb again when getting scheduled back in. If you have any way to test them, that would be great.