Re: Deadlock on the mm->mmap_sem

Re: Deadlock on the mm->mmap_sem

[Studierende der Universitaet des Saarlandes]

> On Thu, Sep 20, 2001 at 09:01:13AM +0100, David Howells wrote:
> > 
> > Andrea Arcangeli <andrea@suse.de> wrote:
> > > the process doesn't need to lock multiple mm_structs at the same time.
> > 
> > fork, ptrace, /proc/pid/mem, /proc/pid/maps
> >

David, coredump is the only difficult recursive user of mmap_sem.
ptrace & /proc/pid/mem double buffer into kernel buffers, fork just
doesn't lock the new mm_struct - it's new, noone can get a pointer to it
before it's linked into the various lists.

> for /proc/<pid>/maps this check takes care of it of course (or it could
> get unfair again: only when we're faulting on our vm we're allowed to go
> through):
> 
>         if (task == current)
>                 down_read_recursive(&mm->mmap_sem, &current->mm_recursor);
>         else
>                 down_read(&mm->mmap_sem);
> 
Andrea, my rewrite of proc_pid_read_maps fixes that without any ugly
recursive/nonrecursive tests.

Short summary of the possible fixes for the deadlock:

* A simple unfair mmap_sem (rw_lock like) is not possible.
* Copying the mm_struct is ugly.
* A fair, recursive mmap_sem (a task that already owns the mmap_sem can
acquire it again without deadlocking, all other cases are fair). That's
what Andrea proposes. (Andrea, is that correct?)
* moving the locking into each coredump handler. The main advantage is
that for some coredump handlers down_read is not enough - e.g.
elf_core_dump should call down_write to prevent concurrent expand_stack
calls, and acquire the pagetable_lock around some lines (right now it
walks the page tables without locking). I'll check the other coredump
handlers - during a quick check I couldn't find any oopsable races if
only a read lock is taken.

I'll write a patch that moves the locking into the coredump handlers,
then we can compare that with Andrea's proposal.

--
	Manfred

Re: Deadlock on the mm->mmap_sem

[David Howells]

> David, coredump is the only difficult recursive user of mmap_sem.  ptrace &
> /proc/pid/mem double buffer into kernel buffers, fork just doesn't lock the
> new mm_struct - it's new, noone can get a pointer to it before it's linked
> into the various lists.

Yes, you're right. So what you and Andrea are proposing is to have a field in
the task struct that counts the number of active readlocks you hold on your
own mm_struct. If this is >0, then you can add another readlock to it. If this
is the case, then you can add an extra asm-rwsem operation that simply
increments the semaphore counter. BUT you can only use this operation if you
_know_ you already have a readlock. And as you know that some function higher
up the stack holds the lock, you can guarantee that the lock isn't going to go
away.

Give me a few minutes, and I can handle this:-)

David

[PATCH] Make same-process recursive mm_struct access possible

[David Howells]

[-- Attachment #1: Type: text/plain, Size: 1556 bytes --]

Based on an idea by Andrea Arcangeli, this patch wraps the rw_semaphore on the
mm_struct (to force you to use special locking operations to access it) and
adds a counter to the task_struct (which counts the number of active read
locks you hold on _your_ mm_struct). The four new ops are:

	mm_lock_shared(struct mm_struct *);
	mm_unlock_shared(struct mm_struct *);
	mm_lock_exclusive(struct mm_struct *);
	mm_unlock_exclusive(struct mm_struct *);

 * The exclusive ops map directly to down_write() and up_write().

 * The unlock-shared op decrements the current mm readlock counter if the mm
   unlocked is the current mm; finally it calls up_read().

 * The lock-shared op does one of the following two things:

   * If the mm being locked is the current mm, and if the current readlock
     counter is greater than zero, then it calls down_read_recursive() and
     then increments the current readlock counter.

   * Otherwise, it just calls down_read() and then increments the current
     readlock counter.

down_read_recursive() is a new rw-semaphore operation. It simply adjusts the
semaphore to record an extra active readlock. As specified in the comments
associated with it, it MUST ONLY be called if you know that there is already
an active read lock on the semaphore. Note that it never needs to wait; given
the preconditions for calling it, it knows it can always succeed immediately.

This patch makes no changes to existing rw-semaphore operations. They are
still fair. It simply makes mm_struct locks fair and capable of recursion.

David


[-- Attachment #2: rwsem+mmlock.diff --]
[-- Type: application/octet-stream, Size: 10319 bytes --]

Re: Deadlock on the mm->mmap_sem

[Andrea Arcangeli]

On Thu, Sep 20, 2001 at 10:57:08AM +0000, Studierende der Universitaet des Saarlandes wrote:
> * A fair, recursive mmap_sem (a task that already owns the mmap_sem can
> acquire it again without deadlocking, all other cases are fair). That's
> what Andrea proposes. (Andrea, is that correct?)

Exactly.

> elf_core_dump should call down_write to prevent concurrent expand_stack

expand_stack doesn't need the write sem, see the locking comments in the
00_silent-stack-overflow patch in -aa.

> calls, and acquire the pagetable_lock around some lines (right now it
> walks the page tables without locking). I'll check the other coredump

Also expand_stack needs the page_table_lock, that's ok.

> I'll write a patch that moves the locking into the coredump handlers,
> then we can compare that with Andrea's proposal.

Ok.

Andrea

Re: Deadlock on the mm->mmap_sem

[Manfred Spraul]

[-- Attachment #1: Type: text/plain, Size: 1645 bytes --]

Andrea Arcangeli wrote:
> > elf_core_dump should call down_write to prevent concurrent expand_stack
> 
> expand_stack doesn't need the write sem, see the locking comments in the
> 00_silent-stack-overflow patch in -aa.
>
You misunderstood me, it's the other way around:
elf_core_dump walks the vma list twice and assumes that the segment
sizes don't change --> elf_core_dump needs a write lock to ensure that.

> > calls, and acquire the pagetable_lock around some lines (right now it
> > walks the page tables without locking). I'll check the other coredump
> 
> Also expand_stack needs the page_table_lock, that's ok.
>
Dito: elf_core_dump walks the page tables without locking.

> > I'll write a patch that moves the locking into the coredump handlers,
> > then we can compare that with Andrea's proposal.
> 
> Ok.
> 
Attached is a beta version:

* remove {down,up}_read from fs/exec.c::do_coredump
* add down_read into each arch/*/process.c::dump_thread. Most of them
are probably save without the locking, but it's better for the
consistency. Most of them access mm->brk and perform some calculations.
* explicit memset(,0,) around all structures that are dumped in
fs/binfmt_elf.c: depending on the structure alignment, we could
otherwise leak kernel stack.
* Do not walk the vma list twice, copy it into a temporary kernel
buffer. down_write around it to prevent concurrent expand_stack.
* spin_lock(&current->mm->page_table_lock) around the code that walks
the page tables.
* all other binfmt's have trivial coredump implementation that only call
dump_thread, or no coredump at all.

I'll do more extensive testing tomorrow.

--
	Manfred

[-- Attachment #2: patch-coredump --]
[-- Type: text/plain, Size: 15801 bytes --]

// $Header$
// Kernel Version:
//  VERSION = 2
//  PATCHLEVEL = 4
//  SUBLEVEL = 10
//  EXTRAVERSION =-pre12
diff -ur 2.4/fs/binfmt_elf.c build-2.4/fs/binfmt_elf.c
--- 2.4/fs/binfmt_elf.c	Wed Sep 19 22:39:35 2001
+++ build-2.4/fs/binfmt_elf.c	Thu Sep 20 22:48:52 2001
@@ -31,6 +31,7 @@
 #include <linux/init.h>
 #include <linux/highuid.h>
 #include <linux/smp_lock.h>
+#include <linux/vmalloc.h>
 
 #include <asm/uaccess.h>
 #include <asm/param.h>
@@ -895,22 +896,22 @@
  *
  * I think we should skip something. But I am not sure how. H.J.
  */
-static inline int maydump(struct vm_area_struct *vma)
+static inline int maydump(unsigned long flags)
 {
 	/*
 	 * If we may not read the contents, don't allow us to dump
 	 * them either. "dump_write()" can't handle it anyway.
 	 */
-	if (!(vma->vm_flags & VM_READ))
+	if (!(flags & VM_READ))
 		return 0;
 
 	/* Do not dump I/O mapped devices! -DaveM */
-	if (vma->vm_flags & VM_IO)
+	if (flags & VM_IO)
 		return 0;
 #if 1
-	if (vma->vm_flags & (VM_WRITE|VM_GROWSUP|VM_GROWSDOWN))
+	if (flags & (VM_WRITE|VM_GROWSUP|VM_GROWSDOWN))
 		return 1;
-	if (vma->vm_flags & (VM_READ|VM_EXEC|VM_EXECUTABLE|VM_SHARED))
+	if (flags & (VM_READ|VM_EXEC|VM_EXECUTABLE|VM_SHARED))
 		return 0;
 #endif
 	return 1;
@@ -967,6 +968,7 @@
 {
 	struct elf_note en;
 
+	memset(&en,0,sizeof(en));
 	en.n_namesz = strlen(men->name);
 	en.n_descsz = men->datasz;
 	en.n_type = men->type;
@@ -989,6 +991,46 @@
 #define DUMP_SEEK(off)	\
 	if (!dump_seek(file, (off))) \
 		goto end_coredump;
+
+struct elf_dumpinfo {
+	unsigned long start;
+	size_t len;
+	unsigned long flags;
+};
+
+static struct elf_dumpinfo * alloc_dumpinfo(int segs)
+{
+	int len = sizeof(struct elf_dumpinfo)*segs;
+	if (len < PAGE_SIZE)
+		return kmalloc(len, GFP_KERNEL);
+	return vmalloc(len);
+}
+
+void free_dumpinfo(struct elf_dumpinfo * ptr, int segs)
+{
+	int len = sizeof(struct elf_dumpinfo)*segs;
+	if (len < PAGE_SIZE)
+		return kfree(ptr);
+	return vfree(ptr);
+}
+
+static struct elf_dumpinfo *get_dumpinfo(void)
+{
+	int i;
+	struct vm_area_struct *vma;
+	struct elf_dumpinfo *di = alloc_dumpinfo(current->mm->map_count);
+	if (!di)
+		return NULL;
+
+	vma = current->mm->mmap;
+	for(i = 0, vma = current->mm->mmap; vma != NULL; i++,vma = vma->vm_next) {
+		di[i].start = vma->vm_start;
+		di[i].len =  vma->vm_end - vma->vm_start;
+		di[i].flags = vma->vm_flags;
+	}
+	if (i != current->mm->map_count) BUG();
+	return di;
+}
 /*
  * Actual dumper
  *
@@ -1003,7 +1045,6 @@
 	int segs;
 	size_t size = 0;
 	int i;
-	struct vm_area_struct *vma;
 	struct elfhdr elf;
 	off_t offset = 0, dataoff;
 	unsigned long limit = current->rlim[RLIMIT_CORE].rlim_cur;
@@ -1012,19 +1053,26 @@
 	struct elf_prstatus prstatus;	/* NT_PRSTATUS */
 	elf_fpregset_t fpu;		/* NT_PRFPREG */
 	struct elf_prpsinfo psinfo;	/* NT_PRPSINFO */
+	struct elf_dumpinfo *di;
 
+	/* stop all vm operations, including expand_stack */
+	down_write(&current->mm->mmap_sem);
 	segs = current->mm->map_count;
+	di = get_dumpinfo();
+	up_write(&current->mm->mmap_sem);
+	if (!di)
+		return 0;
 
 #ifdef DEBUG
 	printk("elf_core_dump: %d segs %lu limit\n", segs, limit);
 #endif
 
 	/* Set up header */
+	memset(&elf, 0, sizeof(elf));
 	memcpy(elf.e_ident, ELFMAG, SELFMAG);
 	elf.e_ident[EI_CLASS] = ELF_CLASS;
 	elf.e_ident[EI_DATA] = ELF_DATA;
 	elf.e_ident[EI_VERSION] = EV_CURRENT;
-	memset(elf.e_ident+EI_PAD, 0, EI_NIDENT-EI_PAD);
 
 	elf.e_type = ET_CORE;
 	elf.e_machine = ELF_ARCH;
@@ -1156,42 +1204,34 @@
 		for(i = 0; i < numnote; i++)
 			sz += notesize(&notes[i]);
 
+		memset(&phdr, 0, sizeof(phdr));
 		phdr.p_type = PT_NOTE;
 		phdr.p_offset = offset;
-		phdr.p_vaddr = 0;
-		phdr.p_paddr = 0;
 		phdr.p_filesz = sz;
-		phdr.p_memsz = 0;
-		phdr.p_flags = 0;
-		phdr.p_align = 0;
 
 		offset += phdr.p_filesz;
 		DUMP_WRITE(&phdr, sizeof(phdr));
-	}
 
-	/* Page-align dumped data */
-	dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
+		/* Page-align dumped data */
+		dataoff = offset = roundup(offset, ELF_EXEC_PAGESIZE);
 
-	/* Write program headers for segments dump */
-	for(vma = current->mm->mmap; vma != NULL; vma = vma->vm_next) {
-		struct elf_phdr phdr;
-		size_t sz;
+		/* Write program headers for segments dump */
+		for(i = 0;i < segs; i++) {
 
-		sz = vma->vm_end - vma->vm_start;
+			phdr.p_type = PT_LOAD;
+			phdr.p_offset = offset;
+			phdr.p_vaddr = di[i].start;
+			phdr.p_paddr = 0;
+			phdr.p_filesz = maydump(di[i].flags) ? di[i].len : 0;
+			phdr.p_memsz = di[i].len;
+			offset += phdr.p_filesz;
+			phdr.p_flags = di[i].flags & VM_READ ? PF_R : 0;
+			if (di[i].flags & VM_WRITE) phdr.p_flags |= PF_W;
+			if (di[i].flags & VM_EXEC) phdr.p_flags |= PF_X;
+			phdr.p_align = ELF_EXEC_PAGESIZE;
 
-		phdr.p_type = PT_LOAD;
-		phdr.p_offset = offset;
-		phdr.p_vaddr = vma->vm_start;
-		phdr.p_paddr = 0;
-		phdr.p_filesz = maydump(vma) ? sz : 0;
-		phdr.p_memsz = sz;
-		offset += phdr.p_filesz;
-		phdr.p_flags = vma->vm_flags & VM_READ ? PF_R : 0;
-		if (vma->vm_flags & VM_WRITE) phdr.p_flags |= PF_W;
-		if (vma->vm_flags & VM_EXEC) phdr.p_flags |= PF_X;
-		phdr.p_align = ELF_EXEC_PAGESIZE;
-
-		DUMP_WRITE(&phdr, sizeof(phdr));
+			DUMP_WRITE(&phdr, sizeof(phdr));
+		}
 	}
 
 	for(i = 0; i < numnote; i++)
@@ -1202,29 +1242,29 @@
 
 	DUMP_SEEK(dataoff);
 
-	for(vma = current->mm->mmap; vma != NULL; vma = vma->vm_next) {
+	for(i = 0;i < segs; i++) {
 		unsigned long addr;
 
-		if (!maydump(vma))
+		if (!maydump(di[i].flags))
 			continue;
 #ifdef DEBUG
 		printk("elf_core_dump: writing %08lx %lx\n", addr, len);
 #endif
-		for (addr = vma->vm_start;
-		     addr < vma->vm_end;
-		     addr += PAGE_SIZE) {
+		for (addr = di[i].start; addr < di[i].start+di[i].len; addr += PAGE_SIZE) {
 			pgd_t *pgd;
 			pmd_t *pmd;
-			pte_t *pte;
-
-			pgd = pgd_offset(vma->vm_mm, addr);
+			pte_t pte;
+			
+			spin_lock(&current->mm->page_table_lock);
+			pgd = pgd_offset(current->mm, addr);
 			if (pgd_none(*pgd))
 				goto nextpage_coredump;
 			pmd = pmd_offset(pgd, addr);
 			if (pmd_none(*pmd))
 				goto nextpage_coredump;
-			pte = pte_offset(pmd, addr);
-			if (pte_none(*pte)) {
+			pte = *pte_offset(pmd, addr);
+			spin_unlock(&current->mm->page_table_lock);
+			if (pte_none(pte)) {
 nextpage_coredump:
 				DUMP_SEEK (file->f_pos + PAGE_SIZE);
 			} else {
@@ -1241,6 +1281,7 @@
 
  end_coredump:
 	set_fs(fs);
+	free_dumpinfo(di, segs);
 	return has_dumped;
 }
 #endif		/* USE_ELF_CORE_DUMP */
diff -ur 2.4/fs/exec.c build-2.4/fs/exec.c
--- 2.4/fs/exec.c	Wed Sep 19 22:39:35 2001
+++ build-2.4/fs/exec.c	Thu Sep 20 19:41:24 2001
@@ -969,9 +969,7 @@
 	if (do_truncate(file->f_dentry, 0) != 0)
 		goto close_fail;
 
-	down_read(&current->mm->mmap_sem);
 	retval = binfmt->core_dump(signr, regs, file);
-	up_read(&current->mm->mmap_sem);
 
 close_fail:
 	filp_close(file, NULL);
diff -ur 2.4/arch/alpha/kernel/process.c build-2.4/arch/alpha/kernel/process.c
--- 2.4/arch/alpha/kernel/process.c	Wed Sep 19 22:39:31 2001
+++ build-2.4/arch/alpha/kernel/process.c	Thu Sep 20 21:26:54 2001
@@ -344,6 +344,7 @@
 	struct switch_stack * sw = ((struct switch_stack *) pt) - 1;
 
 	dump->magic = CMAGIC;
+	down_read(&current->mm->mmap_sem);
 	dump->start_code  = current->mm->start_code;
 	dump->start_data  = current->mm->start_data;
 	dump->start_stack = rdusp() & ~(PAGE_SIZE - 1);
@@ -353,6 +354,7 @@
 			 >> PAGE_SHIFT);
 	dump->u_ssize = (current->mm->start_stack - dump->start_stack
 			 + PAGE_SIZE-1) >> PAGE_SHIFT;
+	up_read(&current->mm->mmap_sem);
 
 	/*
 	 * We store the registers in an order/format that is
diff -ur 2.4/arch/arm/kernel/process.c build-2.4/arch/arm/kernel/process.c
--- 2.4/arch/arm/kernel/process.c	Wed Sep 19 22:39:31 2001
+++ build-2.4/arch/arm/kernel/process.c	Thu Sep 20 21:27:45 2001
@@ -339,11 +339,13 @@
 	struct task_struct *tsk = current;
 
 	dump->magic = CMAGIC;
+	down_read(&tsk->mm->mmap_sem);
 	dump->start_code = tsk->mm->start_code;
 	dump->start_stack = regs->ARM_sp & ~(PAGE_SIZE - 1);
 
 	dump->u_tsize = (tsk->mm->end_code - tsk->mm->start_code) >> PAGE_SHIFT;
 	dump->u_dsize = (tsk->mm->brk - tsk->mm->start_data + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	up_read(&tsk->mm->mmap_sem);
 	dump->u_ssize = 0;
 
 	dump->u_debugreg[0] = tsk->thread.debug.bp[0].address;
diff -ur 2.4/arch/cris/kernel/process.c build-2.4/arch/cris/kernel/process.c
--- 2.4/arch/cris/kernel/process.c	Wed Sep 19 22:39:31 2001
+++ build-2.4/arch/cris/kernel/process.c	Thu Sep 20 21:28:38 2001
@@ -227,8 +227,10 @@
 	dump->magic = CMAGIC;
 	dump->start_code = 0;
 	dump->start_stack = regs->esp & ~(PAGE_SIZE - 1);
+	down_read(&current->mm->mmap_sem);
 	dump->u_tsize = ((unsigned long) current->mm->end_code) >> PAGE_SHIFT;
 	dump->u_dsize = ((unsigned long) (current->mm->brk + (PAGE_SIZE-1))) >> PAGE_SHIFT;
+	up_read(&current->mm->mmap_sem);
 	dump->u_dsize -= dump->u_tsize;
 	dump->u_ssize = 0;
 	for (i = 0; i < 8; i++)
diff -ur 2.4/arch/i386/kernel/process.c build-2.4/arch/i386/kernel/process.c
--- 2.4/arch/i386/kernel/process.c	Wed Sep 19 22:39:31 2001
+++ build-2.4/arch/i386/kernel/process.c	Thu Sep 20 19:47:58 2001
@@ -611,8 +611,10 @@
 	dump->magic = CMAGIC;
 	dump->start_code = 0;
 	dump->start_stack = regs->esp & ~(PAGE_SIZE - 1);
+	down_read(&current->mm->mmap_sem);
 	dump->u_tsize = ((unsigned long) current->mm->end_code) >> PAGE_SHIFT;
 	dump->u_dsize = ((unsigned long) (current->mm->brk + (PAGE_SIZE-1))) >> PAGE_SHIFT;
+	up_read(&current->mm->mmap_sem);
 	dump->u_dsize -= dump->u_tsize;
 	dump->u_ssize = 0;
 	for (i = 0; i < 8; i++)
diff -ur 2.4/arch/m68k/kernel/process.c build-2.4/arch/m68k/kernel/process.c
--- 2.4/arch/m68k/kernel/process.c	Wed Sep 19 22:39:31 2001
+++ build-2.4/arch/m68k/kernel/process.c	Thu Sep 20 21:29:11 2001
@@ -291,9 +291,11 @@
 	dump->magic = CMAGIC;
 	dump->start_code = 0;
 	dump->start_stack = rdusp() & ~(PAGE_SIZE - 1);
+	down_read(&current->mm->mmap_sem);
 	dump->u_tsize = ((unsigned long) current->mm->end_code) >> PAGE_SHIFT;
 	dump->u_dsize = ((unsigned long) (current->mm->brk +
 					  (PAGE_SIZE-1))) >> PAGE_SHIFT;
+	up_read(&current->mm->mmap_sem);
 	dump->u_dsize -= dump->u_tsize;
 	dump->u_ssize = 0;
 
diff -ur 2.4/arch/mips/kernel/process.c build-2.4/arch/mips/kernel/process.c
--- 2.4/arch/mips/kernel/process.c	Wed Sep 19 22:39:31 2001
+++ build-2.4/arch/mips/kernel/process.c	Thu Sep 20 21:29:34 2001
@@ -140,6 +140,7 @@
 void dump_thread(struct pt_regs *regs, struct user *dump)
 {
 	dump->magic = CMAGIC;
+	down_read(&current->mm->mmap_sem);
 	dump->start_code  = current->mm->start_code;
 	dump->start_data  = current->mm->start_data;
 	dump->start_stack = regs->regs[29] & ~(PAGE_SIZE - 1);
@@ -147,6 +148,7 @@
 	dump->u_dsize = (current->mm->brk + (PAGE_SIZE - 1) - dump->start_data) >> PAGE_SHIFT;
 	dump->u_ssize =
 		(current->mm->start_stack - dump->start_stack + PAGE_SIZE - 1) >> PAGE_SHIFT;
+	up_read(&current->mm->mmap_sem);
 	memcpy(&dump->regs[0], regs, sizeof(struct pt_regs));
 	memcpy(&dump->regs[EF_SIZE/4], &current->thread.fpu, sizeof(current->thread.fpu));
 }
diff -ur 2.4/arch/mips64/kernel/process.c build-2.4/arch/mips64/kernel/process.c
--- 2.4/arch/mips64/kernel/process.c	Fri Feb 23 15:24:13 2001
+++ build-2.4/arch/mips64/kernel/process.c	Thu Sep 20 21:29:51 2001
@@ -133,6 +133,7 @@
 void dump_thread(struct pt_regs *regs, struct user *dump)
 {
 	dump->magic = CMAGIC;
+	down_read(&current->mm->mmap_sem);
 	dump->start_code  = current->mm->start_code;
 	dump->start_data  = current->mm->start_data;
 	dump->start_stack = regs->regs[29] & ~(PAGE_SIZE - 1);
@@ -142,6 +143,7 @@
 	                >> PAGE_SHIFT;
 	dump->u_ssize = (current->mm->start_stack - dump->start_stack +
 	                 PAGE_SIZE - 1) >> PAGE_SHIFT;
+	up_read(&current->mm->mmap_sem);
 	memcpy(&dump->regs[0], regs, sizeof(struct pt_regs));
 	memcpy(&dump->regs[EF_SIZE/4], &current->thread.fpu,
 	       sizeof(current->thread.fpu));
diff -ur 2.4/arch/s390/kernel/process.c build-2.4/arch/s390/kernel/process.c
--- 2.4/arch/s390/kernel/process.c	Fri Aug 17 18:24:46 2001
+++ build-2.4/arch/s390/kernel/process.c	Thu Sep 20 21:30:14 2001
@@ -415,10 +415,12 @@
 	dump->magic = CMAGIC;
 	dump->start_code = 0;
 	dump->start_stack = regs->gprs[15] & ~(PAGE_SIZE - 1);
+	down_read(&current->mm->mmap_sem);
 	dump->u_tsize = ((unsigned long) current->mm->end_code) >> PAGE_SHIFT;
 	dump->u_dsize = ((unsigned long) (current->mm->brk + (PAGE_SIZE-1))) >> PAGE_SHIFT;
 	dump->u_dsize -= dump->u_tsize;
 	dump->u_ssize = 0;
+	up_read(&current->mm->mmap_sem);
 	if (dump->start_stack < TASK_SIZE)
 		dump->u_ssize = ((unsigned long) (TASK_SIZE - dump->start_stack)) >> PAGE_SHIFT;
 	memcpy(&dump->regs.gprs[0],regs,sizeof(s390_regs));
diff -ur 2.4/arch/s390x/kernel/process.c build-2.4/arch/s390x/kernel/process.c
--- 2.4/arch/s390x/kernel/process.c	Fri Aug 17 18:24:46 2001
+++ build-2.4/arch/s390x/kernel/process.c	Thu Sep 20 21:30:29 2001
@@ -409,10 +409,12 @@
 	dump->magic = CMAGIC;
 	dump->start_code = 0;
 	dump->start_stack = regs->gprs[15] & ~(PAGE_SIZE - 1);
+	down_read(&current->mm->mmap_sem);
 	dump->u_tsize = ((unsigned long) current->mm->end_code) >> PAGE_SHIFT;
 	dump->u_dsize = ((unsigned long) (current->mm->brk + (PAGE_SIZE-1))) >> PAGE_SHIFT;
 	dump->u_dsize -= dump->u_tsize;
 	dump->u_ssize = 0;
+	up_read(&current->mm->mmap_sem);
 	if (dump->start_stack < TASK_SIZE)
 		dump->u_ssize = ((unsigned long) (TASK_SIZE - dump->start_stack)) >> PAGE_SHIFT;
 	memcpy(&dump->regs.gprs[0],regs,sizeof(s390_regs));
diff -ur 2.4/arch/sh/kernel/process.c build-2.4/arch/sh/kernel/process.c
--- 2.4/arch/sh/kernel/process.c	Wed Sep 19 22:39:32 2001
+++ build-2.4/arch/sh/kernel/process.c	Thu Sep 20 21:30:47 2001
@@ -236,6 +236,7 @@
 void dump_thread(struct pt_regs * regs, struct user * dump)
 {
 	dump->magic = CMAGIC;
+	down_read(&current->mm->mmap_sem);
 	dump->start_code = current->mm->start_code;
 	dump->start_data  = current->mm->start_data;
 	dump->start_stack = regs->regs[15] & ~(PAGE_SIZE - 1);
@@ -243,6 +244,7 @@
 	dump->u_dsize = (current->mm->brk + (PAGE_SIZE-1) - dump->start_data) >> PAGE_SHIFT;
 	dump->u_ssize = (current->mm->start_stack - dump->start_stack +
 			 PAGE_SIZE - 1) >> PAGE_SHIFT;
+	up_read(&current->mm->mmap_sem);
 	/* Debug registers will come here. */
 
 	dump->regs = *regs;
diff -ur 2.4/arch/sparc/kernel/process.c build-2.4/arch/sparc/kernel/process.c
--- 2.4/arch/sparc/kernel/process.c	Fri Feb 23 15:24:18 2001
+++ build-2.4/arch/sparc/kernel/process.c	Thu Sep 20 21:31:28 2001
@@ -581,9 +581,11 @@
 	/* fuck me plenty */
 	memcpy(&dump->regs.regs[0], &regs->u_regs[1], (sizeof(unsigned long) * 15));
 	dump->uexec = current->thread.core_exec;
+	down_read(&current->mm->mmap_sem);
 	dump->u_tsize = (((unsigned long) current->mm->end_code) -
 		((unsigned long) current->mm->start_code)) & ~(PAGE_SIZE - 1);
 	dump->u_dsize = ((unsigned long) (current->mm->brk + (PAGE_SIZE-1)));
+	up_read(&current->mm->mmap_sem);
 	dump->u_dsize -= dump->u_tsize;
 	dump->u_dsize &= ~(PAGE_SIZE - 1);
 	first_stack_page = (regs->u_regs[UREG_FP] & ~(PAGE_SIZE - 1));
diff -ur 2.4/arch/sparc64/kernel/process.c build-2.4/arch/sparc64/kernel/process.c
--- 2.4/arch/sparc64/kernel/process.c	Sat Jul  7 13:05:52 2001
+++ build-2.4/arch/sparc64/kernel/process.c	Thu Sep 20 21:35:32 2001
@@ -699,9 +699,11 @@
 	dump->regs.y = regs->y;
 	/* fuck me plenty */
 	memcpy(&dump->regs.regs[0], &regs->u_regs[1], (sizeof(unsigned long) * 15));
+	down_read(&current->mm->mmap_sem);
 	dump->u_tsize = (((unsigned long) current->mm->end_code) -
 		((unsigned long) current->mm->start_code)) & ~(PAGE_SIZE - 1);
 	dump->u_dsize = ((unsigned long) (current->mm->brk + (PAGE_SIZE-1)));
+	up_read(&current->mm->mmap_sem);
 	dump->u_dsize -= dump->u_tsize;
 	dump->u_dsize &= ~(PAGE_SIZE - 1);
 	first_stack_page = (regs->u_regs[UREG_FP] & ~(PAGE_SIZE - 1));

Re: Deadlock on the mm->mmap_sem

[Manfred Spraul]

Andrea Arcangeli wrote:
> 
> > I'll write a patch that moves the locking into the coredump handlers,
> > then we can compare that with Andrea's proposal.
> 
> Ok.
>
I've changed my mind:

Modifying the mmap_sem is a better solution for 2.4 than integrating the
locking into elf_core_dump.

My patch copies the vm areas into a list (under down_write()) and calls
up_write(), but I found 2 races:
* the kernel must not touch VM_IO memory. Another thread could call
"munmap(), mmap(,VM_IO)".
* If another thread calls munmap(), my coredump handler would abort
dumping due to the resulting pagefault.

The proper solution would be using a page table walker in elf_core_dump
(similar to access_process_vm()), everything under down_write().

But that would be a large rewrite. I'm aware of at least 4 users who
want such a page table walker: map_user_kiobuf, access_process_vm,
singlecopy pipe (not merged), elf_core_dump.

--
	Manfred