| Message ID | 55DB16AF.7090607@freebox.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Aug 24, 2015 at 03:05:51PM +0200, Nicolas Schichan wrote: > I gave your patch serie a try on ARMv5/kirkwood (backported on a v4.1 kernel) > and at first I got the following panic just after the kernel transitioned > to userland (with CONFIG_CPU_SW_DOMAIN_PAN=y): Ah, damn. Thanks for testing. I really need to add some non-ARMv7 platforms to my nightly test rig, but I'm out of physical space to do that. :p > I have tracked this to the attempt made by the code in > arch/arm/mm/abort-ev5t.S to read the fault instruction which in this > case is in unserspace: > > ldreq r3, [r4] @ read aborted ARM instruction There's going to be many more of these... it may be better if I left the domain enabled when calling into these handlers, and had every handler do the turn-off itself when it's ready to do so - there's no point turning off userspace access only to then immediately re-enable it. > With the changes above, userland boots fine and attempts to > dereference LIST_POISON1 from the kernel results the expected "page > domain fault". > > To test that I mapped LIST_POISON1 from user space via mmap() and > triggered the fault by reading from /proc/cpu/alignment. I modified the > code showing /proc/cpu/alignment to access LIST_POISON1. Without your > patch serie the access to LIST_POISON1 goes through without a hitch. Great, thanks for the independent testing of its effectiveness. > Also, when CONFIG_CPU_SW_DOMAIN_PAN is not set, the DACR_INIT constant is > setup with (domain_val(DOMAIN_USER, DOMAIN_NOACCESS) which will cause the > kernel to die with a "page domain fault" when running init. If you don't mind, I'll merge that into the patch adding this so it doesn't introduce a regression there. Once I've fixed the abort handler issue, would you mind re-testing and giving a tested-by attributation please?
On 08/25/2015 10:15 AM, Russell King - ARM Linux wrote: >> Also, when CONFIG_CPU_SW_DOMAIN_PAN is not set, the DACR_INIT constant is >> setup with (domain_val(DOMAIN_USER, DOMAIN_NOACCESS) which will cause the >> kernel to die with a "page domain fault" when running init. > > If you don't mind, I'll merge that into the patch adding this so it > doesn't introduce a regression there. > > Once I've fixed the abort handler issue, would you mind re-testing > and giving a tested-by attributation please? Sure, I've got no problem with that and I'll be happy to re-test.
diff --git a/arch/arm/mm/abort-ev5t.S b/arch/arm/mm/abort-ev5t.S index a0908d4..fc3a219 100644 --- a/arch/arm/mm/abort-ev5t.S +++ b/arch/arm/mm/abort-ev5t.S @@ -15,12 +15,33 @@ * abort here if the I-TLB and D-TLB aren't seeing the same * picture. Unfortunately, this does happen. We live with it. */ + + .macro uaccess_disable, tmp +#ifdef CONFIG_CPU_SW_DOMAIN_PAN + /* + * Whenever we re-enter userspace, the domains should always be + * set appropriately. + */ + mov \tmp, #DACR_UACCESS_DISABLE + mcr p15, 0, \tmp, c3, c0, 0 @ Set domain register +#endif + .endm + + .macro uaccess_enable, tmp +#ifdef CONFIG_CPU_SW_DOMAIN_PAN + mov \tmp, #DACR_UACCESS_ENABLE + mcr p15, 0, \tmp, c3, c0, 0 @ set domain register +#endif + .endm + .align 5 ENTRY(v5t_early_abort) mrc p15, 0, r1, c5, c0, 0 @ get FSR mrc p15, 0, r0, c6, c0, 0 @ get FAR do_thumb_abort fsr=r1, pc=r4, psr=r5, tmp=r3 + uaccess_enable ip ldreq r3, [r4] @ read aborted ARM instruction + uaccess_disable ip bic r1, r1, #1 << 11 @ clear bits 11 of FSR do_ldrd_abort tmp=ip, insn=r3 tst r3, #1 << 20 @ check write It looks like ARMv6 with CONFIG_ARM_ERRATA_326103 enabled will suffer from the same issue as it reads the faulty ARM instruction, possibly from userland (see arch/arm/mm/abort-ev6.S). With the changes above, userland boots fine and attempts to dereference LIST_POISON1 from the kernel results the expected "page domain fault". To test that I mapped LIST_POISON1 from user space via mmap() and triggered the fault by reading from /proc/cpu/alignment. I modified the code showing /proc/cpu/alignment to access LIST_POISON1. Without your patch serie the access to LIST_POISON1 goes through without a hitch. Also, when CONFIG_CPU_SW_DOMAIN_PAN is not set, the DACR_INIT constant is setup with (domain_val(DOMAIN_USER, DOMAIN_NOACCESS) which will cause the kernel to die with a "page domain fault" when running init. The following (crude patch) works around that: diff --git a/arch/arm/include/asm/domain.h b/arch/arm/include/asm/domain.h index 0c37397..e878129 100644 --- a/arch/arm/include/asm/domain.h +++ b/arch/arm/include/asm/domain.h @@ -57,11 +57,19 @@ #define domain_mask(dom) ((3) << (2 * (dom))) #define domain_val(dom,type) ((type) << (2 * (dom))) +#ifdef CONFIG_CPU_SW_DOMAIN_PAN #define DACR_INIT \ (domain_val(DOMAIN_USER, DOMAIN_NOACCESS) | \ domain_val(DOMAIN_KERNEL, DOMAIN_MANAGER) | \ domain_val(DOMAIN_IO, DOMAIN_CLIENT) | \ domain_val(DOMAIN_VECTORS, DOMAIN_CLIENT)) +#else +#define DACR_INIT \ + (domain_val(DOMAIN_USER, DOMAIN_CLIENT) | \ + domain_val(DOMAIN_KERNEL, DOMAIN_MANAGER) | \ + domain_val(DOMAIN_IO, DOMAIN_CLIENT) | \ + domain_val(DOMAIN_VECTORS, DOMAIN_CLIENT)) +#endif #define __DACR_DEFAULT \ domain_val(DOMAIN_KERNEL, DOMAIN_CLIENT) | \