Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Russell King - ARM Linux]

On Thu, Dec 03, 2015 at 08:33:13AM +0000, Peter Rosin wrote:
> I wrote:
> > If I enable CONFIG_CPU_SW_DOMAIN_PAN, I sometimes (but not always) get the
> > following (or very similar) on boot.
> 
> I should have said "if I don't disable", as the option is "default y".
> 
> Also, if it survives on boot, below is an example of later trouble (after
> 30+ minutes on this occasion).

Please apply this patch so we (might) get a slightly better oops dump,
and then try to reproduce.

diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 7a7c4ce..92789ce 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -95,6 +95,22 @@ void __show_regs(struct pt_regs *regs)
 {
 	unsigned long flags;
 	char buf[64];
+#ifndef CONFIG_CPU_V7M
+	unsigned int domain;
+#ifdef CONFIG_CPU_SW_DOMAIN_PAN
+	/*
+	 * Get the domain register for the parent context. In user
+	 * mode, we don't save the DACR, so lets use what it should
+	 * be. For other modes, we place it after the pt_regs struct.
+	 */
+	if (user_mode(regs))
+		domain = DACR_UACCESS_ENABLE;
+	else
+		domain = *(unsigned int *)(regs + 1);
+#else
+	domain = get_domain();
+#endif
+#endif
 
 	show_regs_print_info(KERN_DEFAULT);
 
@@ -123,21 +139,8 @@ void __show_regs(struct pt_regs *regs)
 
 #ifndef CONFIG_CPU_V7M
 	{
-		unsigned int domain = get_domain();
 		const char *segment;
 
-#ifdef CONFIG_CPU_SW_DOMAIN_PAN
-		/*
-		 * Get the domain register for the parent context. In user
-		 * mode, we don't save the DACR, so lets use what it should
-		 * be. For other modes, we place it after the pt_regs struct.
-		 */
-		if (user_mode(regs))
-			domain = DACR_UACCESS_ENABLE;
-		else
-			domain = *(unsigned int *)(regs + 1);
-#endif
-
 		if ((domain & domain_mask(DOMAIN_USER)) ==
 		    domain_val(DOMAIN_USER, DOMAIN_NOACCESS))
 			segment = "none";
@@ -163,11 +166,11 @@ void __show_regs(struct pt_regs *regs)
 		buf[0] = '\0';
 #ifdef CONFIG_CPU_CP15_MMU
 		{
-			unsigned int transbase, dac = get_domain();
+			unsigned int transbase;
 			asm("mrc p15, 0, %0, c2, c0\n\t"
 			    : "=r" (transbase));
 			snprintf(buf, sizeof(buf), "  Table: %08x  DAC: %08x",
-			  	transbase, dac);
+			  	transbase, domain);
 		}
 #endif
 		asm("mrc p15, 0, %0, c1, c0\n" : "=r" (ctrl));

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

Russell King wrote:
> On Thu, Dec 03, 2015 at 08:33:13AM +0000, Peter Rosin wrote:
> > I wrote:
> > > If I enable CONFIG_CPU_SW_DOMAIN_PAN, I sometimes (but not always) get the
> > > following (or very similar) on boot.
> > 
> > I should have said "if I don't disable", as the option is "default y".
> > 
> > Also, if it survives on boot, below is an example of later trouble (after
> > 30+ minutes on this occasion).
> 
> Please apply this patch so we (might) get a slightly better oops dump,
> and then try to reproduce.

Sure thing, but it's still "DAC: 00000051"...

Cheers,
Peter

Unhandled fault: page domain fault (0x81b) at 0x00086578
pgd = c280c000
[00086578] *pgd=22f77831, *pte=2347034f, *ppte=2347083f
Internal error: : 81b [#1] ARM
Modules linked in:
CPU: 0 PID: 904 Comm: ntpd Not tainted 4.3.0+ #30
Hardware name: Atmel SAMA5
task: c39bb500 ti: c2fca000 task.ti: c2fca000
PC is at memcpy+0x50/0x330
LR is at 0x1
pc : [<c01daff0>]    lr : [<00000001>]    psr: 800f0013
sp : c2fcbed4  ip : 00000007  fp : 000d3808
r10: 00000000  r9 : 00000080  r8 : 00000001
r7 : 00000010  r6 : 00000010  r5 : 003f5c28  r4 : 00000000
r3 : 00000002  r2 : ffffffe0  r1 : c2fcbf38  r0 : 00086578
Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c53c7d  Table: 2280c059  DAC: 00000051
Process ntpd (pid: 904, stack limit = 0xc2fca208)
Stack: (0xc2fcbed4 to 0xc2fcc000)
bec0:                                              00000000 c2fca000 00000000
bee0: c2fcbf18 00086578 00086578 c01e76c4 c2f77218 c2ff70f4 000d4344 00086578
bf00: 00000000 00000051 0000007c c0010224 c2fca000 c004c988 00000002 00000000
bf20: 003f5c28 00000010 00000010 00000001 00000007 00000001 01f40000 5660283b
bf40: 000ae9dc 00002710 00000000 00000000 00000000 00000000 00000000 00000000
bf60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
bf80: 00000000 00000000 00000000 00000000 00000000 00000000 000d3968 0005e38f
bfa0: 000d4344 c0010060 000d3968 0005e38f 00086578 00000000 ffffffff 00000001
bfc0: 000d3968 0005e38f 000d4344 0000007c 000d3814 00000001 000d4338 000d3808
bfe0: 00081e7c bec3e8b4 00026d3d b6ced126 00000030 00086578 8499c1b5 48743d49
[<c01daff0>] (memcpy) from [<c01e76c4>] (__copy_to_user_memcpy+0x138/0x17c)
[<c01e76c4>] (__copy_to_user_memcpy) from [<c004c988>] (SyS_adjtimex+0xd4/0xf0)
[<c004c988>] (SyS_adjtimex) from [<c0010060>] (ret_fast_syscall+0x0/0x3c)
Code: f5d1f05c f5d1f07c e8b151f8 e2522020 (e8a051f8)
---[ end trace bd9256bb17081c58 ]---

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Russell King - ARM Linux]

On Thu, Dec 03, 2015 at 11:38:20AM +0000, Peter Rosin wrote:
> Russell King wrote:
> > On Thu, Dec 03, 2015 at 08:33:13AM +0000, Peter Rosin wrote:
> > > I wrote:
> > > > If I enable CONFIG_CPU_SW_DOMAIN_PAN, I sometimes (but not always) get the
> > > > following (or very similar) on boot.
> > > 
> > > I should have said "if I don't disable", as the option is "default y".
> > > 
> > > Also, if it survives on boot, below is an example of later trouble (after
> > > 30+ minutes on this occasion).
> > 
> > Please apply this patch so we (might) get a slightly better oops dump,
> > and then try to reproduce.
> 
> Sure thing, but it's still "DAC: 00000051"...

Thanks, that confirms that something in the uaccess-with-memcpy code
is clearing the DACR back to 0x51.

This has come up several times before, and I really can't spot the
problem in this code, so I've always said to disable the
uaccess-with-memcpy code.  Personally, I'd like to see the back
of that code...

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

Russell King wrote:
> On Thu, Dec 03, 2015 at 11:38:20AM +0000, Peter Rosin wrote:
> > Russell King wrote:
> > > On Thu, Dec 03, 2015 at 08:33:13AM +0000, Peter Rosin wrote:
> > > > I wrote:
> > > > > If I enable CONFIG_CPU_SW_DOMAIN_PAN, I sometimes (but not always) get the
> > > > > following (or very similar) on boot.
> > > > 
> > > > I should have said "if I don't disable", as the option is "default y".
> > > > 
> > > > Also, if it survives on boot, below is an example of later trouble (after
> > > > 30+ minutes on this occasion).
> > > 
> > > Please apply this patch so we (might) get a slightly better oops dump,
> > > and then try to reproduce.
> > 
> > Sure thing, but it's still "DAC: 00000051"...
> 
> Thanks, that confirms that something in the uaccess-with-memcpy code
> is clearing the DACR back to 0x51.
> 
> This has come up several times before, and I really can't spot the
> problem in this code, so I've always said to disable the
> uaccess-with-memcpy code.  Personally, I'd like to see the back
> of that code...

Ok, but it's not me doing crazy things if that's what you are implying
(not saying that you are), because the sama5_defconfig has

	CONFIG_UACCESS_WITH_MEMCPY=y

So, something needs to happen or sama5 remains default-broken.

Cheers,
Peter

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Russell King - ARM Linux]

On Thu, Dec 03, 2015 at 12:08:11PM +0000, Peter Rosin wrote:
> Russell King wrote:
> > On Thu, Dec 03, 2015 at 11:38:20AM +0000, Peter Rosin wrote:
> > > Russell King wrote:
> > > > On Thu, Dec 03, 2015 at 08:33:13AM +0000, Peter Rosin wrote:
> > > > > I wrote:
> > > > > > If I enable CONFIG_CPU_SW_DOMAIN_PAN, I sometimes (but not always) get the
> > > > > > following (or very similar) on boot.
> > > > > 
> > > > > I should have said "if I don't disable", as the option is "default y".
> > > > > 
> > > > > Also, if it survives on boot, below is an example of later trouble (after
> > > > > 30+ minutes on this occasion).
> > > > 
> > > > Please apply this patch so we (might) get a slightly better oops dump,
> > > > and then try to reproduce.
> > > 
> > > Sure thing, but it's still "DAC: 00000051"...
> > 
> > Thanks, that confirms that something in the uaccess-with-memcpy code
> > is clearing the DACR back to 0x51.
> > 
> > This has come up several times before, and I really can't spot the
> > problem in this code, so I've always said to disable the
> > uaccess-with-memcpy code.  Personally, I'd like to see the back
> > of that code...
> 
> Ok, but it's not me doing crazy things if that's what you are implying
> (not saying that you are), because the sama5_defconfig has
> 
> 	CONFIG_UACCESS_WITH_MEMCPY=y
> 
> So, something needs to happen or sama5 remains default-broken.

I have no solution for this, other than saying that uaccess-with-memcpy
seems to be (for some unknown reason) incompatible with SW PAN.

Out of the two features, I'd go for SW PAN over uaccess-with-memcpy,
but others will have a different opinion.  The real solution is to
track down what's going on and why, but I don't think anyone has the
motivation to do that.

I've looked into this problem, and I've been unable to identify what's
going on here.  I can see no reason for the DACR to be set to 0x51 in
this code.

The entry path into this code is via __copy_to_user(), which saves and
sets the DACR to 0x55 before calling arm_copy_to_user(), which for
uaccess-with-memcpy() is in arch/arm/lib/uaccess_with_memcpy.c.  That
tail-calls into __copy_to_user_memcpy(), which should run with the
DACR set to 0x55.

The only place that the DACR is changed is inside __put_user(), which
saves the DACR before also setting it to 0x55, and then restores the
old DACR value, which, because the old value was 0x55, will have no
effect.

So, I can see no way that the DACR should ever be 0x51 inside
__copy_to_user_memcpy(), but you are seeing such a scenario.  I've no
idea how you could ever get a value of 0x51 out of the DACR within this
code.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

Russell King wrote:
> On Thu, Dec 03, 2015 at 12:08:11PM +0000, Peter Rosin wrote:
> > Russell King wrote:
> > > On Thu, Dec 03, 2015 at 11:38:20AM +0000, Peter Rosin wrote:
> > > > Russell King wrote:
> > > > > On Thu, Dec 03, 2015 at 08:33:13AM +0000, Peter Rosin wrote:
> > > > > > I wrote:
> > > > > > > If I enable CONFIG_CPU_SW_DOMAIN_PAN, I sometimes (but not always) get the
> > > > > > > following (or very similar) on boot.
> > > > > > 
> > > > > > I should have said "if I don't disable", as the option is "default y".
> > > > > > 
> > > > > > Also, if it survives on boot, below is an example of later trouble (after
> > > > > > 30+ minutes on this occasion).
> > > > > 
> > > > > Please apply this patch so we (might) get a slightly better oops dump,
> > > > > and then try to reproduce.
> > > > 
> > > > Sure thing, but it's still "DAC: 00000051"...
> > > 
> > > Thanks, that confirms that something in the uaccess-with-memcpy code
> > > is clearing the DACR back to 0x51.
> > > 
> > > This has come up several times before, and I really can't spot the
> > > problem in this code, so I've always said to disable the
> > > uaccess-with-memcpy code.  Personally, I'd like to see the back
> > > of that code...
> > 
> > Ok, but it's not me doing crazy things if that's what you are implying
> > (not saying that you are), because the sama5_defconfig has
> > 
> > 	CONFIG_UACCESS_WITH_MEMCPY=y
> > 
> > So, something needs to happen or sama5 remains default-broken.
> 
> I have no solution for this, other than saying that uaccess-with-memcpy
> seems to be (for some unknown reason) incompatible with SW PAN.
> 
> Out of the two features, I'd go for SW PAN over uaccess-with-memcpy,
> but others will have a different opinion.  The real solution is to
> track down what's going on and why, but I don't think anyone has the
> motivation to do that.
> 
> I've looked into this problem, and I've been unable to identify what's
> going on here.  I can see no reason for the DACR to be set to 0x51 in
> this code.
> 
> The entry path into this code is via __copy_to_user(), which saves and
> sets the DACR to 0x55 before calling arm_copy_to_user(), which for
> uaccess-with-memcpy() is in arch/arm/lib/uaccess_with_memcpy.c.  That
> tail-calls into __copy_to_user_memcpy(), which should run with the
> DACR set to 0x55.
> 
> The only place that the DACR is changed is inside __put_user(), which
> saves the DACR before also setting it to 0x55, and then restores the
> old DACR value, which, because the old value was 0x55, will have no
> effect.
> 
> So, I can see no way that the DACR should ever be 0x51 inside
> __copy_to_user_memcpy(), but you are seeing such a scenario.  I've no
> idea how you could ever get a value of 0x51 out of the DACR within this
> code.

Since it seems like a race is at the bottom of the observed problems, I'm
going to look for things that look racy. The things that stand out to me
are:

* uaccess.h:modify_domain() does a read-modify-write on DACR using
  get_domain and set_domain, and I don't see any locking. Is that
  safe? Why?

* uaccess_with_memcpy.c:__copy_to_user() has a mode in which it copies
  "non-atomically" (if faulthandler_disabled() returns 0). If a fault
  happens during __copy_to_user, what prevents some other thread from
  clobbering DACR?

* In uaccess.h:uaccess_save_and_enable(), what prevents a context
  switch between the get_domain and set_domain calls?

Just asking questions, I have no prior experience with this code...

Cheers,
Peter

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Russell King - ARM Linux]

On Thu, Dec 03, 2015 at 04:12:06PM +0000, Peter Rosin wrote:
> Since it seems like a race is at the bottom of the observed problems, I'm
> going to look for things that look racy. The things that stand out to me
> are:
> 
> * uaccess.h:modify_domain() does a read-modify-write on DACR using
>   get_domain and set_domain, and I don't see any locking. Is that
>   safe? Why?

It's safe:
* the DACR is per-CPU
* all exceptions preserve the original DACR value when they return.
  This is done by storing the DACR value at entry onto the stack, along
  with the register set, and restoring it along with the register set
  on exit from exception processing, as if "nothing ever happened".
  This includes if the exception processing caused a switch to another
  thread.

> * uaccess_with_memcpy.c:__copy_to_user() has a mode in which it copies
>   "non-atomically" (if faulthandler_disabled() returns 0). If a fault
>   happens during __copy_to_user, what prevents some other thread from
>   clobbering DACR?

See the second point above.  Moreover, if we sleep in down_read(),
then __switch_to() reads the current DACR value and saves it in the
thread information, and will restore that value when resuming the
thread - even if the thread has been migrated to a different CPU.

> * In uaccess.h:uaccess_save_and_enable(), what prevents a context
>   switch between the get_domain and set_domain calls?

Nothing, but it doesn't matter, because the DACR register is saved
and restored to preserve its value across all exceptions and thread
switches.

I suspect the only way to nail this down is to litter the uaccess
code (virtually every alternate line) with:

	BUG_ON(get_domain() & domain_mask(DOMAIN_USER) ==
	       domain_val(DOMAIN_USER, DOMAIN_NOACCESS));

to narrow down the exact point where the domain register seemingly
gets reset.  Maybe it'll provide some hint.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Russell King - ARM Linux]

On Thu, Dec 03, 2015 at 04:41:18PM +0000, Russell King - ARM Linux wrote:
> On Thu, Dec 03, 2015 at 04:12:06PM +0000, Peter Rosin wrote:
> > * uaccess_with_memcpy.c:__copy_to_user() has a mode in which it copies
> >   "non-atomically" (if faulthandler_disabled() returns 0). If a fault
> >   happens during __copy_to_user, what prevents some other thread from
> >   clobbering DACR?
> 
> See the second point above.  Moreover, if we sleep in down_read(),
> then __switch_to() reads the current DACR value and saves it in the
> thread information, and will restore that value when resuming the
> thread - even if the thread has been migrated to a different CPU.

I thought this was correct, but it isn't - that's what my original solution
did, but I think when Will reviewed it, we decided it wasn't necessary -
and it isn't necessary for every single case with the exception of this
one.  This is exactly what's going wrong: the down_read() in these paths
calls into the scheduler, which switches away.  When we come back, the
DACR value is reset by the other thread to 0x51.

There's a few ways to solve this:

1. Make the thread switching code save and restore the DACR register as
   it would do for domains.  This imposes an overhead on every single
   context switch whether or not we happen to be in this _single_
   troublesome code.  (Patch attached - as there's several, I'm attaching
   them.)

2. Add additional code to the uaccess-with-memcpy stuff to reset the
   DACR value prior to using memcpy() or memset().  (Patch attached.)

3. Make uaccess-with-memcpy depend on !CPU_SW_DOMAINS_PAN (suggested by
   Will)

4. Delete the uaccess-with-memcpy code (also suggested by Will.)

I think the best thing I can do is say... "Discuss amongst yourselves" :)

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
-------------- next part --------------
 arch/arm/kernel/entry-armv.S       | 4 ++--
 arch/arm/kernel/process.c          | 2 +-
 arch/arm/lib/uaccess_with_memcpy.c | 0
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/arm/kernel/entry-armv.S b/arch/arm/kernel/entry-armv.S
index 3ce377f7251f..ae8a3ad763d9 100644
--- a/arch/arm/kernel/entry-armv.S
+++ b/arch/arm/kernel/entry-armv.S
@@ -782,7 +782,7 @@ ENTRY(__switch_to)
  THUMB(	str	lr, [ip], #4		   )
 	ldr	r4, [r2, #TI_TP_VALUE]
 	ldr	r5, [r2, #TI_TP_VALUE + 4]
-#ifdef CONFIG_CPU_USE_DOMAINS
+#if defined(CONFIG_CPU_USE_DOMAINS) || defined(CONFIG_CPU_SW_DOMAIN_PAN)
 	mrc	p15, 0, r6, c3, c0, 0		@ Get domain register
 	str	r6, [r1, #TI_CPU_DOMAIN]	@ Save old domain register
 	ldr	r6, [r2, #TI_CPU_DOMAIN]
@@ -793,7 +793,7 @@ ENTRY(__switch_to)
 	ldr	r8, =__stack_chk_guard
 	ldr	r7, [r7, #TSK_STACK_CANARY]
 #endif
-#ifdef CONFIG_CPU_USE_DOMAINS
+#if defined(CONFIG_CPU_USE_DOMAINS) || defined(CONFIG_CPU_SW_DOMAIN_PAN)
 	mcr	p15, 0, r6, c3, c0, 0		@ Set domain register
 #endif
 	mov	r5, r0
diff --git a/arch/arm/kernel/process.c b/arch/arm/kernel/process.c
index 4adfb46e3ee9..9d80eb20488f 100644
--- a/arch/arm/kernel/process.c
+++ b/arch/arm/kernel/process.c
@@ -229,7 +229,7 @@ copy_thread(unsigned long clone_flags, unsigned long stack_start,
 
 	memset(&thread->cpu_context, 0, sizeof(struct cpu_context_save));
 
-#ifdef CONFIG_CPU_USE_DOMAINS
+#if defined(CONFIG_CPU_USE_DOMAINS) || defined(CONFIG_CPU_SW_DOMAIN_PAN)
 	/*
 	 * Copy the initial value of the domain access control register
 	 * from the current thread: thread->addr_limit will have been
-------------- next part --------------
 arch/arm/kernel/entry-armv.S       | 0
 arch/arm/kernel/process.c          | 0
 arch/arm/lib/uaccess_with_memcpy.c | 7 +++++++
 3 files changed, 7 insertions(+)

diff --git a/arch/arm/lib/uaccess_with_memcpy.c b/arch/arm/lib/uaccess_with_memcpy.c
index d72b90905132..110e3e272583 100644
--- a/arch/arm/lib/uaccess_with_memcpy.c
+++ b/arch/arm/lib/uaccess_with_memcpy.c
@@ -88,6 +88,7 @@ pin_page_for_write(const void __user *_addr, pte_t **ptep, spinlock_t **ptlp)
 static unsigned long noinline
 __copy_to_user_memcpy(void __user *to, const void *from, unsigned long n)
 {
+	unsigned long dacr;
 	int atomic;
 
 	if (unlikely(segment_eq(get_fs(), KERNEL_DS))) {
@@ -98,6 +99,7 @@ __copy_to_user_memcpy(void __user *to, const void *from, unsigned long n)
 	/* the mmap semaphore is taken only if not in an atomic context */
 	atomic = faulthandler_disabled();
 
+	dacr = get_domain();
 	if (!atomic)
 		down_read(&current->mm->mmap_sem);
 	while (n) {
@@ -118,6 +120,7 @@ __copy_to_user_memcpy(void __user *to, const void *from, unsigned long n)
 		if (tocopy > n)
 			tocopy = n;
 
+		set_domain(dacr);
 		memcpy((void *)to, from, tocopy);
 		to += tocopy;
 		from += tocopy;
@@ -153,11 +156,14 @@ arm_copy_to_user(void __user *to, const void *from, unsigned long n)
 static unsigned long noinline
 __clear_user_memset(void __user *addr, unsigned long n)
 {
+	unsigned long dacr;
+
 	if (unlikely(segment_eq(get_fs(), KERNEL_DS))) {
 		memset((void *)addr, 0, n);
 		return 0;
 	}
 
+	dacr = get_domain();
 	down_read(&current->mm->mmap_sem);
 	while (n) {
 		pte_t *pte;
@@ -175,6 +181,7 @@ __clear_user_memset(void __user *addr, unsigned long n)
 		if (tocopy > n)
 			tocopy = n;
 
+		set_domain(dacr);
 		memset((void *)addr, 0, tocopy);
 		addr += tocopy;
 		n -= tocopy;

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Nicolas Pitre]

On Thu, 3 Dec 2015, Russell King - ARM Linux wrote:

> On Thu, Dec 03, 2015 at 04:41:18PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Dec 03, 2015 at 04:12:06PM +0000, Peter Rosin wrote:
> > > * uaccess_with_memcpy.c:__copy_to_user() has a mode in which it copies
> > >   "non-atomically" (if faulthandler_disabled() returns 0). If a fault
> > >   happens during __copy_to_user, what prevents some other thread from
> > >   clobbering DACR?
> > 
> > See the second point above.  Moreover, if we sleep in down_read(),
> > then __switch_to() reads the current DACR value and saves it in the
> > thread information, and will restore that value when resuming the
> > thread - even if the thread has been migrated to a different CPU.
> 
> I thought this was correct, but it isn't - that's what my original solution
> did, but I think when Will reviewed it, we decided it wasn't necessary -
> and it isn't necessary for every single case with the exception of this
> one.  This is exactly what's going wrong: the down_read() in these paths
> calls into the scheduler, which switches away.  When we come back, the
> DACR value is reset by the other thread to 0x51.
> 
> There's a few ways to solve this:
> 
> 1. Make the thread switching code save and restore the DACR register as
>    it would do for domains.  This imposes an overhead on every single
>    context switch whether or not we happen to be in this _single_
>    troublesome code.  (Patch attached - as there's several, I'm attaching
>    them.)
> 
> 2. Add additional code to the uaccess-with-memcpy stuff to reset the
>    DACR value prior to using memcpy() or memset().  (Patch attached.)
> 
> 3. Make uaccess-with-memcpy depend on !CPU_SW_DOMAINS_PAN (suggested by
>    Will)
> 
> 4. Delete the uaccess-with-memcpy code (also suggested by Will.)
> 
> I think the best thing I can do is say... "Discuss amongst yourselves" :)

Personally, I'd advocate for #2 or #4.  Prior commit 0f64b247e6 I was 
already leaning towards #4.

So if some people are still relying on uaccess-with-memcpy and #2 fixes 
it then it's all good.  I'd suggest surrounding the DACR accesses with 
#ifdef CONFIG_CPU_SW_DOMAIN_PAN in the final patch.


Nicolas

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

Russell King wrote:
> On Thu, Dec 03, 2015 at 04:41:18PM +0000, Russell King - ARM Linux wrote:
> > On Thu, Dec 03, 2015 at 04:12:06PM +0000, Peter Rosin wrote:
> > > * uaccess_with_memcpy.c:__copy_to_user() has a mode in which it copies
> > >   "non-atomically" (if faulthandler_disabled() returns 0). If a fault
> > >   happens during __copy_to_user, what prevents some other thread from
> > >   clobbering DACR?
> > 
> > See the second point above.  Moreover, if we sleep in down_read(),
> > then __switch_to() reads the current DACR value and saves it in the
> > thread information, and will restore that value when resuming the
> > thread - even if the thread has been migrated to a different CPU.
> 
> I thought this was correct, but it isn't - that's what my original solution
> did, but I think when Will reviewed it, we decided it wasn't necessary -
> and it isn't necessary for every single case with the exception of this
> one.  This is exactly what's going wrong: the down_read() in these paths
> calls into the scheduler, which switches away.  When we come back, the
> DACR value is reset by the other thread to 0x51.
> 
> There's a few ways to solve this:
> 
> 1. Make the thread switching code save and restore the DACR register as
>    it would do for domains.  This imposes an overhead on every single
>    context switch whether or not we happen to be in this _single_
>    troublesome code.  (Patch attached - as there's several, I'm attaching
>    them.)
> 
> 2. Add additional code to the uaccess-with-memcpy stuff to reset the
>    DACR value prior to using memcpy() or memset().  (Patch attached.)

I took both patches for a quick spin (a dozen boots and one hour uptime
after that for each patch) and no incidents. I have not gathered data,
but the crash on boot feels like it's quite a bit above 50% when there
is a problem so this feels good (I used 5 clean reboots when I bisected
and that worked).

Reported-by: Peter Rosin <peda@axentia.se>
Tested-by: Peter Rosin <peda@axentia.se>

(and please don't forget to cc stable)

> 3. Make uaccess-with-memcpy depend on !CPU_SW_DOMAINS_PAN (suggested by
>    Will)
> 
> 4. Delete the uaccess-with-memcpy code (also suggested by Will.)
> 
> I think the best thing I can do is say... "Discuss amongst yourselves" :)

I have no personal preference on what should be done, I only had
copy_to_user_memcpy enabled since that was what Atmel fed me.

Cheers,
Peter

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Russell King - ARM Linux]

On Thu, Dec 03, 2015 at 01:28:12PM -0500, Nicolas Pitre wrote:
> On Thu, 3 Dec 2015, Russell King - ARM Linux wrote:
> 
> > On Thu, Dec 03, 2015 at 04:41:18PM +0000, Russell King - ARM Linux wrote:
> > > On Thu, Dec 03, 2015 at 04:12:06PM +0000, Peter Rosin wrote:
> > > > * uaccess_with_memcpy.c:__copy_to_user() has a mode in which it copies
> > > >   "non-atomically" (if faulthandler_disabled() returns 0). If a fault
> > > >   happens during __copy_to_user, what prevents some other thread from
> > > >   clobbering DACR?
> > > 
> > > See the second point above.  Moreover, if we sleep in down_read(),
> > > then __switch_to() reads the current DACR value and saves it in the
> > > thread information, and will restore that value when resuming the
> > > thread - even if the thread has been migrated to a different CPU.
> > 
> > I thought this was correct, but it isn't - that's what my original solution
> > did, but I think when Will reviewed it, we decided it wasn't necessary -
> > and it isn't necessary for every single case with the exception of this
> > one.  This is exactly what's going wrong: the down_read() in these paths
> > calls into the scheduler, which switches away.  When we come back, the
> > DACR value is reset by the other thread to 0x51.
> > 
> > There's a few ways to solve this:
> > 
> > 1. Make the thread switching code save and restore the DACR register as
> >    it would do for domains.  This imposes an overhead on every single
> >    context switch whether or not we happen to be in this _single_
> >    troublesome code.  (Patch attached - as there's several, I'm attaching
> >    them.)
> > 
> > 2. Add additional code to the uaccess-with-memcpy stuff to reset the
> >    DACR value prior to using memcpy() or memset().  (Patch attached.)
> > 
> > 3. Make uaccess-with-memcpy depend on !CPU_SW_DOMAINS_PAN (suggested by
> >    Will)
> > 
> > 4. Delete the uaccess-with-memcpy code (also suggested by Will.)
> > 
> > I think the best thing I can do is say... "Discuss amongst yourselves" :)
> 
> Personally, I'd advocate for #2 or #4.  Prior commit 0f64b247e6 I was 
> already leaning towards #4.
> 
> So if some people are still relying on uaccess-with-memcpy and #2 fixes 
> it then it's all good.  I'd suggest surrounding the DACR accesses with 
> #ifdef CONFIG_CPU_SW_DOMAIN_PAN in the final patch.

Last time this was discussed, people were unhappy about removing it
as they were seeing performance advantages with it enabled.  Of course,
that was with it being buggy.

I think at this point, short of deleting it, I'd opt for (2) so the
overhead is attributed to the appropriate place, and not spread across
the entire system.  That should then be the prelude for another round
of performance evaluation to see whether it is still advantageous to
have the uaccess-with-memcpy code before making a final decision whether
to revert (2) and apply (3) instead, or to go with (4).

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Russell King - ARM Linux]

On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> I took both patches for a quick spin (a dozen boots and one hour uptime
> after that for each patch) and no incidents. I have not gathered data,
> but the crash on boot feels like it's quite a bit above 50% when there
> is a problem so this feels good (I used 5 clean reboots when I bisected
> and that worked).
> 
> Reported-by: Peter Rosin <peda@axentia.se>
> Tested-by: Peter Rosin <peda@axentia.se>
> 
> (and please don't forget to cc stable)

I've decided to do a more in-depth fix, so that we also solve the issue
that when we schedule in these down_read()s, we don't leak the permissive
domain register setting into the switched-to context.

Can you test this patch please?  Thanks.

8<====
Subject: [PATCH] ARM: fix uaccess_with_memcpy() with SW_DOMAIN_PAN

The uaccess_with_memcpy() code is currently incompatible with the SW
PAN code: it takes locks within the region that we've changed the DACR,
potentially sleeping as a result.  As we do not save and restore the
DACR across co-operative sleep events, can lead to an incorrect DACR
value later in this code path.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 arch/arm/include/asm/uaccess.h     |  4 ++++
 arch/arm/lib/uaccess_with_memcpy.c | 29 +++++++++++++++++++++++------
 2 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/uaccess.h b/arch/arm/include/asm/uaccess.h
index 8cc85a4ebec2..35c9db857ebe 100644
--- a/arch/arm/include/asm/uaccess.h
+++ b/arch/arm/include/asm/uaccess.h
@@ -510,10 +510,14 @@ __copy_to_user_std(void __user *to, const void *from, unsigned long n);
 static inline unsigned long __must_check
 __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
+#ifndef CONFIG_UACCESS_WITH_MEMCPY
 	unsigned int __ua_flags = uaccess_save_and_enable();
 	n = arm_copy_to_user(to, from, n);
 	uaccess_restore(__ua_flags);
 	return n;
+#else
+	return arm_copy_to_user(to, from, n);
+#endif
 }
 
 extern unsigned long __must_check
diff --git a/arch/arm/lib/uaccess_with_memcpy.c b/arch/arm/lib/uaccess_with_memcpy.c
index d72b90905132..588bbc288396 100644
--- a/arch/arm/lib/uaccess_with_memcpy.c
+++ b/arch/arm/lib/uaccess_with_memcpy.c
@@ -88,6 +88,7 @@ pin_page_for_write(const void __user *_addr, pte_t **ptep, spinlock_t **ptlp)
 static unsigned long noinline
 __copy_to_user_memcpy(void __user *to, const void *from, unsigned long n)
 {
+	unsigned long ua_flags;
 	int atomic;
 
 	if (unlikely(segment_eq(get_fs(), KERNEL_DS))) {
@@ -118,7 +119,9 @@ __copy_to_user_memcpy(void __user *to, const void *from, unsigned long n)
 		if (tocopy > n)
 			tocopy = n;
 
+		ua_flags = uaccess_save_and_enable();
 		memcpy((void *)to, from, tocopy);
+		uaccess_restore(ua_flags);
 		to += tocopy;
 		from += tocopy;
 		n -= tocopy;
@@ -145,14 +148,21 @@ arm_copy_to_user(void __user *to, const void *from, unsigned long n)
 	 * With frame pointer disabled, tail call optimization kicks in
 	 * as well making this test almost invisible.
 	 */
-	if (n < 64)
-		return __copy_to_user_std(to, from, n);
-	return __copy_to_user_memcpy(to, from, n);
+	if (n < 64) {
+		unsigned long ua_flags = uaccess_save_and_enable();
+		n = __copy_to_user_std(to, from, n);
+		uaccess_restore(ua_flags);
+	} else {
+		n = __copy_to_user_memcpy(to, from, n);
+	}
+	return n;
 }
 	
 static unsigned long noinline
 __clear_user_memset(void __user *addr, unsigned long n)
 {
+	unsigned long ua_flags;
+
 	if (unlikely(segment_eq(get_fs(), KERNEL_DS))) {
 		memset((void *)addr, 0, n);
 		return 0;
@@ -175,7 +185,9 @@ __clear_user_memset(void __user *addr, unsigned long n)
 		if (tocopy > n)
 			tocopy = n;
 
+		ua_flags = uaccess_save_and_enable();
 		memset((void *)addr, 0, tocopy);
+		uaccess_restore(ua_flags);
 		addr += tocopy;
 		n -= tocopy;
 
@@ -193,9 +205,14 @@ __clear_user_memset(void __user *addr, unsigned long n)
 unsigned long arm_clear_user(void __user *addr, unsigned long n)
 {
 	/* See rational for this in __copy_to_user() above. */
-	if (n < 64)
-		return __clear_user_std(addr, n);
-	return __clear_user_memset(addr, n);
+	if (n < 64) {
+		unsigned long ua_flags = uaccess_save_and_enable();
+		n = __clear_user_std(addr, n);
+		uaccess_restore(ua_flags);
+	} else {
+		n = __clear_user_memset(addr, n);
+	}
+	return n;
 }
 
 #if 0
-- 
2.1.0

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently@9.6Mbps down 400kbps up
according to speedtest.net.

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

Russell King wrote:
> On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> > I took both patches for a quick spin (a dozen boots and one hour uptime
> > after that for each patch) and no incidents. I have not gathered data,
> > but the crash on boot feels like it's quite a bit above 50% when there
> > is a problem so this feels good (I used 5 clean reboots when I bisected
> > and that worked).
> > 
> > Reported-by: Peter Rosin <peda@axentia.se>
> > Tested-by: Peter Rosin <peda@axentia.se>
> > 
> > (and please don't forget to cc stable)
> 
> I've decided to do a more in-depth fix, so that we also solve the issue
> that when we schedule in these down_read()s, we don't leak the permissive
> domain register setting into the switched-to context.
> 
> Can you test this patch please?  Thanks.

Still looking good.

Cheers,
Peter

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Russell King - ARM Linux]

On Thu, Dec 10, 2015 at 03:29:37PM +0000, Peter Rosin wrote:
> Russell King wrote:
> > On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> > > I took both patches for a quick spin (a dozen boots and one hour uptime
> > > after that for each patch) and no incidents. I have not gathered data,
> > > but the crash on boot feels like it's quite a bit above 50% when there
> > > is a problem so this feels good (I used 5 clean reboots when I bisected
> > > and that worked).
> > > 
> > > Reported-by: Peter Rosin <peda@axentia.se>
> > > Tested-by: Peter Rosin <peda@axentia.se>
> > > 
> > > (and please don't forget to cc stable)
> > 
> > I've decided to do a more in-depth fix, so that we also solve the issue
> > that when we schedule in these down_read()s, we don't leak the permissive
> > domain register setting into the switched-to context.
> > 
> > Can you test this patch please?  Thanks.
> 
> Still looking good.

Does that mean I can add your reported and tested-by to this latest
patch?

Thanks.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

Russell King wrote:
> On Thu, Dec 10, 2015 at 03:29:37PM +0000, Peter Rosin wrote:
> > Russell King wrote:
> > > On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> > > > I took both patches for a quick spin (a dozen boots and one hour 
> > > > uptime after that for each patch) and no incidents. I have not 
> > > > gathered data, but the crash on boot feels like it's quite a bit 
> > > > above 50% when there is a problem so this feels good (I used 5 
> > > > clean reboots when I bisected and that worked).
> > > > 
> > > > Reported-by: Peter Rosin <peda@axentia.se>
> > > > Tested-by: Peter Rosin <peda@axentia.se>
> > > > 
> > > > (and please don't forget to cc stable)
> > > 
> > > I've decided to do a more in-depth fix, so that we also solve the 
> > > issue that when we schedule in these down_read()s, we don't leak the 
> > > permissive domain register setting into the switched-to context.
> > > 
> > > Can you test this patch please?  Thanks.
> > 
> > Still looking good.
> 
> Does that mean I can add your reported and tested-by to this latest patch?

Right, I thought that was obvious, sorry for the confusion.

Cheers,
Peter

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

[I repeat myself just in case my last message disappeared. It would be
 a shame if 4.4 was also regressed because of a missing response.]

I wrote:
> Russell King wrote:
> > On Thu, Dec 10, 2015 at 03:29:37PM +0000, Peter Rosin wrote:
> > > Russell King wrote:
> > > > On Thu, Dec 03, 2015 at 09:37:51PM +0000, Peter Rosin wrote:
> > > > > I took both patches for a quick spin (a dozen boots and one hour 
> > > > > uptime after that for each patch) and no incidents. I have not 
> > > > > gathered data, but the crash on boot feels like it's quite a bit 
> > > > > above 50% when there is a problem so this feels good (I used 5 
> > > > > clean reboots when I bisected and that worked).
> > > > > 
> > > > > Reported-by: Peter Rosin <peda@axentia.se>
> > > > > Tested-by: Peter Rosin <peda@axentia.se>
> > > > > 
> > > > > (and please don't forget to cc stable)
> > > > 
> > > > I've decided to do a more in-depth fix, so that we also solve the 
> > > > issue that when we schedule in these down_read()s, we don't leak 
> > > > the permissive domain register setting into the switched-to context.
> > > > 
> > > > Can you test this patch please?  Thanks.
> > > 
> > > Still looking good.
> > 
> > Does that mean I can add your reported and tested-by to this latest patch?
> 
> Right, I thought that was obvious, sorry for the confusion.

Reported-by: Peter Rosin <peda@axentia.se>
Tested-by: Peter Rosin <peda@axentia.se>

(and please don't forget to cc stable)

Cheers,
Peter

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

I wrote:
> [I repeat myself just in case my last message disappeared. It would be
>  a shame if 4.4 was also regressed because of a missing response.]

Crap, I should have checked one more time before hitting send. The patch
is already in there! Sorry for the confusion.

Cheers,
Peter

Domain faults when CONFIG_CPU_SW_DOMAIN_PAN is enabled

[Peter Rosin]

Hi!

If I enable CONFIG_CPU_SW_DOMAIN_PAN, I sometimes (but not always) get the
following (or very similar) on boot.

Cheers,
Peter

Unhandled fault: page domain fault (0x81b) at 0x00086578
pgd = c2810000
[00086578] *pgd=22819831, *pte=224fe34f, *ppte=224fe83f
Internal error: : 81b [#1] ARM
Modules linked in:
CPU: 0 PID: 907 Comm: ntpd Not tainted 4.3.0+ #29
Hardware name: Atmel SAMA5
task: c398dac0 ti: c2804000 task.ti: c2804000
PC is at memcpy+0x50/0x330
LR is at 0x1
pc : [<c01daff0>]??? lr : [<00000001>]??? psr: 80070013
sp : c2805ed4? ip : 00000007? fp : 000d3808
r10: 00000000? r9 : 00000080? r8 : 00000001
r7 : 00000010? r6 : 00000010? r5 : 003f7374? r4 : 00000000
r3 : 00000002? r2 : ffffffe0? r1 : c2805f38? r0 : 00086578
Flags: Nzcv? IRQs on? FIQs on? Mode SVC_32? ISA ARM? Segment none
Control: 10c53c7d? Table: 22810059? DAC: 00000051
Process ntpd (pid: 907, stack limit = 0xc2804208)
Stack: (0xc2805ed4 to 0xc2806000)
5ec0:????????????????????????????????????????????? 00000000 c2804000 00000000
5ee0: c2805f18 00086578 00086578 c01e76c4 c2819218 c2ff3b74 000d4344 00086578
5f00: 00000000 00000051 0000007c c0010224 c2804000 c004c988 00000002 00000000
5f20: 003f7374 00000010 00000010 00000001 00000007 00000001 01f40000 565ff060
5f40: 000183ef 00002710 00000000 00000000 00000000 00000000 00000000 00000000
5f60: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
5f80: 00000000 00000000 00000000 00000000 00000000 00000000 000d3968 0005e38f
5fa0: 000d4344 c0010060 000d3968 0005e38f 00086578 00000000 ffffffff 00000001
5fc0: 000d3968 0005e38f 000d4344 0000007c 000d3814 00000001 000d4338 000d3808
5fe0: 00081e7c bec558b4 00026d3d b6d09126 00000030 00086578 009e3675 2a090bb6
[<c01daff0>] (memcpy) from [<c01e76c4>] (__copy_to_user_memcpy+0x138/0x17c)
[<c01e76c4>] (__copy_to_user_memcpy) from [<c004c988>] (SyS_adjtimex+0xd4/0xf0)
[<c004c988>] (SyS_adjtimex) from [<c0010060>] (ret_fast_syscall+0x0/0x3c)
Code: f5d1f05c f5d1f07c e8b151f8 e2522020 (e8a051f8)
---[ end trace 04981945a3df2e5e ]---