ARM: Do not call flush_cache_user_range with mmap_sem held

From: Dima Zavin <[email protected]>

We can't be holding the mmap_sem while calling flush_cache_user_range because the flush can fault. If we fault on a user address, the page fault handler will try to take mmap_sem again. Since both places acquire the read lock, most of the time it succeeds. However, if another thread tries to acquire the write lock on the mmap_sem (e.g. mmap) in between the call to flush_cache_user_range and the fault, the down_read in do_page_fault will deadlock.

Also, since we really can't be holding the mmap_sem while calling flush_cache_user_range AND vma is actually unused by the flush itself, get rid of vma as an argument.

Signed-off-by Dima Zavin <[email protected]>

Signed-off-by John Stultz <[email protected]>

Signed-off-by Catalin Marinas <[email protected]>

Russell, this patch has been around for a while but still no decision made, so I'm reposting it. It solves a real bug in the ARM kernels and I'd like to be considered for upstream.

Agreed. Russell, please consider picking this up -- the bug is very real and it sounds like the objection is vague.

Quoted 2 lines · Olof Johansson

Agreed. Russell, please consider picking this up -- the bug is very real and it sounds like the objection is vague.

No, it isn't. ?It's creating an unsafe situation. ?If we're going to do this, we might as well give up on architecture correctness because we're throwing out locking correctness.

1. We look up the VMA.
2. We pass the VMA to the cache operation.
3. The cache operation dereferences the VMA to obtain the VMA flags.

If something else happens to modify the VMA (eg, a concurrent mremap or munmap) then without locking, we could end up dereferencing something that is no longer the VMA we thought we had.

Plus, consider that the VMA list is modifyable while the mmap_sem is not held - do we really want to see the effects of having find_vma() being preempted having loaded a vm_next pointer, another thread coming in, modifying the VMA list (possibly deleting the next vma), that memory being reallocated to something else, and then we resume and fall off the VMA list and possibly OOPS because we've accessed what we thought was the next vm_next pointer but it wasn't?

While we can say "an application should not do that" that's not a reason to ignore this problem and just delete all the locking in the hope that we'll always have well behaving applications.

So, it's *totally* unsafe to delete the locking here.

The requirement is that the mmap_sem must be held while you expect to hold any reference to a vm_area_struct. ?There's no exceptions to that.

Hi,

Quoted 11 lines · Russell King - ARM Linux

Quoted 2 lines · Olof Johansson

Agreed. Russell, please consider picking this up -- the bug is very real and it sounds like the objection is vague.

No, it isn't. ?It's creating an unsafe situation. ?If we're going to do this, we might as well give up on architecture correctness because we're throwing out locking correctness.

1. We look up the VMA.
2. We pass the VMA to the cache operation.
3. The cache operation dereferences the VMA to obtain the VMA flags.

The current implementation doesn't use the flush_cache_user_range per-cpu function, it uses the coherent_user_page:

from arch/arm/include/asm/cacheflush.h:

#define flush_cache_user_range(vma,start,end) \
? ?__cpuc_coherent_user_range((start) & PAGE_MASK, PAGE_ALIGN(end))

Which doesn't take flags. So the vma isn't used at all at the flushing step (which is also why this patch removes the flags being passed in).

Quoted 11 lines · Russell King - ARM Linux

Quoted 2 lines · Olof Johansson

Agreed. Russell, please consider picking this up -- the bug is very real and it sounds like the objection is vague.

No, it isn't. ?It's creating an unsafe situation. ?If we're going to do this, we might as well give up on architecture correctness because we're throwing out locking correctness.

1. We look up the VMA.
2. We pass the VMA to the cache operation.
3. The cache operation dereferences the VMA to obtain the VMA flags.

We also don't continue iterating over the vmas, we only flush for the first one in the range that we find. That is possibly a bug.

I don't remember the details but couple of years ago someone working on Java in ARM pointed out that a cache operation spanning two vmas only flushes a single one. I think the JIT was modified since as I haven't heard back but I would be more in favour of just dropping the vma checks altogether.

Yep, I would agree if we actually needed the vma for the flags, but we don't with the current implementation.

We also don't continue iterating over the vmas, we only flush for the first one in the range that we find. That is possibly a bug.

Quoted 2 lines · Olof Johansson

Yep, I would agree if we actually needed the vma for the flags, but we don't with the current implementation.

That's just one reason. ?I mentioned more than one reason why the locking was necessary in my mail but you seem to have ignored that. ?The second reason is a far more serious problem.

Quoted 2 lines

We also don't continue iterating over the vmas, we only flush for the first one in the range that we find. That is possibly a bug.

No, that's intentional. ?This function is not supposed to be used to invalidate across several mappings. ?It's there to deal with JIT code, which will be written into some allocated memory. ?It is extremely unlikely that JIT code will be written into two consecutive, independently created mappings and a single cache operation requested over both.

What if the two independent mappings are located at wildly different addresses?

Quoted 2 lines · Olof Johansson

Agreed. Russell, please consider picking this up -- the bug is very real and it sounds like the objection is vague.

No, it isn't. It's creating an unsafe situation. If we're going to do this, we might as well give up on architecture correctness because we're throwing out locking correctness.

1. We look up the VMA.
2. We pass the VMA to the cache operation.
3. The cache operation dereferences the VMA to obtain the VMA flags.

BTW, we could even go a step further an remove the vma checks entirely, just use access_ok() since __cpuc_coherent_user_range() can handle unmapped ranges properly (though it may introduce some latency if some user app passes a 3G range but we can change the fixup code to abort the operation when it gets a fault that can't be fixed up).

Quoted 5 lines · Catalin Marinas

BTW, we could even go a step further an remove the vma checks entirely, just use access_ok() since __cpuc_coherent_user_range() can handle unmapped ranges properly (though it may introduce some latency if some user app passes a 3G range but we can change the fixup code to abort the operation when it gets a fault that can't be fixed up).

So, do you think that it is acceptable to be able to pass into this from userspace the arguments '0', '~0', '0' and have the kernel spin over the entire 4G space, including IO space on any of the supported architectures.

Note that pre-ARMv6 CPUs will spin over that range in 32-byte steps whether or not there's a page present.

Quoted 10 lines · Russell King - ARM Linux

Quoted 5 lines · Catalin Marinas

BTW, we could even go a step further an remove the vma checks entirely, just use access_ok() since __cpuc_coherent_user_range() can handle unmapped ranges properly (though it may introduce some latency if some user app passes a 3G range but we can change the fixup code to abort the operation when it gets a fault that can't be fixed up).

So, do you think that it is acceptable to be able to pass into this from userspace the arguments '0', '~0', '0' and have the kernel spin over the entire 4G space, including IO space on any of the supported architectures.

We have access_ok() to check for user only space.

Quoted 2 lines

Note that pre-ARMv6 CPUs will spin over that range in 32-byte steps whether or not there's a page present.

You are right, the pre-v6 hardware would not trigger a fault. So if we don't want another #ifdef in this function, we just keep the vma checks for all architecture versions.

But I consider that the original patch is still valid.

Quoted 17 lines · Catalin Marinas

Quoted 10 lines · Russell King - ARM Linux

Quoted 5 lines · Catalin Marinas

BTW, we could even go a step further an remove the vma checks entirely, just use access_ok() since __cpuc_coherent_user_range() can handle unmapped ranges properly (though it may introduce some latency if some user app passes a 3G range but we can change the fixup code to abort the operation when it gets a fault that can't be fixed up).

So, do you think that it is acceptable to be able to pass into this from userspace the arguments '0', '~0', '0' and have the kernel spin over the entire 4G space, including IO space on any of the supported architectures.

We have access_ok() to check for user only space.

Quoted 2 lines

Note that pre-ARMv6 CPUs will spin over that range in 32-byte steps whether or not there's a page present.

Quoted 5 lines

You are right, the pre-v6 hardware would not trigger a fault. So if we don't want another #ifdef in this function, we just keep the vma checks for all architecture versions.

But I consider that the original patch is still valid.

You thinks it's safe to walk the vma list with no locks held?

Quoted 22 lines · Russell King - ARM Linux

Quoted 21 lines · Catalin Marinas

Quoted 10 lines · Russell King - ARM Linux

Quoted 5 lines · Catalin Marinas

BTW, we could even go a step further an remove the vma checks entirely, just use access_ok() since __cpuc_coherent_user_range() can handle unmapped ranges properly (though it may introduce some latency if some user app passes a 3G range but we can change the fixup code to abort the operation when it gets a fault that can't be fixed up).

So, do you think that it is acceptable to be able to pass into this from userspace the arguments '0', '~0', '0' and have the kernel spin over the entire 4G space, including IO space on any of the supported architectures.

We have access_ok() to check for user only space.

Quoted 2 lines

Note that pre-ARMv6 CPUs will spin over that range in 32-byte steps whether or not there's a page present.

You are right, the pre-v6 hardware would not trigger a fault. So if we don't want another #ifdef in this function, we just keep the vma checks for all architecture versions.

Quoted 3 lines

Quoted 1 line

But I consider that the original patch is still valid.

You thinks it's safe to walk the vma list with no locks held?

No, I don't, but the patch only releases the semaphore when calling the cache flushing function. The vmas are walked with the semaphore held.

Please read the patch I posted properly.

Quoted 40 lines · Catalin Marinas

Quoted 38 lines · Russell King - ARM Linux

Quoted 34 lines · Catalin Marinas

Quoted 12 lines · Russell King - ARM Linux

Quoted 8 lines · Catalin Marinas

BTW, we could even go a step further an remove the vma checks entirely,
just use access_ok() since __cpuc_coherent_user_range() can handle unmapped ranges properly (though it may introduce some latency if some user app passes a 3G range but we can change the fixup code to abort the
operation when it gets a fault that can't be fixed up).

Quoted 6 lines

So, do you think that it is acceptable to be able to pass into this from
userspace the arguments '0', '~0', '0' and have the kernel spin over the
entire 4G space, including IO space on any of the supported architectures.

We have access_ok() to check for user only space.

Quoted 2 lines

Note that pre-ARMv6 CPUs will spin over that range in 32-byte steps whether or not there's a page present.

You are right, the pre-v6 hardware would not trigger a fault. So if we don't want another #ifdef in this function, we just keep the vma checks for all architecture versions.

But I consider that the original patch is still valid.

Quoted 7 lines

Quoted 1 line

You thinks it's safe to walk the vma list with no locks held?

No, I don't, but the patch only releases the semaphore when calling the cache flushing function. The vmas are walked with the semaphore held.

Please read the patch I posted properly.

In an other thread

http://lists.arm.linux.org.uk/lurker/message/20120406.033509.ca9fe8cf.en.html

it seems that we have an additional user where this patch fixed an issue.

So would it be possible to talk about this patch, again?

Quoted 2 lines · Dirk Behme

So would it be possible to talk about this patch, again?

This patch had plenty of talk already. :-) What it needs is for someone to go in and annotate (and thus partially rewrite) the pre-v6 cacheflush loops with exception fixups, so that the mmap_sem can be safely dropped.

As mentioned above, v6 and v7 are already annotated -- so a bad passed-in pointer will just cause a fault, fixup and signal to the application like other architectures where the flush can be done from userspace, or any other bad pointer passed into a syscall.

Quoted 9 lines · Olof Johansson

This patch had plenty of talk already. :-) What it needs is for someone to go in and annotate (and thus partially rewrite) the pre-v6 cacheflush loops with exception fixups, so that the mmap_sem can be safely dropped.

As mentioned above, v6 and v7 are already annotated -- so a bad passed-in pointer will just cause a fault, fixup and signal to the application like other architectures where the flush can be done from userspace, or any other bad pointer passed into a syscall.

Are you sure about this? It looks to me like the v6 implementation just skips over faulting pages and the v7 implementation gives up after the first faulting page and just returns 0 (despite the comment suggesting otherwise).

If we want to send a signal, we need to rework do_cache_op slightly and fix up all of the low-level cache accessors. We also need to decide whether we continue past a faulting page or give up at that point. Alternatively, we could return -EFAULT from the syscall instead of sending a signal.

From adbb915ab1ef37d7c66f6d48761d481fe6361f32 Mon Sep 17 00:00:00 2001

... and here's a patch on top of the original which lets us return errors to userspace from the cacheflush syscall.

Quoted 1 line

From adbb915ab1ef37d7c66f6d48761d481fe6361f32 Mon Sep 17 00:00:00 2001

From: Will Deacon <[email protected]>
Date: Wed, 18 Apr 2012 10:50:15 +0100
Subject: [PATCH] ARM: cacheflush: return error to userspace when flushing syscall fails

The cacheflush syscall can fail for two reasons:

(1) The arguments are invalid (nonsensical address range or no VMA)

(2) The region generates a translation fault on a VIPT or PIPT cache

Quoted 2 lines · Will Deacon

... and here's a patch on top of the original which lets us return errors to userspace from the cacheflush syscall.

Is this worth it? No one's checking for errors as it stands.

Quoted 3 lines

The cacheflush syscall can fail for two reasons:

(1) The arguments are invalid (nonsensical address range or no VMA)

That's a good reason...

Quoted 1 line

(2) The region generates a translation fault on a VIPT or PIPT cache

But I don't think this is, for two reasons:

1. Remember that the purpose of this operation is to allow userspace to write code into pages and then execute that code - so all that we're concerned about in this call is to ensure that the I/D caches for the memory region are synchronized.

2. We'll fault at the cache operation on v6 and v7 because the page was not present, and that will make the system try and bring the page back into the memory space. Only if that fails (eg, because we're running low on memory) will we fail to replace the page, and use the exception handling.

I think the original intention for (2) was that the page would merely get skipped and we'd move onto the next page without trying to bring the previous page back into the memory space - this won't work because of the way the page fault handler works. If we want that behaviour, we need to use pagefault_disable()..pagefault_enable() around the cache flushing to ensure that the page fault handler does not try to replace the missing page, but instead immediately calls the fixup function.

Now, the fact is that 99% of userspace will not be checking the return value of this syscall (they can't, because the return value has never been defined.) They can't even start to check the return value, because they may fail on older kernels which don't. So all round, this is a bad idea.

Just treat this the same way that the 'prefetch' instruction does - ignore errors, and continue. If you're going to be calling this function on a region which you haven't just written code to (and therefore should exist) then you're doing something wrong in userspace.

Quoted 3 lines · Will Deacon

The cacheflush syscall can fail for two reasons:

(1) The arguments are invalid (nonsensical address range or no VMA)

That's a good reason...

Quoted 1 line

(2) The region generates a translation fault on a VIPT or PIPT cache

But I don't think this is, for two reasons:

1. Remember that the purpose of this operation is to allow userspace to write code into pages and then execute that code - so all that we're concerned about in this call is to ensure that the I/D caches for the memory region are synchronized.

2. We'll fault at the cache operation on v6 and v7 because the page was not present, and that will make the system try and bring the page back into the memory space.

Only if that fails (eg, because we're running
low on memory) will we fail to replace the page, and use the exception handling.

I think the original intention for (2) was that the page would merely get skipped and we'd move onto the next page without trying to bring the previous page back into the memory space - this won't work because of the way the page fault handler works. If we want that behaviour, we need to use pagefault_disable()..pagefault_enable() around the cache flushing to ensure that the page fault handler does not try to replace the missing page, but instead immediately calls the fixup function.

Quoted 4 lines · Dirk Behme

In an other thread

http://lists.arm.linux.org.uk/lurker/message/20120406.033509.ca9fe8cf.en.html

Quoted 3 lines

it seems that we have an additional user where this patch fixed an issue.

So would it be possible to talk about this patch, again?

This patch had plenty of talk already. :-) What it needs is for someone to go in and annotate (and thus partially rewrite) the pre-v6 cacheflush loops with exception fixups, so that the mmap_sem can be safely dropped.

As mentioned above, v6 and v7 are already annotated -- so a bad passed-in pointer will just cause a fault, fixup and signal to the application like other architectures where the flush can be done from userspace, or any other bad pointer passed into a syscall.

From my perspective, this patch is still valid (it keeps the find_vma()