Re: KVM guest sometimes failed to boot because of kernel stack overflow if KPTI is enabled on a hisilicon ARM64 platform.

[Will Deacon]

Hi Wei,

Thanks for giving that a spin.

On Fri, Jun 22, 2018 at 06:45:15PM +0800, Wei Xu wrote:
> On 2018/6/22 17:23, Will Deacon wrote:
> >On Fri, Jun 22, 2018 at 09:33:04AM +0100, Wei Xu wrote:
> >>On 2018/6/21 11:54, Will Deacon wrote:
> >>>On Thu, Jun 21, 2018 at 11:14:28AM +0100, Wei Xu wrote:
> >>>>On 2018/6/21 10:18, Will Deacon wrote:
> >>>>>Wei -- does the diff below help at all? Make sure you disable CONFIG_KASAN,
> >>>>>otherwise your kernel will take an age to boot.
> >>>>Yes, amazing! This patch resolved the issue.
> >>>Great...
> >>>
> >>>>I have tested 50 times and can not reproduce the issue any more.
> >>>>Could you please tell more why this patch works?
> >>>You might need to ask your CPU design team ;)
> >>>
> >>>Without this patch, the code in idmap_kpti_install_ng_mappings() sets
> >>>bit 11 in table descriptors so that we can keep track of which parts of
> >>>the page table we've visited. With this patch, we don't bother tracking
> >>>and potentially rewalk parts of the page table (which takes a very long
> >>>time if KASAN is enabled).
> >>Got it. Thanks!
> >>
> >>>The architecture documents I've looked at are clear that bit 11 is IGNORED
> >>>by the CPU, which:
> >>>
> >>>   "Indicates that the architecture guarantees that the bit or field is not
> >>>    interpreted or modified by hardware."
> >>>
> >>>Please can you double-check that your CPU is indeed ignoring bit 11 in
> >>>non-leaf (table) descriptors?
> >>Do the non-leaf(table) descriptors mean the table descriptors
> >>of the section D4.3.1 "VMSAv8-64 translation table level 0, level 1, and level 2 descriptor formats"
> >>in the ARM Architecture Reference Manual ARMv8 for ARMv8-A(DDI0487C_a_armv8_arm.pdf)?
> >>
> >>If yes, our hardware does ignore it(not interpret or modify).
> >Ok, thanks for checking.
> >
> >>Is there any other possible reason cause this?
> >Perhaps just writing back the table entries is enough to cause the issue,
> >although I really can't understand why that would be the case. Can you try
> >the diff below (without my previous change), please?
> 
> Thanks!
> But it does not resolve the issue(only apply this patch based on 4.17.0).

Thanks, that's a useful data point. It means that it still crashes even if
we write back the same table entries, so it's the fact that we're writing
them at all which causes the problem, not the value that we write.

Whilst looking at the code, we noticed a missing DMB. On the off-chance
that it helps, can you try this instead please?

Will

--->8

diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
index 5f9a73a4452c..03646e6a2ef4 100644
--- a/arch/arm64/mm/proc.S
+++ b/arch/arm64/mm/proc.S
@@ -217,8 +217,9 @@ ENDPROC(idmap_cpu_replace_ttbr1)
 
 	.macro __idmap_kpti_put_pgtable_ent_ng, type
 	orr	\type, \type, #PTE_NG		// Same bit for blocks and pages
-	str	\type, [cur_\()\type\()p]	// Update the entry and ensure it
-	dc	civac, cur_\()\type\()p		// is visible to all CPUs.
+	str	\type, [cur_\()\type\()p]	// Update the entry and ensure
+	dmb	sy				// that it is visible to all
+	dc	civac, cur_\()\type\()p		// CPUs.
 	.endm
 
 /*