arm kernel oops in highmem.c with 4.2

[linux@arm.linux.org.uk (Russell King - ARM Linux)]

On Tue, Aug 11, 2015 at 01:48:10PM -0400, Mark Salter wrote:
> On Wed, 2015-08-05 at 12:27 +0100, Russell King - ARM Linux wrote:
> > It helps if I look at 4.2 rather than an older kernel :)
> > 
> > However, I've checked that I have DEBUG_HIGHMEM enabled, which I do, and
> > I'm unable to reproduce this here.  My kernels are built with gcc 4.7.4.
> > 
> > What it looks like from your oops is that the address which was passed
> > in was 0xffedf000, but the address we calculated via the following for
> > the current index was 0xfff00000:
> > 
> > type = kmap_atomic_idx();
> > idx = type + KM_TYPE_NR * smp_processor_id();
> > __fix_to_virt(idx)
> > 
> > Doing a bit of maths... the address 0xffedf000 corresponds to a fixmap
> > index of... (0xffeff000 - 0xffedf000) >> 12 = 32.  KM_TYPE_NR is 16 on
> > ARM, so the mapping was created by CPU 2, and type was zero.
> > 
> > On unmap, 0xfff00000 gives... (0xffeff000 - 0xfff00000) >> 12 = -1.
> > That suggests we're on CPU 0, and type is -1 - in other words, there
> > are no atomically mapped mappings on CPU 0.
> > 
> > Since kmap_atomic() disables preemption and page faults, how did your
> > kernel migrate this thread from CPU 2 to CPU 0... and I can't see how
> > that happened.
> > 
> 
> The fedora kernel is using PREEMPT_VOLUNTARY with !PREEMPT and
> !PREEMPT_CPOUNT. So preempt_disable() is a nop. I added some code
> to catch the kernel scheduling between kmap_atomic() and
> kunmap_atomic() and got this straightaway:

Looking at the backtrace, and grepping for __copy_to_user_memcpy, it
seems to imply that you're using the uaccess-with-memcpy code.

This code is relatively unmaintained, and probably mostly unused by
people today, so I doubt it gets much in the way of testing - and
you've certainly found a bug in there.

        /* the mmap semaphore is taken only if not in an atomic context */
        atomic = in_atomic();

        if (!atomic)
                down_read(&current->mm->mmap_sem);

is not sufficient to tell whether we can take the semaphore.

We _could_ replace the above with:

	int ret;

	ret = down_read_trylock(&current->mm->mmap_sem);
	if (!ret) {
		__copy_to_user_std(to, from, n);
		return;
	}

but that's just a guess.  I'm not a big fan of this code, and given
that it probably doesn't get much use, we may be better off deleting
it so it doesn't sit around rotting...  Code like this really needs
regular testing.

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