BUG: Linux 2.6.25 ptrace leaks struct_task

BUG: Linux 2.6.25 ptrace leaks struct_task

[Joris van Rantwijk]

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.

Re: BUG: Linux 2.6.25 ptrace leaks struct_task

[Jeff Dike]

On Fri, Jun 27, 2008 at 08:30:45PM +0200, Joris van Rantwijk wrote:
> 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().

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

Re: BUG: Linux 2.6.25 ptrace leaks struct_task

[Roland McGrath]

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

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

[PATCH 2.6.25-stable] x86_64 ptrace: fix sys32_ptrace task_struct leak

[Roland McGrath]

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 PTRACE_GETFPXREGS:	/* Get the child extended FPU state. */
-		return copy_regset_to_user(child, &user_x86_32_view,
-					   REGSET_XFP, 0,
-					   sizeof(struct user32_fxsr_struct),
-					   datap);
+		ret = copy_regset_to_user(child, &user_x86_32_view,
+					  REGSET_XFP, 0,
+					  sizeof(struct user32_fxsr_struct),
+					  datap);
+		break;
 
 	case PTRACE_SETFPXREGS:	/* Set the child extended FPU state. */
-		return copy_regset_from_user(child, &user_x86_32_view,
+		ret = copy_regset_from_user(child, &user_x86_32_view,
 					     REGSET_XFP, 0,
 					     sizeof(struct user32_fxsr_struct),
 					     datap);
+		break;
 
 	default:
-		return compat_ptrace_request(child, request, addr, data);
+		ret = compat_ptrace_request(child, request, addr, data);
+		break;
 	}
 
  out:

[PATCH 2.6.25-stable] x86_64 ptrace: fix sys32_ptrace task_struct leak

[Roland McGrath]

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 PTRACE_GETFPXREGS:	/* Get the child extended FPU state. */
-		return copy_regset_to_user(child, &user_x86_32_view,
-					   REGSET_XFP, 0,
-					   sizeof(struct user32_fxsr_struct),
-					   datap);
+		ret = copy_regset_to_user(child, &user_x86_32_view,
+					  REGSET_XFP, 0,
+					  sizeof(struct user32_fxsr_struct),
+					  datap);
+		break;
 
 	case PTRACE_SETFPXREGS:	/* Set the child extended FPU state. */
-		return copy_regset_from_user(child, &user_x86_32_view,
+		ret = copy_regset_from_user(child, &user_x86_32_view,
 					     REGSET_XFP, 0,
 					     sizeof(struct user32_fxsr_struct),
 					     datap);
+		break;
 
 	default:
-		return compat_ptrace_request(child, request, addr, data);
+		ret = compat_ptrace_request(child, request, addr, data);
+		break;
 	}
 
  out: