arm kernel oops in highmem.c with 4.2

[Nicolas Pitre]

On Tue, 11 Aug 2015, Russell King - ARM Linux wrote:

> On Tue, Aug 11, 2015 at 03:41:52PM -0400, Nicolas Pitre wrote:
> > I'd agree.  But first I'd like to know why the fedora kernel is using 
> > CONFIG_UACCESS_WITH_MEMCPY?  If it's just because it sounded cool then 
> > that's a bad reason.
> > 
> > That code was created to work around inneficiency in the Orion CPU core 
> > that didn't coalesce writes from STRT instructions, and by using plain 
> > STR and/or STM the actual throughput was more than doubled.  This was 
> > fixed in later cores. However Orion users (if they still exist) might 
> > like the added performance. I don't have Orion-based targets anymore 
> > (they took the way of the recycling facility a while ago).
> 
> Irrespective of that, what has been found is that the implementation is
> unsafe - it is taking a semaphore when page faults are disabled.  In
> other words, notwithstanding the above, it's buggy no matter if it's
> an Orion CPU core, or highmem, or what.  Any place in the kernel which
> uses pagefault_disable() and then calls into this code will be buggy.
> 
> It needs fixing or removing.

Absolutely. I'm not disputing that. I'm only asking so we can wisely 
choose between fixing or removing.  Personally I'd be inclined towards 
the later, unless the following is sufficient to fix it:

diff --git a/arch/arm/lib/uaccess_with_memcpy.c b/arch/arm/lib/uaccess_with_memcpy.c
index 3e58d71001..4b39af2dfd 100644
--- a/arch/arm/lib/uaccess_with_memcpy.c
+++ b/arch/arm/lib/uaccess_with_memcpy.c
@@ -96,7 +96,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 = in_atomic();
+	atomic = faulthandler_disabled();
 
 	if (!atomic)
 		down_read(&current->mm->mmap_sem);

> Even if it is fixed, making it _depend_ on CPU_FEROCEON sounds like a
> good idea if non-orion stuff isn't supposed to be enabling it.  Or
> something like that.

It is not clear to me exactly which cores are affected.  That's why the 
Kconfig entry was left open to all, in case it could benefit others.
In theory it shouldn't be harmful to anyone notwithstanding the caveat
mentioned in the help text.


Nicolas