multithreaded coredumps for elf exeecutables for O(1) scheduler

PATCH Manfred Spraul 3 participants 4 messages May 10, 2002 – May 13, 2002

Subsystems (2)

VFS exec and binfmt

lore

Share & tools

lore

Manfred Spraul

author

2002-05-10 17:13 UTC #0 lore raw

Have you checked that your patch doesn't deadlock on ia64?

Review

Quoted 9 lines

(quoted diff) +9 -0

+       /* First pause all related threaded processes */
+       if (dump_threads)       {
+               suspend_threads();
+       }
+       
+       /* now stop all vm operations */
+       down_write(&current->mm->mmap_sem);
+       segs = current->mm->map_count;
+

Stopping all vm operations means that copy_{to,from}_user can cause deadlocks.
ia64 needs copy_to_user in their stack unwind handler, IIRC called by ELF_CORE_COPY_REGS.

Afaics you don't handle that. You must dump all thread state before down_write(mmap_sem). And I don't see how you protect against 2 threads of one process calling suspend_threads() simultaneously.

--
Manfred

Mark Gross

2002-05-10 18:13 UTC #1 lore raw

ia64 requires more tweaks than just this to work. This patch as it stands isn't expected to work for ia64.

Getting the register states for the note sections is more involved for ia64 as well as the avoiding of patch collisions with the diffs in /pub/linux/kernel/ports/ia64/v2.5/

I have an ia64 patch set thats partially tested for 2.4.17, that seems to work. It didn't get posted as O(1) support became a bigger priority.

I'm hoping to start updating the ia64 patch to support 2.5x very soon.

--mgross

Quoted 20 lines · Manfred Spraul

Have you checked that your patch doesn't deadlock on ia64?
Review
Quoted 9 lines
(quoted diff) +9 -0
+       /* First pause all related threaded processes */
+       if (dump_threads)       {
+               suspend_threads();
+       }
+
+       /* now stop all vm operations */
+       down_write(&current->mm->mmap_sem);
+       segs = current->mm->map_count;
+
Stopping all vm operations means that copy_{to,from}_user can cause deadlocks.
ia64 needs copy_to_user in their stack unwind handler, IIRC called by ELF_CORE_COPY_REGS.
Afaics you don't handle that. You must dump all thread state before down_write(mmap_sem). And I don't see how you protect against 2 threads of one process calling suspend_threads() simultaneously.

Vamsi Krishna S.

2002-05-13 07:28 UTC #2 lore raw

In reply to Manfred Spraul

Hi Mark,

Despite other problems with ia64, stopping vm ops after the status (regs) of all the threads is captured, is a bug fix in generic code, and our patch effectively nukes that. Not a good thing. I had posted a patch on top of the previous tcore patch to fix this, I suppose it still applies to the 2.5.x patch. Please apply that (given here).

Vamsi.

Vamsi Krishna S.
Linux Technology Center,
IBM Software Lab, Bangalore.
Ph: +91 80 5262355 Extn: 3959
Internet: [email protected]

fs/binfmt_elf.c renamed from fs/binfmt_elf.c.ori +13 ~~-11~~

@@ -1289,10 +1289,6 @@
 	int dump_threads = 0;
 	int thread_status_size = 0;
 	
-	/* now stop all vm operations */
-	down_write(&current->mm->mmap_sem);
-	segs = current->mm->map_count;
-
  	if (atomic_read(&current->mm->mm_users) != 1) {
 		dump_threads = core_dumps_threads;
 	}

@@ -1337,6 +1333,19 @@
 		}
 	} /* End if(dump_threads) */
 
+	/*
+	 * This transfers the registers from regs into the standard
+	 * coredump arrangement, whatever that is. We need to do this
+	 * before acquiring mmap_sem as on some architectures (IA64)
+	 * we may need to access user pages to get register state.
+	 */
+	memset(&prstatus, 0, sizeof(prstatus));
+	elf_core_copy_regs(&prstatus.pr_reg, regs);
+
+	/* now stop all vm operations */
+	down_write(&current->mm->mmap_sem);
+	segs = current->mm->map_count;
+
 #ifdef DEBUG
 	printk("elf_core_dump: %d segs %lu limit\n", segs, limit);
 #endif

@@ -1358,16 +1367,9 @@
 	 * Set up the notes in similar form to SVR4 core dumps made
 	 * with info from their /proc.
 	 */
-	memset(&prstatus, 0, sizeof(prstatus));
 	fill_prstatus(&prstatus, current, signr);
 	fill_note(&notes[0], "CORE", NT_PRSTATUS, sizeof(prstatus), &prstatus);
 
-	/*
-	 * This transfers the registers from regs into the standard
-	 * coredump arrangement, whatever that is.
-	 */
-	elf_core_copy_regs(&prstatus.pr_reg, regs);
-
 #ifdef DEBUG
 	dump_regs("Passed in regs", (elf_greg_t *)regs);
 	dump_regs("prstatus regs", (elf_greg_t *)&prstatus.pr_reg);

Quoted 39 lines · Mark Gross

ia64 requires more tweaks than just this to work. This patch as it stands isn't expected to work for ia64.
Getting the register states for the note sections is more involved for ia64 as well as the avoiding of patch collisions with the diffs in /pub/linux/kernel/ports/ia64/v2.5/
I have an ia64 patch set thats partially tested for 2.4.17, that seems to work. It didn't get posted as O(1) support became a bigger priority.
I'm hoping to start updating the ia64 patch to support 2.5x very soon.
--mgross
Quoted 16 lines · Manfred Spraul
Have you checked that your patch doesn't deadlock on ia64?
Quoted 9 lines
+ /* First pause all related threaded processes */ + if
(dump_threads) {
(quoted diff) +5 -0
+               suspend_threads();
+       }
+
+       /* now stop all vm operations */
+       down_write(&current->mm->mmap_sem); +       segs =
current->mm->map_count;
+
Stopping all vm operations means that copy_{to,from}_user can cause deadlocks. ia64 needs copy_to_user in their stack unwind handler, IIRC called by ELF_CORE_COPY_REGS.
Quoted 3 lines
Afaics you don't handle that. You must dump all thread state before down_write(mmap_sem). And I don't see how you protect against 2 threads of one process calling suspend_threads() simultaneously.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html Please read the FAQ at
http://www.tux.org/lkml/

Mark Gross

2002-05-13 17:25 UTC #3 lore raw

In reply to Manfred Spraul

Thanks Vamsi,

I'll add this to the next version of the patch.

I've gotten a few comments about function names being too generic, and the
fact that I'm holding too many locks in the resume_threads function. Minor
stuff but, I want to clean that up as well.

--mgross

Quoted 114 lines · Vamsi Krishna S.

Hi Mark,
Despite other problems with ia64, stopping vm ops after the status (regs) of all the threads is captured, is a bug fix in generic code, and our patch effectively nukes that. Not a good thing. I had posted a patch on top of the previous tcore patch to fix this, I suppose it still applies to the 2.5.x patch. Please apply that (given here).
Vamsi.
Vamsi Krishna S.
Linux Technology Center,
IBM Software Lab, Bangalore.
Ph: +91 80 5262355 Extn: 3959
Internet: [email protected]
fs/binfmt_elf.c renamed from fs/binfmt_elf.c.ori +13 ~~-11~~
@@ -1289,10 +1289,6 @@
 	int dump_threads = 0;
 	int thread_status_size = 0;
 
-	/* now stop all vm operations */
-	down_write(&current->mm->mmap_sem);
-	segs = current->mm->map_count;
-
  	if (atomic_read(&current->mm->mm_users) != 1) {
 		dump_threads = core_dumps_threads;
 	}
@@ -1337,6 +1333,19 @@
 		}
 	} /* End if(dump_threads) */
 
+	/*
+	 * This transfers the registers from regs into the standard
+	 * coredump arrangement, whatever that is. We need to do this
+	 * before acquiring mmap_sem as on some architectures (IA64)
+	 * we may need to access user pages to get register state.
+	 */
+	memset(&prstatus, 0, sizeof(prstatus));
+	elf_core_copy_regs(&prstatus.pr_reg, regs);
+
+	/* now stop all vm operations */
+	down_write(&current->mm->mmap_sem);
+	segs = current->mm->map_count;
+
 #ifdef DEBUG
 	printk("elf_core_dump: %d segs %lu limit\n", segs, limit);
 #endif
@@ -1358,16 +1367,9 @@
 	 * Set up the notes in similar form to SVR4 core dumps made
 	 * with info from their /proc.
 	 */
-	memset(&prstatus, 0, sizeof(prstatus));
 	fill_prstatus(&prstatus, current, signr);
 	fill_note(&notes[0], "CORE", NT_PRSTATUS, sizeof(prstatus), &prstatus);
 
-	/*
-	 * This transfers the registers from regs into the standard
-	 * coredump arrangement, whatever that is.
-	 */
-	elf_core_copy_regs(&prstatus.pr_reg, regs);
-
 #ifdef DEBUG
 	dump_regs("Passed in regs", (elf_greg_t *)regs);
 	dump_regs("prstatus regs", (elf_greg_t *)&prstatus.pr_reg);
Quoted 35 lines · Mark Gross
ia64 requires more tweaks than just this to work. This patch as it stands isn't expected to work for ia64.
Getting the register states for the note sections is more involved for ia64 as well as the avoiding of patch collisions with the diffs in /pub/linux/kernel/ports/ia64/v2.5/
I have an ia64 patch set thats partially tested for 2.4.17, that seems to work. It didn't get posted as O(1) support became a bigger priority.
I'm hoping to start updating the ia64 patch to support 2.5x very soon.
--mgross
Quoted 16 lines · Manfred Spraul
Have you checked that your patch doesn't deadlock on ia64?
Quoted 9 lines
+ /* First pause all related threaded processes */ + if
(dump_threads) {
(quoted diff) +5 -0
+               suspend_threads();
+       }
+
+       /* now stop all vm operations */
+       down_write(&current->mm->mmap_sem); +       segs =
current->mm->map_count;
+
Stopping all vm operations means that copy_{to,from}_user can cause deadlocks. ia64 needs copy_to_user in their stack unwind handler, IIRC called by ELF_CORE_COPY_REGS.
Quoted 3 lines
Afaics you don't handle that. You must dump all thread state before down_write(mmap_sem). And I don't see how you protect against 2 threads of one process calling suspend_threads() simultaneously.
Quoted 5 lines
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at
http://www.tux.org/lkml/
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/