Fix signal race during process exit

PATCH In review Jeremy Kerr 3 participants 6 messages Jun 2, 2004

lore

Share & tools

lore

Jeremy Kerr

author

2004-06-02 02:18 UTC #0 lore raw

PATCH 1/1 Fix signal race during process exit

Andrew,

This patch fixes a race where timer-generated signals are delivered to an exiting process, after task->sighand is cleared.

update_one_process() declared static, as it is only used in kernel/timer.c

Signed-off-by Jeremy Kerr <[email protected]>

Jeremy

diff -urN linux-2.6.7-rc2-bk2.orig/include/linux/sched.h linux-2.6.7-rc2-bk2/include/linux/sched.h
--- linux-2.6.7-rc2-bk2.orig/include/linux/sched.h 2004-06-02
11:29:13.000000000 +1000
+++ linux-2.6.7-rc2-bk2/include/linux/sched.h 2004-06-02 11:46:57.000000000
+1000
@@ -168,8 +168,6 @@
extern void cpu_init (void);
extern void trap_init(void);
extern void update_process_times(int user);
-extern void update_one_process(struct task_struct *p, unsigned long user,
- unsigned long system, int cpu);
extern void scheduler_tick(int user_tick, int system);
extern unsigned long cache_decay_ticks;

Binary files linux-2.6.7-rc2-bk2.orig/kernel/.signal.c.swp and linux-2.6.7-rc2-bk2/kernel/.signal.c.swp differ Binary files linux-2.6.7-rc2-bk2.orig/kernel/.timer.c.swp and linux-2.6.7-rc2-bk2/kernel/.timer.c.swp differ
diff -urN linux-2.6.7-rc2-bk2.orig/kernel/signal.c linux-2.6.7-rc2-bk2/kernel/signal.c
--- linux-2.6.7-rc2-bk2.orig/kernel/signal.c 2004-06-02 11:29:13.000000000
+1000
+++ linux-2.6.7-rc2-bk2/kernel/signal.c 2004-06-02 11:47:28.000000000 +1000
@@ -323,7 +323,10 @@
{
struct sighand_struct * sighand = tsk->sighand;

- /* Ok, we're done with the signal handlers */
+ /* Ok, we're done with the signal handlers.
+ * Set sighand to NULL to tell kernel/timer.c not
+ * to deliver further signals to this task
+ */
tsk->sighand = NULL;
if (atomic_dec_and_test(&sighand->count))
kmem_cache_free(sighand_cachep, sighand);
diff -urN linux-2.6.7-rc2-bk2.orig/kernel/timer.c linux-2.6.7-rc2-bk2/kernel/timer.c
--- linux-2.6.7-rc2-bk2.orig/kernel/timer.c 2004-06-02 11:29:13.000000000
+1000
+++ linux-2.6.7-rc2-bk2/kernel/timer.c 2004-06-02 11:47:08.000000000 +1000
@@ -829,7 +829,7 @@
}
}

-void update_one_process(struct task_struct *p, unsigned long user, +static void update_one_process(struct task_struct *p, unsigned long user,
unsigned long system, int cpu)
{
do_process_times(p, user, system);
@@ -846,7 +846,9 @@
struct task_struct *p = current;
int cpu = smp_processor_id(), system = user_tick ^ 1;

- update_one_process(p, user_tick, system, cpu);
+ /* Don't send signals to current after release_task() */
+ if (likely(p->sighand))
+ update_one_process(p, user_tick, system, cpu);
run_local_timers();
scheduler_tick(user_tick, system);
}

Andrew Morton

2004-06-02 05:57 UTC #1 lore raw

Quoted 3 lines · Jeremy Kerr

This patch fixes a race where timer-generated signals are delivered to an
exiting process, after task->sighand is cleared.

Nasty. I'm surprised that we haven't hit this more frequently. I guess timer-generated signals aren't very common.

However I'm not sure that your fix is complete:

void update_process_times(int user_tick)

{
	struct task_struct *p = current;
	int cpu = smp_processor_id(), system = user_tick ^ 1;

	/* Don't send signals to current after release_task() */
	if (likely(p->sighand))
		update_one_process(p, user_tick, system, cpu);

versus:

void __exit_sighand(struct task_struct *tsk)

{
	struct sighand_struct * sighand = tsk->sighand;

	/* Ok, we're done with the signal handlers.
	 * Set sighand to NULL to tell kernel/timer.c not
	 * to deliver further signals to this task
	 */
	tsk->sighand = NULL;
	if (atomic_dec_and_test(&sighand->count))
		kmem_cache_free(sighand_cachep, sighand);

If these two functions are running on different CPUs then the race is still there - exit_sighand() can call update_process_times() while __exit_sighand is throwing away p->sighand.

Question is, can these functions run on separate CPUs? Certainly a different CPU can run release_task(), via wait4().

And there's a little window at the end of exit_notify() where the exitting task (which is still "current" on its CPU) can take a timer interrupt while in a state TASK_ZOMBIE. The CPU which is running wait4() will run release_task() for the exitting task and the above race can occur.

(And if the exitting task is being ptraced things get more complex..)

Did I miss something?

It seems silly to be trying to deliver timer signals to processes which are so late in exit and we could perhaps set ->it_prof_value and ->it_virt_value to zero earlier in exit. That's sane, but doesn't fix the race.

Right now, I see no alternative to adding locking which pins task->sighand while the timer handler is running. Taking tasklist_lock on each timer tick will hurt - maybe a new per-process lock is needed?

Rusty Russell

2004-06-02 06:50 UTC #2 lore raw

Quoted 9 lines · Andrew Morton

void update_process_times(int user_tick)
{
	struct task_struct *p = current;
	int cpu = smp_processor_id(), system = user_tick ^ 1;
versus:
void __exit_sighand(struct task_struct *tsk)
{
	struct sighand_struct * sighand = tsk->sighand;

No. tsk == current for __exit_sighand. You know, getting current is SO expensive, and so we should PASS IT to functions explicitly!

One of my pet gripes: this code is badly obfuscated by this. Is it just me?

Quoted 4 lines

And there's a little window at the end of exit_notify() where the exitting task (which is still "current" on its CPU) can take a timer interrupt while in a state TASK_ZOMBIE. The CPU which is running wait4() will run release_task() for the exitting task and the above race can occur.

Hmm, while we're at it, the task seems to release itself while running
here: exit_notify() -> release_task() -> put_task_struct() -> __put_task_struct() -> BOOM?

Surely not, what am I missing?

Quoted 3 lines

Right now, I see no alternative to adding locking which pins task->sighand while the timer handler is running. Taking tasklist_lock on each timer tick will hurt - maybe a new per-process lock is needed?

Hmm, a per-cpu cache of exited tasks: one task for each CPU. We hold a reference to the task struct until the next exit on the same CPU happens? We could also reuse that cache for fork()...

Rusty.

Signature · 1 line

Anyone who quotes me in their signature is an idiot -- Rusty Russell

Andrew Morton

2004-06-02 07:08 UTC #3 lore raw

Quoted 8 lines · Rusty Russell

Quoted 6 lines · Andrew Morton
void update_process_times(int user_tick)
{
	struct task_struct *p = current;
	int cpu = smp_processor_id(), system = user_tick ^ 1;
versus:

Quoted 5 lines

Quoted 3 lines
void __exit_sighand(struct task_struct *tsk)
{
	struct sighand_struct * sighand = tsk->sighand;
No. tsk == current for __exit_sighand.

Nope.

	wait4->wait_task_zombie->release_task->__exit_sighand

Quoted 5 lines

You know, getting current is SO
expensive, and so we should PASS IT to functions explicitly!
One of my pet gripes: this code is badly obfuscated by this. Is it just me?

It does make the code a bit hard to follow, but yes, evaluating `current' takes ~14 bytes of text on x86. Although I think recent versions of the compiler are able to cse it.

Still, the right way to do this is to lose all the dopey p's and tsk's and adopt a convention of using `this_task' or some such identifier.

Quoted 10 lines

Quoted 4 lines
And there's a little window at the end of exit_notify() where the exitting task (which is still "current" on its CPU) can take a timer interrupt while in a state TASK_ZOMBIE. The CPU which is running wait4() will run release_task() for the exitting task and the above race can occur.
Hmm, while we're at it, the task seems to release itself while running
here: exit_notify() -> release_task() -> put_task_struct() -> __put_task_struct() -> BOOM?
Surely not, what am I missing?

There's still one put to go - that happens at the end of finish_task_switch(), via the next-to-run process.

Quoted 7 lines

Quoted 3 lines
Right now, I see no alternative to adding locking which pins task->sighand while the timer handler is running. Taking tasklist_lock on each timer tick will hurt - maybe a new per-process lock is needed?
Hmm, a per-cpu cache of exited tasks: one task for each CPU. We hold a reference to the task struct until the next exit on the same CPU happens? We could also reuse that cache for fork()...

The problem is task_struct.sighand, not the task_struct itself. ->sighand can be thrown away while update_process_times() is still poking at it.

hmm, maybe it _is_ sufficient to null out current->it_prof_value and current->it_virt_incr in the do_exit() path. Because in *this* case we know that it is always the exitting task which is doing the nulling, so the timer interrupt cannot run on a different CPU.

kernel/exit.c renamed from kernel/exit.c~a +7 -0

@@ -749,6 +749,13 @@ static void exit_notify(struct task_stru
 	 * complete, and with interrupts blocked that will never happen.
 	 */
 	_raw_write_unlock(&tasklist_lock);
+
+	/*
+	 * comment goes here
+	 */
+	tsk->it_virt_incr = 0;
+	tsk->it_prof_value = 0;
+
 	local_irq_enable();
 
 	/* If the process is dead, release it - nobody will wait for it */

yes?

Andrew Morton

2004-06-02 07:17 UTC #4 lore raw

Quoted 2 lines · Andrew Morton

yes?

no. It needs tasklist_lock as well, to keep the other CPU (which is doing wait4) at bay.

Fix a race identified by Jeremy Kerr <[email protected]>: if update_process_times() decides to deliver a signal due to process timer expiry, it can race with __exit_sighand()'s freeing of task->sighand.

Fix that by clearing the per-process timer state in exit_notify(), while under local_irq_disable() and under tasklist_lock. tasklist_lock provides exclusion wrt release_task()'s freeing of task->sighand and local_irq_disable() provides exclusion wrt update_process_times()'s inspection of the per-process timer state.

Signed-off-by Andrew Morton <[email protected]>

---

 25-akpm/kernel/exit.c |    7 +++++++
 1 files changed, 7 insertions(+)

kernel/exit.c renamed from kernel/exit.c~really-fix-signal-race-during-process-exit +7 -0

diff -puN kernel/exit.c~really-fix-signal-race-during-process-exit kernel/exit.c
@@ -737,6 +737,13 @@ static void exit_notify(struct task_stru
 	tsk->flags |= PF_DEAD;
 
 	/*
+	 * Clear these here so that update_process_times() won't try to deliver
+	 * itimer signals to this task while it is in late exit.
+	 */
+	tsk->it_virt_incr = 0;
+	tsk->it_prof_value = 0;
+
+	/*
 	 * In the preemption case it must be impossible for the task
 	 * to get runnable again, so use "_raw_" unlock to keep
 	 * preempt_count elevated until we schedule().

Jeremy Kerr

author

2004-06-02 08:18 UTC #5 lore raw

Quoted 5 lines

Quoted 1 line · Andrew Morton
yes?
no. It needs tasklist_lock as well, to keep the other CPU (which is doing wait4) at bay.

almost:

Review

Quoted 2 lines

(quoted diff) +2 -0

+	tsk->it_virt_incr = 0;
+	tsk->it_prof_value = 0;

If we're using this approach, we also need to deal with the send_sig() calls in do_process_times():

	if (psecs / HZ > p->rlim[RLIMIT_CPU].rlim_cur) {
		/* Send SIGXCPU every second.. */
		if (!(psecs % HZ))
			send_sig(SIGXCPU, p, 1);
		/* and SIGKILL when we go over max.. */
		if (psecs / HZ > p->rlim[RLIMIT_CPU].rlim_max)
			send_sig(SIGKILL, p, 1);
	}

by setting rlim_cur to RLIM_INFINITY.

Signed-off-by Andrew Morton <[email protected]>

Signed-off-by Jeremy Kerr <[email protected]>

diff -urN --exclude '.*.sw[op]' linux-2.6.7-rc2-bk2.orig/kernel/exit.c linux-2.6.7-rc2-bk2/kernel/exit.c

kernel/exit.c +8 -0

@@ -736,6 +736,14 @@
 	tsk->state = state;
 	tsk->flags |= PF_DEAD;
 
+	/*
+	 * Clear these here so that update_process_times() won't try to deliver
+	 * itimer, profile or rlimit signals to this task while it is in late exit.
+	 */
+	tsk->it_virt_incr = 0;
+	tsk->it_prof_value = 0;
+	tsk->rlim[RLIMIT_CPU].rlim_cur = RLIM_INFINITY;
+
 	/*
 	 * In the preemption case it must be impossible for the task
 	 * to get runnable again, so use "_raw_" unlock to keep

Jeremy