Q: utrace->stopped && utrace_report_jctl()
Oleg Nesterov
oleg at redhat.com
Sun Mar 15 22:33:00 UTC 2009
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.
More information about the utrace-devel
mailing list