[Patch 2.6.7]might-sleep-in-atomic while dumping elf

[Patch 2.6.7]might-sleep-in-atomic while dumping elf

[Zou, Nanhai]

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

Here is a patch to fix a problem of might-sleep-in-atomic which
David Mosberger mentioned at
http://www.gelato.unsw.edu.au/linux-ia64/0407/10526.html

On IA64 platform, a might-sleep-in-atomic warning raise while dumping a
multi-thread process.
That is because elf_cord_dump hold the tasklist_lock before kernel doing
a access_process_vm in elf_core_copy_task_regs, 

This patch detached elf_core_copy_task_regs function from inside
tasklist_lock to remove the warning.

diff -Nraup a/fs/binfmt_elf.c b/fs/binfmt_elf.c
--- a/fs/binfmt_elf.c	2004-06-24 09:25:07.000000000 +0800
+++ b/fs/binfmt_elf.c	2004-07-30 16:44:10.000000000 +0800
@@ -1216,6 +1216,7 @@ struct elf_thread_status
 	struct list_head list;
 	struct elf_prstatus prstatus;	/* NT_PRSTATUS */
 	elf_fpregset_t fpu;		/* NT_PRFPREG */
+	struct task_struct *thread;
 #ifdef ELF_CORE_COPY_XFPREGS
 	elf_fpxregset_t xfpu;		/* NT_PRXFPREG */
 #endif
@@ -1228,18 +1229,10 @@ struct elf_thread_status
  * we need to keep a linked list of every threads pr_status and then
  * create a single section for them in the final core file.
  */
-static int elf_dump_thread_status(long signr, struct task_struct * p,
struct list_head * thread_list)
+static int elf_dump_thread_status(long signr, struct elf_thread_status
*t)
 {
-
-	struct elf_thread_status *t;
 	int sz = 0;
-
-	t = kmalloc(sizeof(*t), GFP_ATOMIC);
-	if (!t)
-		return 0;
-	memset(t, 0, sizeof(*t));
-
-	INIT_LIST_HEAD(&t->list);
+	struct task_struct *p = t->thread;
 	t->num_notes = 0;
 
 	fill_prstatus(&t->prstatus, p, signr);
@@ -1262,7 +1255,6 @@ static int elf_dump_thread_status(long s
 		sz += notesize(&t->notes[2]);
 	}
 #endif	
-	list_add(&t->list, thread_list);
 	return sz;
 }
 
@@ -1333,22 +1325,32 @@ static int elf_core_dump(long signr, str
 		goto cleanup;
 #endif
 
-	/* capture the status of all other threads */
 	if (signr) {
+		struct elf_thread_status *tmp;
 		read_lock(&tasklist_lock);
 		do_each_thread(g,p)
 			if (current->mm == p->mm && current != p) {
-				int sz = elf_dump_thread_status(signr,
p, &thread_list);
-				if (!sz) {
+				tmp = kmalloc(sizeof(*tmp), GFP_ATOMIC);
+				if (!tmp) {
 					read_unlock(&tasklist_lock);
 					goto cleanup;
-				} else
-					thread_status_size += sz;
+				}
+				memset(tmp, 0, sizeof(*tmp));
+				INIT_LIST_HEAD(&tmp->list);
+				tmp->thread = p;
+				list_add(&tmp->list, &thread_list);
 			}
 		while_each_thread(g,p);
 		read_unlock(&tasklist_lock);
+		list_for_each(t, &thread_list) {
+			struct elf_thread_status *tmp = list_entry(t,
struct elf_thread_status, list);
+			int sz = elf_dump_thread_status(signr, tmp);
+			if (!sz)
+				goto cleanup;
+			else
+				thread_status_size += sz;
+		}
 	}
-
 	/* now collect the dump for the current */
 	memset(prstatus, 0, sizeof(*prstatus));
 	fill_prstatus(prstatus, current, signr);




[-- Attachment #2: might_sleep_dump_elf.patch --]
[-- Type: application/octet-stream, Size: 2349 bytes --]

diff -Nraup a/fs/binfmt_elf.c b/fs/binfmt_elf.c
--- a/fs/binfmt_elf.c	2004-06-24 09:25:07.000000000 +0800
+++ b/fs/binfmt_elf.c	2004-07-30 16:44:10.000000000 +0800
@@ -1216,6 +1216,7 @@ struct elf_thread_status
 	struct list_head list;
 	struct elf_prstatus prstatus;	/* NT_PRSTATUS */
 	elf_fpregset_t fpu;		/* NT_PRFPREG */
+	struct task_struct *thread;
 #ifdef ELF_CORE_COPY_XFPREGS
 	elf_fpxregset_t xfpu;		/* NT_PRXFPREG */
 #endif
@@ -1228,18 +1229,10 @@ struct elf_thread_status
  * we need to keep a linked list of every threads pr_status and then
  * create a single section for them in the final core file.
  */
-static int elf_dump_thread_status(long signr, struct task_struct * p, struct list_head * thread_list)
+static int elf_dump_thread_status(long signr, struct elf_thread_status *t)
 {
-
-	struct elf_thread_status *t;
 	int sz = 0;
-
-	t = kmalloc(sizeof(*t), GFP_ATOMIC);
-	if (!t)
-		return 0;
-	memset(t, 0, sizeof(*t));
-
-	INIT_LIST_HEAD(&t->list);
+	struct task_struct *p = t->thread;
 	t->num_notes = 0;
 
 	fill_prstatus(&t->prstatus, p, signr);
@@ -1262,7 +1255,6 @@ static int elf_dump_thread_status(long s
 		sz += notesize(&t->notes[2]);
 	}
 #endif	
-	list_add(&t->list, thread_list);
 	return sz;
 }
 
@@ -1333,22 +1325,32 @@ static int elf_core_dump(long signr, str
 		goto cleanup;
 #endif
 
-	/* capture the status of all other threads */
 	if (signr) {
+		struct elf_thread_status *tmp;
 		read_lock(&tasklist_lock);
 		do_each_thread(g,p)
 			if (current->mm == p->mm && current != p) {
-				int sz = elf_dump_thread_status(signr, p, &thread_list);
-				if (!sz) {
+				tmp = kmalloc(sizeof(*tmp), GFP_ATOMIC);
+				if (!tmp) {
 					read_unlock(&tasklist_lock);
 					goto cleanup;
-				} else
-					thread_status_size += sz;
+				}
+				memset(tmp, 0, sizeof(*tmp));
+				INIT_LIST_HEAD(&tmp->list);
+				tmp->thread = p;
+				list_add(&tmp->list, &thread_list);
 			}
 		while_each_thread(g,p);
 		read_unlock(&tasklist_lock);
+		list_for_each(t, &thread_list) {
+			struct elf_thread_status *tmp = list_entry(t, struct elf_thread_status, list);
+			int sz = elf_dump_thread_status(signr, tmp);
+			if (!sz)
+				goto cleanup;
+			else
+				thread_status_size += sz;
+		}
 	}
-
 	/* now collect the dump for the current */
 	memset(prstatus, 0, sizeof(*prstatus));
 	fill_prstatus(prstatus, current, signr);

Re: [Patch 2.6.7]might-sleep-in-atomic while dumping elf

[Andrew Morton]

"Zou, Nanhai" <nanhai.zou@intel.com> wrote:
>
> Here is a patch to fix a problem of might-sleep-in-atomic which
> David Mosberger mentioned at
> http://www.gelato.unsw.edu.au/linux-ia64/0407/10526.html
> 
> On IA64 platform, a might-sleep-in-atomic warning raise while dumping a
> multi-thread process.
> That is because elf_cord_dump hold the tasklist_lock before kernel doing
> a access_process_vm in elf_core_copy_task_regs, 
> 
> This patch detached elf_core_copy_task_regs function from inside
> tasklist_lock to remove the warning.

hm, OK, no worse than what we had there before :(

That GFP_ATOMIC allocation of one 824-byte-on-x86 structure for each
thread looks really, really nasty.  It could easily chew up 100% of the page
reserves and fail.  I wonder if it is safe to drop the tasklist_lock while we 
allocate the memory?

You're still testing for a zero return from elf_dump_thread_status().  I
think that with your changes, that is no longer possible, is it?

Please edit in 80-col xterms.  You'll find that a layout such as the below
becomes more agreeable.


+		list_for_each(t, &thread_list) {
+			struct elf_thread_status *tmp;
+			int sz;
+
+			tmp = list_entry(t, struct elf_thread_status, list);
+			sz = elf_dump_thread_status(signr, tmp);

RE: [Patch 2.6.7]might-sleep-in-atomic while dumping elf

[Zou, Nanhai]

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

Thanks,
This is the patch that deals with the format issue and removed the
return check of elf_dump_thread_status.

I think it's hard to drop the tasklist_lock  while we allocate the
memory unless we totally reorganize the elf_core_dump related functions
and data structures.

Zou Nan hai
-----Original Message-----
From: Andrew Morton [mailto:akpm@osdl.org] 
Sent: Thursday, August 05, 2004 6:47 AM
To: Zou, Nanhai
Cc: linux-kernel@vger.kernel.org; davidm@napali.hpl.hp.com; Luck, Tony
Subject: Re: [Patch 2.6.7]might-sleep-in-atomic while dumping elf

"Zou, Nanhai" <nanhai.zou@intel.com> wrote:
>
> Here is a patch to fix a problem of might-sleep-in-atomic which
> David Mosberger mentioned at
> http://www.gelato.unsw.edu.au/linux-ia64/0407/10526.html
> 
> On IA64 platform, a might-sleep-in-atomic warning raise while dumping
a
> multi-thread process.
> That is because elf_cord_dump hold the tasklist_lock before kernel
doing
> a access_process_vm in elf_core_copy_task_regs, 
> 
> This patch detached elf_core_copy_task_regs function from inside
> tasklist_lock to remove the warning.

hm, OK, no worse than what we had there before :(

That GFP_ATOMIC allocation of one 824-byte-on-x86 structure for each
thread looks really, really nasty.  It could easily chew up 100% of the
page
reserves and fail.  I wonder if it is safe to drop the tasklist_lock
while we 
allocate the memory?

You're still testing for a zero return from elf_dump_thread_status().  I
think that with your changes, that is no longer possible, is it?

Please edit in 80-col xterms.  You'll find that a layout such as the
below
becomes more agreeable.


+		list_for_each(t, &thread_list) {
+			struct elf_thread_status *tmp;
+			int sz;
+
+			tmp = list_entry(t, struct elf_thread_status,
list);
+			sz = elf_dump_thread_status(signr, tmp);



[-- Attachment #2: might_sleep_dump_elf.patch --]
[-- Type: application/octet-stream, Size: 2324 bytes --]

diff -Nraup a/fs/binfmt_elf.c b/fs/binfmt_elf.c
--- a/fs/binfmt_elf.c	2004-08-04 02:30:27.907022628 -0700
+++ b/fs/binfmt_elf.c	2004-08-04 02:32:24.114052454 -0700
@@ -1216,6 +1216,7 @@ struct elf_thread_status
 	struct list_head list;
 	struct elf_prstatus prstatus;	/* NT_PRSTATUS */
 	elf_fpregset_t fpu;		/* NT_PRFPREG */
+	struct task_struct *thread;
 #ifdef ELF_CORE_COPY_XFPREGS
 	elf_fpxregset_t xfpu;		/* NT_PRXFPREG */
 #endif
@@ -1228,18 +1229,10 @@ struct elf_thread_status
  * we need to keep a linked list of every threads pr_status and then
  * create a single section for them in the final core file.
  */
-static int elf_dump_thread_status(long signr, struct task_struct * p, struct list_head * thread_list)
+static int elf_dump_thread_status(long signr, struct elf_thread_status *t)
 {
-
-	struct elf_thread_status *t;
 	int sz = 0;
-
-	t = kmalloc(sizeof(*t), GFP_ATOMIC);
-	if (!t)
-		return 0;
-	memset(t, 0, sizeof(*t));
-
-	INIT_LIST_HEAD(&t->list);
+	struct task_struct *p = t->thread;
 	t->num_notes = 0;
 
 	fill_prstatus(&t->prstatus, p, signr);
@@ -1262,7 +1255,6 @@ static int elf_dump_thread_status(long s
 		sz += notesize(&t->notes[2]);
 	}
 #endif	
-	list_add(&t->list, thread_list);
 	return sz;
 }
 
@@ -1333,22 +1325,31 @@ static int elf_core_dump(long signr, str
 		goto cleanup;
 #endif
 
-	/* capture the status of all other threads */
 	if (signr) {
+		struct elf_thread_status *tmp;
 		read_lock(&tasklist_lock);
 		do_each_thread(g,p)
 			if (current->mm == p->mm && current != p) {
-				int sz = elf_dump_thread_status(signr, p, &thread_list);
-				if (!sz) {
+				tmp = kmalloc(sizeof(*tmp), GFP_ATOMIC);
+				if (!tmp) {
 					read_unlock(&tasklist_lock);
 					goto cleanup;
-				} else
-					thread_status_size += sz;
+				}
+				memset(tmp, 0, sizeof(*tmp));
+				INIT_LIST_HEAD(&tmp->list);
+				tmp->thread = p;
+				list_add(&tmp->list, &thread_list);
 			}
 		while_each_thread(g,p);
 		read_unlock(&tasklist_lock);
+		list_for_each(t, &thread_list) {
+			struct elf_thread_status *tmp;
+			int sz;
+			tmp = list_entry(t, struct elf_thread_status, list);
+			sz = elf_dump_thread_status(signr, tmp);
+			thread_status_size += sz;
+		}
 	}
-
 	/* now collect the dump for the current */
 	memset(prstatus, 0, sizeof(*prstatus));
 	fill_prstatus(prstatus, current, signr);