acquire mmap semaphore in pagemap_read.

fs/proc/task_mmu.c static ssize_t pagemap_read(struct file · line 721

Quoted 9 lines · Martin Schwidefsky

fs/proc/task_mmu.c +2 -0

@@ -716,7 +716,9 @@ static ssize_t pagemap_read(struct file 
 	 * user buffer is tracked in "pm", and the walk
 	 * will stop when we hit the end of the buffer.
 	 */
+	down_read(&mm->mmap_sem);
 	ret = walk_page_range(start_vaddr, end_vaddr, &pagemap_walk);
+	up_read(&mm->mmap_sem);

This will introduce "put_user under mmap_sem" which is deadlockable.

Quoted 12 lines · On Thu, 12 Mar 2009 14:45:33 +0300 Alexey Dobriyan <[email protected]> wrote:

fs/proc/task_mmu.c static ssize_t pagemap_read(struct file · line 721

Quoted 9 lines · Martin Schwidefsky

fs/proc/task_mmu.c +2 -0

@@ -716,7 +716,9 @@ static ssize_t pagemap_read(struct file 
 	 * user buffer is tracked in "pm", and the walk
 	 * will stop when we hit the end of the buffer.
 	 */
+	down_read(&mm->mmap_sem);
 	ret = walk_page_range(start_vaddr, end_vaddr, &pagemap_walk);
+	up_read(&mm->mmap_sem);

This will introduce "put_user under mmap_sem" which is deadlockable.

Hmm, interesting. In this case the pagemap interface is fundamentally broken.

Quoted 16 lines · Martin Schwidefsky

Quoted 12 lines · On Thu, 12 Mar 2009 14:45:33 +0300 Alexey Dobriyan <[email protected]> wrote:

fs/proc/task_mmu.c static ssize_t pagemap_read(struct file · line 721

Quoted 9 lines · Martin Schwidefsky

fs/proc/task_mmu.c +2 -0

@@ -716,7 +716,9 @@ static ssize_t pagemap_read(struct file 
 	 * user buffer is tracked in "pm", and the walk
 	 * will stop when we hit the end of the buffer.
 	 */
+	down_read(&mm->mmap_sem);
 	ret = walk_page_range(start_vaddr, end_vaddr, &pagemap_walk);
+	up_read(&mm->mmap_sem);

This will introduce "put_user under mmap_sem" which is deadlockable.

Quoted 1 line

Hmm, interesting. In this case the pagemap interface is fundamentally broken.

Well it means we may have to reintroduce the very annoying double buffering from various earlier implementations. But let's leave this discussion until after we've figured out what to do about the walker code.

Quoted 15 lines · On Thu, 12 Mar 2009 10:23:34 -0500 Matt Mackall <[email protected]> wrote:

Quoted 14 lines · Martin Schwidefsky

Quoted 11 lines · On Thu, 12 Mar 2009 14:45:33 +0300 Alexey Dobriyan <[email protected]> wrote:

fs/proc/task_mmu.c static ssize_t pagemap_read(struct file · line 721

Quoted 9 lines · Martin Schwidefsky

fs/proc/task_mmu.c +2 -0

@@ -716,7 +716,9 @@ static ssize_t pagemap_read(struct file 
 	 * user buffer is tracked in "pm", and the walk
 	 * will stop when we hit the end of the buffer.
 	 */
+	down_read(&mm->mmap_sem);
 	ret = walk_page_range(start_vaddr, end_vaddr, &pagemap_walk);
+	up_read(&mm->mmap_sem);

This will introduce "put_user under mmap_sem" which is deadlockable.

Quoted 5 lines

Quoted 1 line

Hmm, interesting. In this case the pagemap interface is fundamentally broken.

Well it means we may have to reintroduce the very annoying double buffering from various earlier implementations. But let's leave this discussion until after we've figured out what to do about the walker code.

Which would be really ugly. I still have not grasped why this will introduce a deadlock though. The worst the put_user can do is to cause a page fault, no? I do not see where the fault handler acquires the mmap_sem as writer. It takes the mmap_sem as reader and two readers should be fine.

Quoted 5 lines

Which would be really ugly. I still have not grasped why this will introduce a deadlock though. The worst the put_user can do is to cause a page fault, no? I do not see where the fault handler acquires the mmap_sem as writer. It takes the mmap_sem as reader and two readers should be fine.

Somebody else can acquire for write in the meantime, for instance another thread doing mprotect. This writer is blocked by the first reader, and the second reader is blocked by the writer. So both tasks are blocked.

Well it means we may have to reintroduce the very annoying double buffering from various earlier implementations. But let's leave this discussion until after we've figured out what to do about the walker code.

Quoted 4 lines · On Thu, 12 Mar 2009 10:23:34 -0500 Matt Mackall <[email protected]> wrote:

Well it means we may have to reintroduce the very annoying double buffering from various earlier implementations. But let's leave this discussion until after we've figured out what to do about the walker code.

About the walker code. I've realized that there is another way to fix this. The TASK_SIZE definition is currently used for two things: 1) as a maximum mappable address, 2) the size of the address space for a process. And there lies a problem: while a process is using a reduced page table 1) and 2) differ. If I make TASK_SIZE give you the current size of the address space then it is not possible to mmap an object beyond 4TB and the page table upgrade never happens. If I make TASK_SIZE return the maximum mappable address the page table walker breaks. The solution could be to introduce MAX_TASK_SIZE and use that in the mmap code to find out what can be mapped.

Quoted 8 lines · On Thu, 12 Mar 2009 16:54:51 +0100 Martin Schwidefsky <[email protected]>…

Quoted 4 lines · On Thu, 12 Mar 2009 10:23:34 -0500 Matt Mackall <[email protected]> wrote:

Well it means we may have to reintroduce the very annoying double buffering from various earlier implementations. But let's leave this discussion until after we've figured out what to do about the walker code.

Quoted 10 lines

About the walker code. I've realized that there is another way to fix this. The TASK_SIZE definition is currently used for two things: 1) as a maximum mappable address, 2) the size of the address space for a process. And there lies a problem: while a process is using a reduced page table 1) and 2) differ. If I make TASK_SIZE give you the current size of the address space then it is not possible to mmap an object beyond 4TB and the page table upgrade never happens. If I make TASK_SIZE return the maximum mappable address the page table walker breaks. The solution could be to introduce MAX_TASK_SIZE and use that in the mmap code to find out what can be mapped.

I got around the TASK_SIZE checks with arch code only. In total I'll need two fixes, one that makes TASK_SIZE to reflect the size of the current address space and another one to get the page table upgrades working again. I'll push these patches via git390 as no common code changes are required.

Which leaves the mmap_sem issue as the only problem left.