Re: BUG: Linux 2.6.25 ptrace leaks struct_task

Previous thread: [PATCH 7/13] EDAC core fix workq timer by dougthompson on Friday, June 27, 2008 - 11:13 am. (1 message)

Next thread: [GIT PULL] firewire update by Stefan Richter on Friday, June 27, 2008 - 12:05 pm. (1 message)
From: Joris van Rantwijk
Date: Friday, June 27, 2008 - 11:30 am

I think sys32_ptrace() is leaking struct_task.

In arch/x86/kernel/ptrace.c, function sys32_ptrace(), there
is a call to ptrace_get_task_struct(). In some cases (such as
PTRACE_GETREGS), there is no matching call to put_task_struct().

Test case: fork many childs, calling PTRACE_GETREGS on each child.
Indeed this causes unbounded growth of the task_struct allocation
in /proc/slabinfo, and it also causes physical memory to disappear
from /proc/meminfo.

I have tested this on Linux 2.6.25.4 for x86_64, but the
relevant code has not been changed in 2.6.25.9.
This bug appears to be fixed in 2.6.26-rc8: the code looks
different and I can't reproduce the issue there.

Oops, just discovered this problem has already been reported
in http://lkml.org/lkml/2008/5/29/266 although not much
seems to have been done about it.

Should this be fixed for 2.6.25.10 ?

Is it likely that this bug is related to the mysterious
disappearance of memory from /proc/meminfo as reported in
http://lkml.org/lkml/2008/6/24/15 ?

Even so, how is it possible that memory just disappears
from /proc/meminfo? It can't be the task_struct cache itself,
because that is all covered under Slab, right ?

Greetings, Joris.
--

From: Jeff Dike
Date: Friday, June 27, 2008 - 1:00 pm

Yup, good diagnosis.  The culprit is
5a4646a4efed8c835f76c3b88f3155f6ab5b8d9b.  Doing an s/return /ret = /
inside that switch should fix the bug.

It looks like it's since been fixed in mainline by the restructuring
done in 562b80bafffaf42a6d916b0a2ee3d684220a1c10.

			Jeff

-- 
Work email - jdike at linux dot intel dot com
--

From: Roland McGrath
Date: Friday, June 27, 2008 - 1:17 pm

I could swear I posted this fix two weeks ago, but I can't find the posting
now.  I do still have it lurking in a GIT tree though.


Thanks,
Roland
--

From: Roland McGrath
Date: Friday, June 27, 2008 - 1:18 pm

Commit 5a4646a4efed8c835f76c3b88f3155f6ab5b8d9b introduced a leak of
task_struct refs into sys32_ptrace.  This bug has already gone away in
for 2.6.26 in commit 562b80bafffaf42a6d916b0a2ee3d684220a1c10.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/x86/kernel/ptrace.c |   45 ++++++++++++++++++++++++++-------------------
 1 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 9003e0b..a10ba65 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1309,42 +1309,49 @@ asmlinkage long sys32_ptrace(long request, u32 pid, u32 addr, u32 data)
 		break;
 
 	case PTRACE_GETREGS:	/* Get all gp regs from the child. */
-		return copy_regset_to_user(child, &user_x86_32_view,
-					   REGSET_GENERAL,
-					   0, sizeof(struct user_regs_struct32),
-					   datap);
+		ret = copy_regset_to_user(child, &user_x86_32_view,
+					  REGSET_GENERAL,
+					  0, sizeof(struct user_regs_struct32),
+					  datap);
+		break;
 
 	case PTRACE_SETREGS:	/* Set all gp regs in the child. */
-		return copy_regset_from_user(child, &user_x86_32_view,
-					     REGSET_GENERAL, 0,
-					     sizeof(struct user_regs_struct32),
-					     datap);
+		ret = copy_regset_from_user(child, &user_x86_32_view,
+					    REGSET_GENERAL, 0,
+					    sizeof(struct user_regs_struct32),
+					    datap);
+		break;
 
 	case PTRACE_GETFPREGS:	/* Get the child FPU state. */
-		return copy_regset_to_user(child, &user_x86_32_view,
-					   REGSET_FP, 0,
-					   sizeof(struct user_i387_ia32_struct),
-					   datap);
+		ret = copy_regset_to_user(child, &user_x86_32_view,
+					  REGSET_FP, 0,
+					  sizeof(struct user_i387_ia32_struct),
+					  datap);
+		break;
 
 	case PTRACE_SETFPREGS:	/* Set the child FPU state. */
-		return copy_regset_from_user(
+		ret = copy_regset_from_user(
 			child, &user_x86_32_view, REGSET_FP,
 			0, sizeof(struct user_i387_ia32_struct), datap);
+		break;
 
 	case ...
From: Roland McGrath
Date: Friday, June 27, 2008 - 1:48 pm

Commit 5a4646a4efed8c835f76c3b88f3155f6ab5b8d9b introduced a leak of
task_struct refs into sys32_ptrace.  This bug has already gone away in
for 2.6.26 in commit 562b80bafffaf42a6d916b0a2ee3d684220a1c10.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/x86/kernel/ptrace.c |   45 ++++++++++++++++++++++++++-------------------
 1 files changed, 26 insertions(+), 19 deletions(-)

diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 9003e0b..a10ba65 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1309,42 +1309,49 @@ asmlinkage long sys32_ptrace(long request, u32 pid, u32 addr, u32 data)
 		break;
 
 	case PTRACE_GETREGS:	/* Get all gp regs from the child. */
-		return copy_regset_to_user(child, &user_x86_32_view,
-					   REGSET_GENERAL,
-					   0, sizeof(struct user_regs_struct32),
-					   datap);
+		ret = copy_regset_to_user(child, &user_x86_32_view,
+					  REGSET_GENERAL,
+					  0, sizeof(struct user_regs_struct32),
+					  datap);
+		break;
 
 	case PTRACE_SETREGS:	/* Set all gp regs in the child. */
-		return copy_regset_from_user(child, &user_x86_32_view,
-					     REGSET_GENERAL, 0,
-					     sizeof(struct user_regs_struct32),
-					     datap);
+		ret = copy_regset_from_user(child, &user_x86_32_view,
+					    REGSET_GENERAL, 0,
+					    sizeof(struct user_regs_struct32),
+					    datap);
+		break;
 
 	case PTRACE_GETFPREGS:	/* Get the child FPU state. */
-		return copy_regset_to_user(child, &user_x86_32_view,
-					   REGSET_FP, 0,
-					   sizeof(struct user_i387_ia32_struct),
-					   datap);
+		ret = copy_regset_to_user(child, &user_x86_32_view,
+					  REGSET_FP, 0,
+					  sizeof(struct user_i387_ia32_struct),
+					  datap);
+		break;
 
 	case PTRACE_SETFPREGS:	/* Set the child FPU state. */
-		return copy_regset_from_user(
+		ret = copy_regset_from_user(
 			child, &user_x86_32_view, REGSET_FP,
 			0, sizeof(struct user_i387_ia32_struct), datap);
+		break;
 
 	case ...
Previous thread: [PATCH 7/13] EDAC core fix workq timer by dougthompson on Friday, June 27, 2008 - 11:13 am. (1 message)

Next thread: [GIT PULL] firewire update by Stefan Richter on Friday, June 27, 2008 - 12:05 pm. (1 message)