Skip to contentSkip to conversation

fix do_coredump() vs SIGSTOP race

PATCH In review Oleg Nesterov 2 participants 2 messages Oct 7, 2005 – Apr 7, 2006

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 fix do_coredump() vs SIGSTOP race

Let's suppose we have 2 threads in thread group:
A - does coredump
B - has pending SIGSTOP

thread A thread B

do_coredump: get_signal_to_deliver:

lock(->sighand)
->signal->flags = SIGNAL_GROUP_EXIT
unlock(->sighand)

							lock(->sighand)
							signr = dequeue_signal()
								->signal->flags |= SIGNAL_STOP_DEQUEUED
								return SIGSTOP;

							do_signal_stop:
							    unlock(->sighand)

coredump_wait:

      zap_threads:
          lock(tasklist_lock)
          send SIGKILL to B
              // signal_wake_up() does nothing
          unlock(tasklist_lock)

							    lock(tasklist_lock)
							    lock(->sighand)
							    re-check sig->flags & SIGNAL_STOP_DEQUEUED, yes
							    set_current_state(TASK_STOPPED);
							    finish_stop:
							        schedule();
							            // ->state == TASK_STOPPED

      wait_for_completion(&startup_done)
         // waits for complete() from B,
         // ->state == TASK_UNINTERRUPTIBLE

We can't wake up 'B' in any way:

SIGCONT will be ignored because handle_stop_signal() sees ->signal->flags & SIGNAL_GROUP_EXIT.

sys_kill(SIGKILL)->__group_complete_signal() will choose uninterruptible 'A', so it can't help.

sys_tkill(B, SIGKILL) will be ignored by specific_send_sig_info() because B already has pending SIGKILL.

This scenario is not possbile if 'A' does do_group_exit(), because it sets sig->flags = SIGNAL_GROUP_EXIT and delivers SIGKILL to subthreads atomically, holding both tasklist_lock and sighand->lock. That means that do_signal_stop() will notice !SIGNAL_STOP_DEQUEUED after re-locking ->sighand. And it is not possible to any other thread to re-add SIGNAL_STOP_DEQUEUED later, because dequeue_signal() can only return SIGKILL.

I think it is better to change do_coredump() to do sigaddset(SIGKILL) and signal_wake_up() under sighand->lock, but this patch is much simpler.

kernel/signal.c renamed from kernel/signal.c~ +2 -1
@@ -578,7 +578,8 @@ int dequeue_signal(struct task_struct *t
  		 * is to alert stop-signal processing code when another
  		 * processor has come along and cleared the flag.
  		 */
- 		tsk->signal->flags |= SIGNAL_STOP_DEQUEUED;
+ 		if (!(tsk->signal->flags & SIGNAL_GROUP_EXIT))
+ 			tsk->signal->flags |= SIGNAL_STOP_DEQUEUED;
  	}
 	if ( signr &&
 	     ((info->si_code & __SI_MASK) == __SI_TIMER) &&
#1 lore raw

Sorry I have been absent from the discussion for such a long time. I'm trying to catch up on old issues that I should have reviewed earlier.

I like this change. I think it's sensible to keep the invariant that SIGNAL_STOP_DEQUEUED can't be set when SIGNAL_GROUP_EXIT is set. Good work, Oleg.

Thanks,
Roland