Re: [PATCH] fix signal->live leak in copy_process()

[Roland McGrath]

Looks right to me.  exit_signal really could use a comment explaining that
the error path is the only place it gets called, or perhaps it would be
cleaner just to do away with it and have do_fork do that work directly in
its error path (i.e. at bad_fork_cleanup_signal: label).  It's less than
obvious when we have the inc in fork.c, the normal case dec in exit.c, and
the error path dec in signal.c.  The following is the same as Oleg's except
for this cosmetic cleanup.


Thanks,
Roland


---
diff-tree a1955c794d1aa805c09cd1c50022ac115438f1e2 (from f0ec9bce933e45bdf732b7a9b27a43d53f3de247)
tree 87c15be65e9ed71feb40f88661490df1c7f30f8d
parent f0ec9bce933e45bdf732b7a9b27a43d53f3de247
author Roland McGrath <roland@xxxxxxxxxx> 1130625045 -0700
committer Roland McGrath <roland@xxxxxxxxxxxxxxxxxxx> 1130625045 -0700

    Fix signal->live leak in copy_process
    
    Oleg Nesterov pointed out that ->signal->live will be incremented and then
    never decremented in certain error paths out of do_fork, leading to leaks
    when cleanup is never done after the last thread in the group dies.
    
    This patch fixes that with a bit of cleanup.  The only caller of
    exit_signal was in this error path.  I've removed that function and moved
    its replacement into fork.c, so the decrement and the increment are done in
    the same source file where it's easier to follow what's going on.
    
    Signed-off-by: Roland McGrath <roland@xxxxxxxxxx>

diff --git a/include/linux/sched.h b/include/linux/sched.h
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1093,7 +1093,6 @@ extern void flush_thread(void);
 extern void exit_thread(void);
 
 extern void exit_files(struct task_struct *);
-extern void exit_signal(struct task_struct *);
 extern void __exit_signal(struct task_struct *);
 extern void exit_sighand(struct task_struct *);
 extern void __exit_sighand(struct task_struct *);
diff --git a/kernel/fork.c b/kernel/fork.c
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -844,6 +844,22 @@ static inline int copy_signal(unsigned l
 	return 0;
 }
 
+/*
+ * Called for error cleanup after copy_signal succeeded.
+ */
+static inline void uncopy_signal(struct task_struct *tsk)
+{
+	/*
+	 * This decrement would ordinarily be done by do_exit.
+	 * We know it won't hit zero for a shared signal_struct,
+	 * because the current thread is still live.
+	 */
+	atomic_dec(&tsk->signal->live);
+	write_lock_irq(&tasklist_lock);
+	__exit_signal(tsk);
+	write_unlock_irq(&tasklist_lock);
+}
+
 static inline void copy_flags(unsigned long clone_flags, struct task_struct *p)
 {
 	unsigned long new_flags = p->flags;
@@ -1167,7 +1183,7 @@ bad_fork_cleanup_mm:
 	if (p->mm)
 		mmput(p->mm);
 bad_fork_cleanup_signal:
-	exit_signal(p);
+	uncopy_signal(p);
 bad_fork_cleanup_sighand:
 	exit_sighand(p);
 bad_fork_cleanup_fs:
diff --git a/kernel/signal.c b/kernel/signal.c
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -405,13 +405,6 @@ void __exit_signal(struct task_struct *t
 	}
 }
 
-void exit_signal(struct task_struct *tsk)
-{
-	write_lock_irq(&tasklist_lock);
-	__exit_signal(tsk);
-	write_unlock_irq(&tasklist_lock);
-}
-
 /*
  * Flush all handlers for a task.
  */

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/