Do not call flush_cache_user_range with mmap_sem held
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
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.
--- arch/arm/include/asm/cacheflush.h | 2 +- arch/arm/kernel/traps.c | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-)
arch/arm/include/asm/cacheflush.h +1 -1
@@ -336,7 +336,7 @@ extern void flush_cache_page(struct vm_area_struct *vma, unsigned long user_addr* Harvard caches are synchronised for the user space address range. * This is used for the ARM private sys_cacheflush system call. */ -#define flush_cache_user_range (vma,start,end) \ +#define flush_cache_user_range (start,end) \ __cpuc_coherent_user_range((start) & PAGE_MASK, PAGE_ALIGN(end))/*
arch/arm/kernel/traps.c +3 -1
@@ -453,7 +453,9 @@ do_cache_op(unsigned long start, unsigned long end, int flags)if (end > vma->vm_end) end = vma->vm_end; -flush_cache_user_range (vma, start, end); +up_read (&mm->mmap_sem); +flush_cache_user_range (start, end); +return ; }up_read (&mm->mmap_sem); }
Signature · 1 line
1.6.6
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
This patch is probably not as efficient as it could be since the flush will end up faulting in pages the user never touched. I think we should print a warning regarding that and skip over the unmapped pages, I just wasn't sure how to do that correctly.
--Dima
2010/4/28 Dima Zavin <[email protected]>:
Quoted 52 lines
We can't be holding the
mmap_semwhile callingflush_cache_user_rangebecause the flush can fault. If we fault on a user address, the page fault handler will try to takemmap_semagain. Since both places acquire the read lock, most of the time it succeeds. However, if another thread tries to acquire the write lock on themmap_sem(e.g. mmap) in between the call toflush_cache_user_rangeand the fault, thedown_readindo_page_faultwill deadlock.Also, since we really can't be holding the
mmap_semwhile callingflush_cache_user_rangeAND vma is actually unused by the flush itself, get rid of vma as an argument.Signed-off-by Dima Zavin <[email protected]>Cc Russell King <[email protected]>Cc Arve Hj?nnev?g <[email protected]>---
?arch/arm/include/asm/cacheflush.h | ? ?2 +-
?arch/arm/kernel/traps.c ? ? ? ? ? | ? ?4 +++-
?2 files changed, 4 insertions(+), 2 deletions(-)arch/arm/include/asm/cacheflush.h +0
-0@@ -336,7 +336,7 @@ extern void flush_cache_page(struct vm_area_struct *vma, unsigned long user_addr?* Harvard caches are synchronised for the user space address range. ?* This is used for the ARM private
sys_cacheflushsystem call. ?*/arch/arm/include/asm/cacheflush.h +1
-1-#define flush_cache_user_range (vma,start,end) \ +#define flush_cache_user_range (start,end) \? ? ? ?__cpuc_coherent_user_range((start) &
PAGE_MASK,PAGE_ALIGN(end))?/*
arch/arm/kernel/traps.c +3
-2@@ -453,7 +453,9 @@ do_cache_op(unsigned long start, unsigned long end, int flags) ? ? ? ? ? ? ? ?if (end > vma->vm_end) ? ? ? ? ? ? ? ? ? ? ? ?end = vma->vm_end; - ? ? ? ? ? ? ? flush_cache_user_range(vma, start, end); + ? ? ? ? ? ? ? up_read(&mm->mmap_sem); + ? ? ? ? ? ? ? flush_cache_user_range(start, end); + ? ? ? ? ? ? ? return; ? ? ? ?} ? ? ? ?up_read(&mm->mmap_sem); ?} --1.6.6
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
In reply to Dima ZavinQuoted 7 lines · Dima Zavin
We can't be holding the
mmap_semwhile callingflush_cache_user_rangebecause the flush can fault. If we fault on a user address, the page fault handler will try to takemmap_semagain. Since both places acquire the read lock, most of the time it succeeds. However, if another thread tries to acquire the write lock on themmap_sem(e.g. mmap) in between the call toflush_cache_user_rangeand the fault, thedown_readindo_page_faultwill deadlock.
That's a non-issue. If do_cache_op is holding a read lock, _nothing_ _else_ can be holding that lock in write mode. So, holding the lock in read mode ensures that when faults occur, the fault handler can be sure that its read lock will succeed.
Other threads trying to get a write lock will be prevented from doing so until the entire cache flush operation has completed, which ensures that the mapping can not be altered via munmap/mmap - and that is completely desirable behaviour.
Please explain why you think this leads to a deadlock.
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Dima Zavin
We can't be holding the
mmap_semwhile callingflush_cache_user_rangebecause the flush can fault. If we fault on a user address, the page fault handler will try to takemmap_semagain. Since both places acquire the read lock, most of the time it succeeds. However, if another thread tries to acquire the write lock on themmap_sem(e.g. mmap) in between the call toflush_cache_user_rangeand the fault, thedown_readindo_page_faultwill deadlock.
Quoted 4 lines
That's a non-issue. If
do_cache_opis holding a read lock, _nothing_ _else_ can be holding that lock in write mode. So, holding the lock in read mode ensures that when faults occur, the fault handler can be sure that its read lock will succeed.
read-write locks will block a reader when there is a already blocked writer. Otherwise the writer can be permanently starved due to new readers always arriving so reader count doesn't reach zero.
In other words, readers queue behind writers if there's a waiting writer.
-- Jamie
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
Quoted 10 lines · Jamie Lokier
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Dima Zavin
We can't be holding the
mmap_semwhile callingflush_cache_user_rangebecause the flush can fault. If we fault on a user address, the page fault handler will try to takemmap_semagain. Since both places acquire the read lock, most of the time it succeeds. However, if another thread tries to acquire the write lock on themmap_sem(e.g. mmap) in between the call toflush_cache_user_rangeand the fault, thedown_readindo_page_faultwill deadlock.
Quoted 8 lines
Quoted 4 lines
That's a non-issue. If
do_cache_opis holding a read lock, _nothing_ _else_ can be holding that lock in write mode. So, holding the lock in read mode ensures that when faults occur, the fault handler can be sure that its read lock will succeed.read-write locks will block a reader when there is a already blocked writer. Otherwise the writer can be permanently starved due to new readers always arriving so reader count doesn't reach zero.
Hmm, true, and rather unfortunate.
As I've already said, we can not do this cache maintainence outside of the lock.
The point of this code is to first validate that the region we're working on is valid. As soon as we drop the lock, we lose the guarantee that the region is valid while we operate on it - indeed, the region could be unmapped and remapped by a different thread.
I think the only reasonable solution is to also walk the page tables and do the cache handling on a per-page basis, which will make this interface quite a bit slower - but that's the price we pay for correctness.
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
Jamie/Russell,
Thanks for the feedback.
Quoted 3 lines
Quoted 3 lines
read-write locks will block a reader when there is a already blocked writer. ?Otherwise the writer can be permanently starved due to new readers always arriving so reader count doesn't reach zero.
That's precisely the problem.
Quoted 9 lines
Hmm, true, and rather unfortunate.
As I've already said, we can not do this cache maintainence outside of the lock.
The point of this code is to first validate that the region we're working on is valid. ?As soon as we drop the lock, we lose the guarantee that the region is valid while we operate on it - indeed, the region could be unmapped and remapped by a different thread.
So what if it was remapped? The worst case scenario in this case is that we needlessly flush a region of memory, but its not "wrong". It can't be any worse than just doing a full cache flush. If another thread unmapped the region, then we should (and will) segfault the flushing thread, which is the correct behavior IMHO.
--Dima
Quoted 7 lines
I think the only reasonable solution is to also walk the page tables and do the cache handling on a per-page basis, which will make this interface quite a bit slower - but that's the price we pay for correctness.
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
Quoted 10 lines
Quoted 4 lines
The point of this code is to first validate that the region we're working on is valid. ?As soon as we drop the lock, we lose the guarantee that the region is valid while we operate on it - indeed, the region could be unmapped and remapped by a different thread.
So what if it was remapped? The worst case scenario in this case is that we needlessly flush a region of memory, but its not "wrong". It can't be any worse than just doing a full cache flush. If another thread unmapped the region, then we should (and will) segfault the flushing thread, which is the correct behavior IMHO.
ping.
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
Quoted 5 lines · Dima Zavin
Quoted 5 lines
Quoted 4 lines
The point of this code is to first validate that the region we're working on is valid. ?As soon as we drop the lock, we lose the guarantee that the region is valid while we operate on it - indeed, the region could be unmapped and remapped by a different thread.
Quoted 7 lines
Quoted 5 lines
So what if it was remapped? The worst case scenario in this case is that we needlessly flush a region of memory, but its not "wrong". It can't be any worse than just doing a full cache flush. If another thread unmapped the region, then we should (and will) segfault the flushing thread, which is the correct behavior IMHO.
ping.
I still do not think this is a good idea for the reasons I've already mentioned.
PATCH 1/1 Do not call flush_cache_user_range with mmap_semheld
In reply to Russell King - ARM LinuxQuoted 22 lines · Russell King - ARM Linux
Quoted 10 lines · Jamie Lokier
Quoted 9 lines · Russell King - ARM Linux
Quoted 7 lines · Dima Zavin
We can't be holding the
mmap_semwhile callingflush_cache_user_rangebecause the flush can fault. If we fault on a user address, the page fault handler will try to takemmap_semagain. Since both places acquire the read lock, most of the time it succeeds. However, if another thread tries to acquire the write lock on themmap_sem(e.g. mmap) in between the call toflush_cache_user_rangeand the fault, thedown_readindo_page_faultwill deadlock.Quoted 8 lines
Quoted 4 lines
That's a non-issue. If
do_cache_opis holding a read lock, _nothing_ _else_ can be holding that lock in write mode. So, holding the lock in read mode ensures that when faults occur, the fault handler can be sure that its read lock will succeed.read-write locks will block a reader when there is a already blocked writer. Otherwise the writer can be permanently starved due to new readers always arriving so reader count doesn't reach zero.
Hmm, true, and rather unfortunate.
Quoted 7 lines
As I've already said, we can not do this cache maintainence outside of the lock.
The point of this code is to first validate that the region we're working on is valid. As soon as we drop the lock, we lose the guarantee that the region is valid while we operate on it - indeed, the region could be unmapped and remapped by a different thread.
The flush_cache_user_range operation cannot actually damage the data. If the application is so badly written that one of its threads remaps a page range while another thread writes to it and flushes the caches, then it deserves the memory corruption.
Personally, I would go even further and remove the find_vma() call (of course with an access_ok() call to make sure the address isn't a kernel one). I actually did some tests but the performance improvement was too significant to be worth arguing the case on the list. But the app I was using was a simple test where the vma tree was small. Things may be different for a fully featured Java VM for example.
Signature · 1 line
Catalin
PATCH 1/1 Do not call flush_cache_user_range with mmap_sem held
In reply to Dima ZavinQuoted 8 lines · Dima Zavin
We can't be holding the
mmap_semwhile callingflush_cache_user_rangebecause the flush can fault. If we fault on a user address, the page fault handler will try to takemmap_semagain. Since both places acquire the read lock, most of the time it succeeds. However, if another thread tries to acquire the write lock on themmap_sem(e.g. mmap) in between the call toflush_cache_user_rangeand the fault, thedown_readindo_page_faultwill deadlock.
Quoted 7 lines
Also, since we really can't be holding the
mmap_semwhile callingflush_cache_user_rangeAND vma is actually unused by the flush itself, get rid of vma as an argument.Signed-off-by Dima Zavin <[email protected]>Cc Russell King <[email protected]>Cc Arve Hj?nnev?g <[email protected]>
FWIW (and since I added the USER() macros to the *_coherent_user_range() functions - commit 32cfb1b1), my view is that this is a safe patch (of course, Russell has to agree with this as well):
Signature · 1 line
Catalin
PATCH 1/1 Do not call flush_cache_user_range with mmap_semheld
In reply to Catalin MarinasQuoted 4 lines · Catalin Marinas
The
flush_cache_user_rangeoperation cannot actually damage the data. If the application is so badly written that one of its threads remaps a page range while another thread writes to it and flushes the caches, then it deserves the memory corruption.
It may deserve corruption, but doing corruption silently is cruel.
Moreover, calling mprotect(PROT_READ) in one thread while another thread is writing to the same regions is a valid, and used, garbage collector dirty-tracking technique. (Page faults provide the info, and the fault handler uses PROT_WRITE to let the faulting thread continue on each tracked page.)
Is it possible to percolate EFAULT to the right places when the cache flush faults?
Quoted 6 lines
Personally, I would go even further and remove the
find_vma()call (of course with anaccess_ok()call to make sure the address isn't a kernel one). I actually did some tests but the performance improvement was too significant to be worth arguing the case on the list. But the app I was using was a simple test where the vma tree was small. Things may be different for a fully featured Java VM for example.
Seems a reasonable thing to do, and a fully-featured use-lots-of-VMAs app sounds like the sort of app which wants to flush part of caches quickly.
-- Jamie
PATCH 1/1 Do not call flush_cache_user_range with mmap_semheld
Quoted 8 lines · Jamie Lokier
Quoted 4 lines · Catalin Marinas
The
flush_cache_user_rangeoperation cannot actually damage the data. If the application is so badly written that one of its threads remaps a page range while another thread writes to it and flushes the caches, then it deserves the memory corruption.It may deserve corruption, but doing corruption silently is cruel.
Quoted 8 lines
Moreover, calling
mprotect(PROT_READ)in one thread while another thread is writing to the same regions is a valid, and used, garbage collector dirty-tracking technique. (Page faults provide the info, and the fault handler usesPROT_WRITEto let the faulting thread continue on each tracked page.)Is it possible to percolate EFAULT to the right places when the cache flush faults?
Scratch that idea. How about just doing a full mm (address-independent) cache flush if a fault occurs?
Performance doesn't matter (it's not a normal situation), and it's better than corruption (no week-long debugging session surprise).
-- Jamie
PATCH 1/1 Do not call flush_cache_user_range with mmap_semheld
In reply to Jamie LokierQuoted 6 lines · Jamie Lokier
Quoted 4 lines · Catalin Marinas
The
flush_cache_user_rangeoperation cannot actually damage the data. If the application is so badly written that one of its threads remaps a page range while another thread writes to it and flushes the caches, then it deserves the memory corruption.
Quoted 7 lines
It may deserve corruption, but doing corruption silently is cruel.
Moreover, calling
mprotect(PROT_READ)in one thread while another thread is writing to the same regions is a valid, and used, garbage collector dirty-tracking technique. (Page faults provide the info, and the fault handler usesPROT_WRITEto let the faulting thread continue on each tracked page.)
For this kind of situation, mprotect() could make the page not accessible for a brief period of time (but with the mm semaphore held). A coherent_user_range call would fault and do_page_fault() handles it correctly by restarting the cache line operation once the PTE is reinstated. So I don't think the app would miss any cache flushing operation.
Quoted 2 lines
Is it possible to percolate EFAULT to the right places when the cache flush faults?
If the cache flush faults because the vma is no longer there (though a few cycles before there was one), the kernel calls __do_kernel_fault() which tries to fix up the exception by calling the 9001 label in cache-v6/v7.S. This label just tries the following page ignoring the cache flushing operation.
That's a place where we can return -EFAULT from v6/v7_coherent_user_range and change do_cache_op() to make use of this value.
But that's a special case when one thread of an application unmaps an address range while another thread writes to it.
Quoted 3 lines
Quoted 3 lines
Personally, I would go even further and remove the
find_vma()call (of course with anaccess_ok()call to make sure the address isn't a kernel one). I actually did some tests but the performance improvement was too
BTW, I meant "was not too" above.
Quoted 7 lines
Quoted 3 lines
significant to be worth arguing the case on the list. But the app I was using was a simple test where the vma tree was small. Things may be different for a fully featured Java VM for example.
Seems a reasonable thing to do, and a fully-featured use-lots-of-VMAs app sounds like the sort of app which wants to flush part of caches quickly.
Signature · 1 line
Catalin
PATCH 1/1 Do not call flush_cache_user_range with mmap_semheld
In reply to Jamie LokierQuoted 15 lines · Jamie Lokier
Quoted 14 lines · Jamie Lokier
Quoted 4 lines · Catalin Marinas
The
flush_cache_user_rangeoperation cannot actually damage the data. If the application is so badly written that one of its threads remaps a page range while another thread writes to it and flushes the caches, then it deserves the memory corruption.It may deserve corruption, but doing corruption silently is cruel.
Moreover, calling
mprotect(PROT_READ)in one thread while another thread is writing to the same regions is a valid, and used, garbage collector dirty-tracking technique. (Page faults provide the info, and the fault handler usesPROT_WRITEto let the faulting thread continue on each tracked page.)
Quoted 8 lines
Quoted 2 lines
Is it possible to percolate EFAULT to the right places when the cache flush faults?
Scratch that idea. How about just doing a full mm (address-independent) cache flush if a fault occurs?
Performance doesn't matter (it's not a normal situation), and it's better than corruption (no week-long debugging session surprise).
I already replied on this specific case. I don't think we get any corruption with the current implementation (and coherent_user_range called outside the mm semaphore).
What if we had user-accessible cache flushing operations (well, a subset of them)? We wouldn't have been able to take any semaphore and I'm sure JIT people would have made use of them.
Signature · 1 line
Catalin