Skip to contentSkip to conversation

x86: Fix io_bitmap_ptr memory leak on copy_process()

PATCH In review Tadashi Abe 2 participants 4 messages Feb 8, 2011 – Feb 14, 2011

Subsystems (4)
x86 MM core Scheduler exec and binfmt
lore
Share & tools
lore

Tadashi Abe

author
#0 lore raw

PATCH 1/1 x86: Fix io_bitmap_ptr memory leak on copy_process()

x86 copy_thread() allocates io_bitmap_ptr area in child's thread_struct when the parent has TIF_IO_BITMAP flag via ioperm().
And in this case, if copy_process() terminates with errors after copy_thread() success,
this io_bitmap_ptr area is being left without kfree().

Signed-off-by Tadashi Abe <[email protected]>
---
 arch/x86/kernel/process.c |    6 ++++++
 include/linux/sched.h     |    5 +++++
 kernel/fork.c             |    4 +++-
 3 files changed, 14 insertions(+), 1 deletions(-)
arch/x86/kernel/process.c +6 -0
index b3feabc..290e98b 100644
@@ -122,6 +122,12 @@ void flush_thread(void)
 	clear_used_math();
 }
 
+void free_thread_struct(struct task_struct *p)
+{
+	if (p->thread.io_bitmap_ptr)
+		kfree(p->thread.io_bitmap_ptr);
+}
+
 static void hard_disable_TSC(void)
 {
 	write_cr4(read_cr4() | X86_CR4_TSD);
include/linux/sched.h +5 -0
index 23e9c27..2345391 100644
@@ -2159,6 +2159,11 @@ extern int copy_thread(unsigned long, unsigned long, unsigned long,
 			struct task_struct *, struct pt_regs *);
 extern void flush_thread(void);
 extern void exit_thread(void);
+#ifdef CONFIG_X86
+extern void free_thread_struct(struct task_struct *p);
+#else
+#define free_thread_struct(p) do { } while (0)
+#endif
 
 extern void exit_files(struct task_struct *);
 extern void __cleanup_sighand(struct sighand_struct *);
kernel/fork.c +3 -1
index 25e4291..ffcabf3 100644
@@ -1179,7 +1179,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		retval = -ENOMEM;
 		pid = alloc_pid(p->nsproxy->pid_ns);
 		if (!pid)
-			goto bad_fork_cleanup_io;
+			goto bad_fork_free_thread;
 
 		if (clone_flags & CLONE_NEWPID) {
 			retval = pid_ns_prepare_proc(p->nsproxy->pid_ns);

@@ -1315,6 +1315,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 bad_fork_free_pid:
 	if (pid != &init_struct_pid)
 		free_pid(pid);
+bad_fork_free_thread:
+	free_thread_struct(p);
 bad_fork_cleanup_io:
 	if (p->io_context)
 		exit_io_context(p);
Signature · 1 line
1.7.3.4
#1 lore raw
Quoted 18 lines · Tadashi Abe

x86 copy_thread() allocates io_bitmap_ptr area in child's thread_struct when the parent has TIF_IO_BITMAP flag via ioperm().
And in this case, if copy_process() terminates with errors after copy_thread() success,
this io_bitmap_ptr area is being left without kfree().

Signed-off-by Tadashi Abe <[email protected]>
---
 arch/x86/kernel/process.c |    6 ++++++
 include/linux/sched.h     |    5 +++++
 kernel/fork.c             |    4 +++-
 3 files changed, 14 insertions(+), 1 deletions(-)
arch/x86/kernel/process.c +0 -0
index b3feabc..290e98b 100644

arch/x86/kernel/process.c void flush_thread(void) · line 128

Quoted 7 lines
arch/x86/kernel/process.c +4 -0
@@ -122,6 +122,12 @@ void flush_thread(void)
 	clear_used_math();
 }
 
+void free_thread_struct(struct task_struct *p)
+{
+	if (p->thread.io_bitmap_ptr)
+		kfree(p->thread.io_bitmap_ptr);

kfree(NULL) is perfectly legal, so the 'if' is not needed.

Why is this a new function? Why not simply call kfree() when needed.

Quoted 10 lines
arch/x86/kernel/process.c +2 -0
+}
+
 static void hard_disable_TSC(void)
 {
 	write_cr4(read_cr4() | X86_CR4_TSD);
include/linux/sched.h +0 -0
index 23e9c27..2345391 100644

include/linux/sched.h extern int copy_thread(unsigned long, unsigned long, unsigned long, · line 2167

Quoted 9 lines
include/linux/sched.h +5 -0
@@ -2159,6 +2159,11 @@ extern int copy_thread(unsigned long, unsigned long, unsigned long,
 			struct task_struct *, struct pt_regs *);
 extern void flush_thread(void);
 extern void exit_thread(void);
+#ifdef CONFIG_X86
+extern void free_thread_struct(struct task_struct *p);
+#else
+#define free_thread_struct(p) do { } while (0)
+#endif

This should just go away IMHO.

Quoted 16 lines
include/linux/sched.h +0 -0
 extern void exit_files(struct task_struct *);
 extern void __cleanup_sighand(struct sighand_struct *);
kernel/fork.c +1 -1
index 25e4291..ffcabf3 100644
@@ -1179,7 +1179,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		retval = -ENOMEM;
 		pid = alloc_pid(p->nsproxy->pid_ns);
 		if (!pid)
-			goto bad_fork_cleanup_io;
+			goto bad_fork_free_thread;
 
 		if (clone_flags & CLONE_NEWPID) {
 			retval = pid_ns_prepare_proc(p->nsproxy->pid_ns);

kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1319

Quoted 5 lines
kernel/fork.c +2 -0
@@ -1315,6 +1315,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 bad_fork_free_pid:
 	if (pid != &init_struct_pid)
 		free_pid(pid);
+bad_fork_free_thread:
+	free_thread_struct(p);

kfree(p);
and be done with it..

kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1322

Quoted 4 lines
kernel/fork.c +0 -0
 bad_fork_cleanup_io:
 	if (p->io_context)
 		exit_io_context(p);

Perhaps I'm overlooking something, in which case I'd be happy to be corrected, but I really can't see why this can't just be as simple as I outlined above.

Signature · 3 lines
Jesper Juhl <[email protected]>            http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
#2 lore raw

Small correction to my previous mail below.

Quoted 68 lines · Jesper Juhl
Quoted 18 lines · Tadashi Abe

x86 copy_thread() allocates io_bitmap_ptr area in child's thread_struct when the parent has TIF_IO_BITMAP flag via ioperm().
And in this case, if copy_process() terminates with errors after copy_thread() success,
this io_bitmap_ptr area is being left without kfree().

Signed-off-by Tadashi Abe <[email protected]>
---
 arch/x86/kernel/process.c |    6 ++++++
 include/linux/sched.h     |    5 +++++
 kernel/fork.c             |    4 +++-
 3 files changed, 14 insertions(+), 1 deletions(-)
arch/x86/kernel/process.c +0 -0
index b3feabc..290e98b 100644

arch/x86/kernel/process.c void flush_thread(void) · line 128

Quoted 7 lines
arch/x86/kernel/process.c +4 -0
@@ -122,6 +122,12 @@ void flush_thread(void)
 	clear_used_math();
 }
 
+void free_thread_struct(struct task_struct *p)
+{
+	if (p->thread.io_bitmap_ptr)
+		kfree(p->thread.io_bitmap_ptr);

kfree(NULL) is perfectly legal, so the 'if' is not needed.

Why is this a new function? Why not simply call kfree() when needed.

Quoted 10 lines
arch/x86/kernel/process.c +2 -0
+}
+
 static void hard_disable_TSC(void)
 {
 	write_cr4(read_cr4() | X86_CR4_TSD);
include/linux/sched.h +0 -0
index 23e9c27..2345391 100644

include/linux/sched.h extern int copy_thread(unsigned long, unsigned long, unsigned long, · line 2167

Quoted 9 lines
include/linux/sched.h +5 -0
@@ -2159,6 +2159,11 @@ extern int copy_thread(unsigned long, unsigned long, unsigned long,
 			struct task_struct *, struct pt_regs *);
 extern void flush_thread(void);
 extern void exit_thread(void);
+#ifdef CONFIG_X86
+extern void free_thread_struct(struct task_struct *p);
+#else
+#define free_thread_struct(p) do { } while (0)
+#endif

This should just go away IMHO.

Quoted 13 lines
include/linux/sched.h +0 -0
 extern void exit_files(struct task_struct *);
 extern void __cleanup_sighand(struct sighand_struct *);
kernel/fork.c +1 -1
index 25e4291..ffcabf3 100644
@@ -1179,7 +1179,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 		retval = -ENOMEM;
 		pid = alloc_pid(p->nsproxy->pid_ns);
 		if (!pid)
-			goto bad_fork_cleanup_io;
+			goto bad_fork_free_thread;
Quoted 11 lines

kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1319

Quoted 8 lines
kernel/fork.c +2 -0
 		if (clone_flags & CLONE_NEWPID) {
 			retval = pid_ns_prepare_proc(p->nsproxy->pid_ns);

@@ -1315,6 +1315,8 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 bad_fork_free_pid:
 	if (pid != &init_struct_pid)
 		free_pid(pid);
+bad_fork_free_thread:
+	free_thread_struct(p);

kfree(p);
and be done with it..

I mean kfree(p->thread.io_bitmap_ptr); of course.

Quoted 11 lines

kernel/fork.c static struct task_struct *copy_process(unsigned long clone_flags, · line 1322

Quoted 5 lines
kernel/fork.c +0 -0
 bad_fork_cleanup_io:
 	if (p->io_context)
 		exit_io_context(p);

Perhaps I'm overlooking something, in which case I'd be happy to be corrected, but I really can't see why this can't just be as simple as I outlined above.

Signature · 3 lines
Jesper Juhl <[email protected]>            http://www.chaosbits.net/
Plain text mails only, please.
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
author
#3 lore raw
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
arch/x86/kernel/process.c +0 -0
index b3feabc..290e98b 100644
@@ -122,6 +122,12 @@ void flush_thread(void)
 	clear_used_math();
 }
Quoted 6 lines
Quoted 6 lines

arch/x86/kernel/process.c void flush_thread(void) · line 128

Quoted 4 lines
arch/x86/kernel/process.c +4 -0
+void free_thread_struct(struct task_struct *p)
+{
+	if (p->thread.io_bitmap_ptr)
+		kfree(p->thread.io_bitmap_ptr);

kfree(NULL) is perfectly legal, so the 'if' is not needed.

Thanks. agreed.

Quoted 1 line
Quoted 1 line

Why is this a new function? Why not simply call kfree() when needed.

io_bitmap_ptr is a x86 specific variable
so I didn't think it should be in generic kernel codes (kernel/fork.c). I understand making a new function looks overkill though. Even if you call kfree() directly in kernel/fork.c,
"#ifdef CONFIG_X86" is necessary anyway.

But I don't have any insistence on how to fix it and leave it to experts' choice.

Thanks
--
Tadashi