powerpc: kernel/kgdb.c: fix memory leakage

PATCH In review Cong Ding 4 participants 4 messages Jan 14, 2013 – Feb 27, 2013 PowerPC

lore

Share & tools

lore

Cong Ding

author

2013-01-14 17:26 UTC #0 lore raw

PATCH 1/1 powerpc: kernel/kgdb.c: fix memory leakage

the variable backup_current_thread_info isn't freed before existing the function.

Signed-off-by Cong Ding <[email protected]>

---
 arch/powerpc/kernel/kgdb.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

arch/powerpc/kernel/kgdb.c +3 -2

index 8747447..5ca82cd 100644
@@ -154,12 +154,12 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
 static int kgdb_singlestep(struct pt_regs *regs)
 {
 	struct thread_info *thread_info, *exception_thread_info;
-	struct thread_info *backup_current_thread_info = \
-		(struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
+	struct thread_info *backup_current_thread_info;
 
 	if (user_mode(regs))
 		return 0;
 
+	backup_current_thread_info = (struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
 	/*
 	 * On Book E and perhaps other processors, singlestep is handled on
 	 * the critical exception stack.  This causes current_thread_info()

@@ -185,6 +185,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
 		/* Restore current_thread_info lastly. */
 		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
 
+	kfree(backup_current_thread_info);
 	return 1;
 }

Signature · 1 line

1.7.9.5

Jason Wessel

2013-02-01 03:03 UTC #1 lore raw

Quoted 13 lines · Cong Ding

the variable backup_current_thread_info isn't freed before existing the function.
Signed-off-by Cong Ding <[email protected]>
---
 arch/powerpc/kernel/kgdb.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
arch/powerpc/kernel/kgdb.c +0 -0
index 8747447..5ca82cd 100644

arch/powerpc/kernel/kgdb.c static int kgdb_handle_breakpoint(struct pt_regs *regs) · line 157

Quoted 6 lines

arch/powerpc/kernel/kgdb.c +1 -2

@@ -154,12 +154,12 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
 static int kgdb_singlestep(struct pt_regs *regs)
 {
 	struct thread_info *thread_info, *exception_thread_info;
-	struct thread_info *backup_current_thread_info = \
-		(struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
+	struct thread_info *backup_current_thread_info;

Woh... This is definitely wrong. You have found a problem for sure, but this is not the right way to fix it.

It is not a good idea to kmalloc while single stepping because you can hang the kernel if you single step any operation in kmalloc().

I am in the process of going through all the kgdb mails from the last few months while I had been away from the project, so I didn't catch this one and I see it has upstream commit (fefd9e6f8). I'll submit another patch to fix this the right way and use a static variable. This is ok to use a static variable here because this is not something we can recursively call at a single CPU level.

If Ben prefers we not burn the memory unless kgdb is active we can kmalloc / kfree the space we need at the time that kgdb is initialized. Else we can go with this patch you see below. We'll see what Ben desires.

-----

arch/powerpc/kernel/kgdb.c +5 -5

index a7bc752..bb12c8b 100644
@@ -151,15 +151,16 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
 	return 1;
 }
 
+static struct thread_info kgdb_backup_thread_info[NR_CPUS];
+
 static int kgdb_singlestep(struct pt_regs *regs)
 {
 	struct thread_info *thread_info, *exception_thread_info;
-	struct thread_info *backup_current_thread_info;
+	int cpu = raw_smp_processor_id();
 
 	if (user_mode(regs))
 		return 0;
 
-	backup_current_thread_info = (struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
 	/*
 	 * On Book E and perhaps other processors, singlestep is handled on
 	 * the critical exception stack.  This causes current_thread_info()

@@ -175,7 +176,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
 
 	if (thread_info != exception_thread_info) {
 		/* Save the original current_thread_info. */
-		memcpy(backup_current_thread_info, exception_thread_info, sizeof *thread_info);
+		memcpy(&kgdb_backup_thread_info[cpu], exception_thread_info, sizeof *thread_info);
 		memcpy(exception_thread_info, thread_info, sizeof *thread_info);
 	}

@@ -183,9 +184,8 @@ static int kgdb_singlestep(struct pt_regs *regs)
 
 	if (thread_info != exception_thread_info)
 		/* Restore current_thread_info lastly. */
-		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
+		memcpy(exception_thread_info, &kgdb_backup_thread_info[cpu], sizeof *thread_info);
 
-	kfree(backup_current_thread_info);
 	return 1;
 }

-----

Thanks,
Jason.

Quoted 17 lines

arch/powerpc/kernel/kgdb.c +2 -0

 	if (user_mode(regs))
 		return 0;
 
+	backup_current_thread_info = (struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
 	/*
 	 * On Book E and perhaps other processors, singlestep is handled on
 	 * the critical exception stack.  This causes current_thread_info()

@@ -185,6 +185,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
 		/* Restore current_thread_info lastly. */
 		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
 
+	kfree(backup_current_thread_info);
 	return 1;
 }

Benjamin Herrenschmidt

2013-02-08 02:41 UTC #2 lore raw

Quoted 17 lines · Jason Wessel

arch/powerpc/kernel/kgdb.c static int kgdb_handle_breakpoint(struct pt_regs *regs) · line 157
Quoted 11 lines
arch/powerpc/kernel/kgdb.c +1 -2
index 8747447..5ca82cd 100644
@@ -154,12 +154,12 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
 static int kgdb_singlestep(struct pt_regs *regs)
 {
 	struct thread_info *thread_info, *exception_thread_info;
-	struct thread_info *backup_current_thread_info = \
-		(struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
+	struct thread_info *backup_current_thread_info;
Woh... This is definitely wrong. You have found a problem for sure, but this is not the right way to fix it.

Quoted 2 lines

It is not a good idea to kmalloc while single stepping because you can hang the kernel if you single step any operation in kmalloc().

Ok. I applied the fix because it was obviously correct vs. the previous version of the code (ie. the kmalloc was already there)

Quoted 6 lines

I am in the process of going through all the kgdb mails from the last few months while I had been away from the project, so I didn't catch this one and I see it has upstream commit (fefd9e6f8). I'll submit another patch to fix this the right way and use a static variable. This is ok to use a static variable here because this is not something we can recursively call at a single CPU level.

But a static will be shared by all CPUs or do you mean a per-cpu one ?

Quoted 4 lines

If Ben prefers we not burn the memory unless kgdb is active we can kmalloc / kfree the space we need at the time that kgdb is initialized. Else we can go with this patch you see below. We'll see what Ben desires.

What about the stack ? thread info isn't very big...

Cheers,
Ben.

Quoted 69 lines

-----

arch/powerpc/kernel/kgdb.c +5 -6

index a7bc752..bb12c8b 100644
@@ -151,15 +151,16 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
 	return 1;
 }
 
+static struct thread_info kgdb_backup_thread_info[NR_CPUS];
+
 static int kgdb_singlestep(struct pt_regs *regs)
 {
 	struct thread_info *thread_info, *exception_thread_info;
-	struct thread_info *backup_current_thread_info;
+	int cpu = raw_smp_processor_id();
 
 	if (user_mode(regs))
 		return 0;
 
-	backup_current_thread_info = (struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
 	/*
 	 * On Book E and perhaps other processors, singlestep is handled on
 	 * the critical exception stack.  This causes current_thread_info()

@@ -175,7 +176,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
 
 	if (thread_info != exception_thread_info) {
 		/* Save the original current_thread_info. */
-		memcpy(backup_current_thread_info, exception_thread_info, sizeof *thread_info);
+		memcpy(&kgdb_backup_thread_info[cpu], exception_thread_info, sizeof *thread_info);
 		memcpy(exception_thread_info, thread_info, sizeof *thread_info);
 	}

@@ -183,9 +184,8 @@ static int kgdb_singlestep(struct pt_regs *regs)
 
 	if (thread_info != exception_thread_info)
 		/* Restore current_thread_info lastly. */
-		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
+		memcpy(exception_thread_info, &kgdb_backup_thread_info[cpu], sizeof *thread_info);
 
-	kfree(backup_current_thread_info);
 	return 1;
 }
 
 
-----

Thanks,
Jason.

Quoted 17 lines

arch/powerpc/kernel/kgdb.c +2 -0

 	if (user_mode(regs))
 		return 0;
 
+	backup_current_thread_info = (struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
 	/*
 	 * On Book E and perhaps other processors, singlestep is handled on
 	 * the critical exception stack.  This causes current_thread_info()

@@ -185,6 +185,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
 		/* Restore current_thread_info lastly. */
 		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
 
+	kfree(backup_current_thread_info);
 	return 1;
 }

tiejun.chen

2013-02-27 02:44 UTC #3 lore raw

Quoted 32 lines · Benjamin Herrenschmidt

Quoted 17 lines · Jason Wessel
arch/powerpc/kernel/kgdb.c static int kgdb_handle_breakpoint(struct pt_regs *regs) · line 157
Quoted 11 lines
arch/powerpc/kernel/kgdb.c +1 -2
index 8747447..5ca82cd 100644
@@ -154,12 +154,12 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
  static int kgdb_singlestep(struct pt_regs *regs)
  {
  	struct thread_info *thread_info, *exception_thread_info;
-	struct thread_info *backup_current_thread_info = \
-		(struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
+	struct thread_info *backup_current_thread_info;
Woh... This is definitely wrong. You have found a problem for sure, but this is not the right way to fix it.
Quoted 2 lines
It is not a good idea to kmalloc while single stepping because you can hang the kernel if you single step any operation in kmalloc().
Ok. I applied the fix because it was obviously correct vs. the previous version of the code (ie. the kmalloc was already there)
Quoted 6 lines
I am in the process of going through all the kgdb mails from the last few months while I had been away from the project, so I didn't catch this one and I see it has upstream commit (fefd9e6f8). I'll submit another patch to fix this the right way and use a static variable. This is ok to use a static variable here because this is not something we can recursively call at a single CPU level.

Quoted 8 lines

But a static will be shared by all CPUs or do you mean a per-cpu one ?
Quoted 4 lines
If Ben prefers we not burn the memory unless kgdb is active we can kmalloc / kfree the space we need at the time that kgdb is initialized. Else we can go with this patch you see below. We'll see what Ben desires.
What about the stack ? thread info isn't very big...

Actually booke already copy the thread_info when we enter the debug exception, and I will send a patch to do the same thing for book3e, so I also remove these copying action here then we're happy without these nasty stuff :)

Tiejun

Quoted 77 lines

Cheers,
Ben.

Quoted 69 lines

-----

arch/powerpc/kernel/kgdb.c +5 -6

index a7bc752..bb12c8b 100644
@@ -151,15 +151,16 @@ static int kgdb_handle_breakpoint(struct pt_regs *regs)
  	return 1;
  }
 
+static struct thread_info kgdb_backup_thread_info[NR_CPUS];
+
  static int kgdb_singlestep(struct pt_regs *regs)
  {
  	struct thread_info *thread_info, *exception_thread_info;
-	struct thread_info *backup_current_thread_info;
+	int cpu = raw_smp_processor_id();
 
  	if (user_mode(regs))
  		return 0;
 
-	backup_current_thread_info = (struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
  	/*
  	 * On Book E and perhaps other processors, singlestep is handled on
  	 * the critical exception stack.  This causes current_thread_info()

@@ -175,7 +176,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
 
  	if (thread_info != exception_thread_info) {
  		/* Save the original current_thread_info. */
-		memcpy(backup_current_thread_info, exception_thread_info, sizeof *thread_info);
+		memcpy(&kgdb_backup_thread_info[cpu], exception_thread_info, sizeof *thread_info);
  		memcpy(exception_thread_info, thread_info, sizeof *thread_info);
  	}

@@ -183,9 +184,8 @@ static int kgdb_singlestep(struct pt_regs *regs)
 
  	if (thread_info != exception_thread_info)
  		/* Restore current_thread_info lastly. */
-		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
+		memcpy(exception_thread_info, &kgdb_backup_thread_info[cpu], sizeof *thread_info);
 
-	kfree(backup_current_thread_info);
  	return 1;
  }
 
 
-----

Thanks,
Jason.

Quoted 17 lines

arch/powerpc/kernel/kgdb.c +2 -0

  	if (user_mode(regs))
  		return 0;
 
+	backup_current_thread_info = (struct thread_info *)kmalloc(sizeof(struct thread_info), GFP_KERNEL);
  	/*
  	 * On Book E and perhaps other processors, singlestep is handled on
  	 * the critical exception stack.  This causes current_thread_info()

@@ -185,6 +185,7 @@ static int kgdb_singlestep(struct pt_regs *regs)
  		/* Restore current_thread_info lastly. */
  		memcpy(exception_thread_info, backup_current_thread_info, sizeof *thread_info);
 
+	kfree(backup_current_thread_info);
  	return 1;
  }