[PATCH] -mm check_rlimit oops on p->signal

[PATCH] -mm check_rlimit oops on p->signal

[Hugh Dickins]

The p->signal check in account_system_time is insufficient.  If the
timer interrupt hits near the end of exit_notify, after EXIT_ZOMBIE has
been set, another cpu may release_task (NULLifying p->signal) in between
account_system_time's check and check_rlimit's dereference.  Nor should
account_it_prof risk send_sig.  But surely account_user_time is safe?

Signed-off-by: Hugh Dickins <hugh@veritas.com>

--- 2.6.10-rc1-mm5/kernel/sched.c	2004-11-11 12:40:12.000000000 +0000
+++ linux/kernel/sched.c	2004-11-14 20:41:26.851384984 +0000
@@ -2333,8 +2333,7 @@ void account_user_time(struct task_struc
 	p->utime = cputime_add(p->utime, cputime);
 
 	/* Check for signals (SIGVTALRM, SIGPROF, SIGXCPU & SIGKILL). */
-	if (likely(p->signal))
-		check_rlimit(p, cputime);
+	check_rlimit(p, cputime);
 	account_it_virt(p, cputime);
 	account_it_prof(p, cputime);
 
@@ -2362,9 +2361,10 @@ void account_system_time(struct task_str
 	p->stime = cputime_add(p->stime, cputime);
 
 	/* Check for signals (SIGPROF, SIGXCPU & SIGKILL). */
-	if (likely(p->signal))
+	if (likely(p->signal && p->exit_state < EXIT_ZOMBIE)) {
 		check_rlimit(p, cputime);
-	account_it_prof(p, cputime);
+		account_it_prof(p, cputime);
+	}
 
 	/* Add system time to cpustat. */
 	tmp = cputime_to_cputime64(cputime);

Re: [PATCH] -mm check_rlimit oops on p->signal

[Andrew Morton]

Hugh Dickins <hugh@veritas.com> wrote:
>
> +	if (likely(p->signal && p->exit_state < EXIT_ZOMBIE)) {

Worried.  This places an ordering interpretation on TASK_* and EXIT_* which
AFAIK hadn't been there beforehand.  If someone later comes along and adds

#define TASK_DOODLING 64

then we lose.

I wonder if for clarity and future-safety we should do something like:

--- 25/include/linux/sched.h~task-exit_state-clarity	Mon Nov 15 15:40:24 2004
+++ 25-akpm/include/linux/sched.h	Mon Nov 15 15:42:40 2004
@@ -105,13 +105,20 @@ extern unsigned long nr_iowait(void);
 
 #include <asm/processor.h>
 
+/*
+ * Tasks whose exit_state is less that TASK_EXIT_MARKER are considered to
+ * be still running.  Tasks whose exit_state is greater than TASK_EXIT_MARKER
+ * are in the process of exitting.  TASK_EXIT_MARKER is never actually set in
+ * task_struct.exit_state.
+ */
 #define TASK_RUNNING		0
 #define TASK_INTERRUPTIBLE	1
 #define TASK_UNINTERRUPTIBLE	2
 #define TASK_STOPPED		4
 #define TASK_TRACED		8
-#define EXIT_ZOMBIE		16
-#define EXIT_DEAD		32
+#define TASK_EXIT_MARKER	16	
+#define EXIT_ZOMBIE		32
+#define EXIT_DEAD		64
 
 #define __set_task_state(tsk, state_value)		\
 	do { (tsk)->state = (state_value); } while (0)
_

It seems a bit dorky for some reason...

Re: [PATCH] -mm check_rlimit oops on p->signal

[Hugh Dickins]

On Mon, 15 Nov 2004, Andrew Morton wrote:
> Hugh Dickins <hugh@veritas.com> wrote:
> >
> > +	if (likely(p->signal && p->exit_state < EXIT_ZOMBIE)) {
> 
> Worried.  This places an ordering interpretation on TASK_* and EXIT_* which
> AFAIK hadn't been there beforehand.  If someone later comes along and adds

I understand your concern, but kernel/exit.c already contained several
tests for "exit_state >= EXIT_ZOMBIE", so I followed those precedents.

Hugh