On 03/14, Oleg Nesterov wrote: > > On 03/12, Roland McGrath wrote: > > > > But, yes, this is a problem. I think this ought to cover it: > > > > @@ -1659,6 +1659,16 @@ void utrace_report_jctl(int notify, int what) > > * longer considered stopped while we run callbacks. > > */ > > spin_lock(&utrace->lock); > > + /* > > + * Now that we have the lock, check in case utrace_reset() has > > + * just now cleared UTRACE_EVENT(JCTL) while it considered us > > + * safely stopped. In that case, we should not touch ->stopped > > + * and have nothing else to do. > > + */ > > + if (unlikely(!(task->utrace_flags & UTRACE_EVENT(JCTL)))) { > > + spin_unlock(&utrace->lock); > > + return; > > I don't think this can help, even if we clear ->stopped before return. > It is still possible to set ->stopped after that, and since we don't > have JCTL we return from get_signal_to_deliver() bypassing tracehook > calls.
I was wrong, I forgot that tracehook_get_signal() doesn't need JCTL. OK, let's look at utrace_do_stop: if (task_is_stopped(target) && !(target->utrace_flags & UTRACE_EVENT(JCTL))) { utrace->stopped = 1; return true; } This doesn't look correct. We don't hold ->siglock, the task can be SIGCONT'ed and return from get_signal_to_deliver(), and then we set ->stopped. Or I missed something again? Then we re-do this (well, almost) check under ->siglock, } else if (task_is_stopped(target)) { if (!(target->utrace_flags & UTRACE_EVENT(JCTL))) utrace->stopped = stopped = true; } But this is not nice. Let's suppose the task is already stopped, we do UTRACE_ATTACH + utrace_set_events(JCTL). Now, utrace_control(UTRACE_STOP) can do nothing until SIGCONT. We don't even set ->report. Yes, we can't set ->stopped if JCTL, we can race with utrace_report_jctl() which does REPORT(). BTW, afaics utrace_report_jctl() has another bug, REPORT(task, utrace, &report, UTRACE_EVENT(JCTL), report_jctl, was_stopped ? CLD_STOPPED : CLD_CONTINUED, what); I think it should do REPORT(task, utrace, &report, UTRACE_EVENT(JCTL), report_jctl, what, notify); instead. > Roland, I _seem_ to have the vague idea, will return tomorrow. Well, this idea is not very nice. But see the draft patches below. With the first patch, we call utrace_report_jctl() before we actually stop. do_signal_stop() can fail then, but I think this is OK, we can pretend that SIGCONT/SIGKILL happened after we stopped. It is not complete, and with this patch we always call ->report_jctl with notify == 0. Just for discussion. --- xxx/include/linux/utrace.h~JCTL 2009-03-03 20:43:43.000000000 +0100 +++ xxx/include/linux/utrace.h 2009-03-15 21:55:45.000000000 +0100 @@ -102,7 +102,7 @@ void utrace_report_exit(long *exit_code) __attribute__((weak)); void utrace_report_death(struct task_struct *, struct utrace *, bool, int) __attribute__((weak)); -void utrace_report_jctl(int notify, int type) +bool utrace_report_jctl(bool sig_locked, int what) __attribute__((weak)); void utrace_report_exec(struct linux_binfmt *, struct linux_binprm *, struct pt_regs *regs) --- xxx/include/linux/tracehook.h~JCTL 2009-03-03 20:40:57.000000000 +0100 +++ xxx/include/linux/tracehook.h 2009-03-15 22:02:05.000000000 +0100 @@ -521,11 +521,11 @@ static inline int tracehook_get_signal(s * * Called with no locks held. */ -static inline int tracehook_notify_jctl(int notify, int why) +static inline bool tracehook_notify_jctl(bool sig_locked, int why) { if (task_utrace_flags(current) & UTRACE_EVENT(JCTL)) - utrace_report_jctl(notify, why); - return notify || (current->ptrace & PT_PTRACED); + return utrace_report_jctl(sig_locked, why); + return true; } #define DEATH_REAP -1 --- xxx/kernel/utrace.c~JCTL 2009-03-12 01:21:05.000000000 +0100 +++ xxx/kernel/utrace.c 2009-03-15 22:59:36.000000000 +0100 @@ -1637,12 +1637,14 @@ void utrace_finish_vfork(struct task_str /* * Called iff UTRACE_EVENT(JCTL) flag is set. */ -void utrace_report_jctl(int notify, int what) +bool utrace_report_jctl(bool sig_locked, int what) { struct task_struct *task = current; struct utrace *utrace = task_utrace_struct(task); INIT_REPORT(report); - bool was_stopped = task_is_stopped(task); + + if (sig_locked) + spin_unlock_irq(&task->sighand->siglock); /* * We get here with CLD_STOPPED when we've just entered @@ -1664,30 +1662,12 @@ void utrace_report_jctl(int notify, int spin_unlock(&utrace->lock); REPORT(task, utrace, &report, UTRACE_EVENT(JCTL), - report_jctl, was_stopped ? CLD_STOPPED : CLD_CONTINUED, what); + report_jctl, what, 0); - if (was_stopped && !task_is_stopped(task)) { - /* - * The event report hooks could have blocked, though - * it should have been briefly. Make sure we're in - * TASK_STOPPED state again to block properly, unless - * we've just come back out of job control stop. - */ + if (sig_locked) spin_lock_irq(&task->sighand->siglock); - if (task->signal->flags & SIGNAL_STOP_STOPPED) - __set_current_state(TASK_STOPPED); - spin_unlock_irq(&task->sighand->siglock); - } - if (task_is_stopped(current)) { - /* - * While in TASK_STOPPED, we can be considered safely - * stopped by utrace_do_stop() only once we set this. - */ - spin_lock(&utrace->lock); - utrace->stopped = 1; - spin_unlock(&utrace->lock); - } + return task->signal->group_stop_count != 0; } /* --- xxx/kernel/signal.c~JCTL 2009-03-03 18:11:47.000000000 +0100 +++ xxx/kernel/signal.c 2009-03-15 22:07:30.000000000 +0100 @@ -1641,7 +1641,7 @@ finish_stop(int stop_count) * a group stop in progress and we are the last to stop, * report to the parent. When ptraced, every thread reports itself. */ - if (tracehook_notify_jctl(stop_count == 0, CLD_STOPPED)) { + if (stop_count == 0) { read_lock(&tasklist_lock); do_notify_parent_cldstop(current, CLD_STOPPED); read_unlock(&tasklist_lock); @@ -1785,8 +1785,7 @@ relock: signal->flags &= ~SIGNAL_CLD_MASK; spin_unlock_irq(&sighand->siglock); - if (unlikely(!tracehook_notify_jctl(1, why))) - goto relock; + tracehook_notify_jctl(false, why); read_lock(&tasklist_lock); do_notify_parent_cldstop(current->group_leader, why); @@ -1798,6 +1797,7 @@ relock: struct k_sigaction *ka; if (unlikely(signal->group_stop_count > 0) && + tracehook_notify_jctl(true, CLD_STOPPED) && do_signal_stop(0)) goto relock; @@ -1872,6 +1872,7 @@ relock: if (is_current_pgrp_orphaned()) goto relock; + tracehook_notify_jctl(false, CLD_STOPPED); spin_lock_irq(&sighand->siglock); } @@ -1953,7 +1954,8 @@ void exit_signals(struct task_struct *ts out: spin_unlock_irq(&tsk->sighand->siglock); - if (unlikely(group_stop) && tracehook_notify_jctl(1, CLD_STOPPED)) { + if (unlikely(group_stop)) { + tracehook_notify_jctl(false, CLD_STOPPED); read_lock(&tasklist_lock); do_notify_parent_cldstop(tsk, CLD_STOPPED); read_unlock(&tasklist_lock); ------------------------------------------------------------------------------- Now we can change utrace_do_stop(), no need to check JCTL any longer, --- xxx/kernel/utrace.c~STOP 2009-03-15 22:59:36.000000000 +0100 +++ xxx/kernel/utrace.c 2009-03-15 23:29:19.000000000 +0100 @@ -794,20 +794,6 @@ static bool utrace_do_stop(struct task_s { bool stopped; - /* - * If it will call utrace_report_jctl() but has not gotten - * through it yet, then don't consider it quiescent yet. - * utrace_report_jctl() will take @utrace->lock and - * set @utrace->stopped itself once it finishes. After that, - * it is considered quiescent; when it wakes up, it will go - * through utrace_get_signal() before doing anything else. - */ - if (task_is_stopped(target) && - !(target->utrace_flags & UTRACE_EVENT(JCTL))) { - utrace->stopped = 1; - return true; - } - stopped = false; spin_lock_irq(&target->sighand->siglock); if (unlikely(target->exit_state)) { @@ -819,8 +805,7 @@ static bool utrace_do_stop(struct task_s if (!(target->utrace_flags & DEATH_EVENTS)) utrace->stopped = stopped = true; } else if (task_is_stopped(target)) { - if (!(target->utrace_flags & UTRACE_EVENT(JCTL))) - utrace->stopped = stopped = true; + utrace->stopped = stopped = true; } else if (!utrace->report && !utrace->interrupt) { utrace->report = 1; set_notify_resume(target); ------------------------------------------------------------------------------- Again, this is not complete and likely buggy. But what do you think? Oleg.