On 07/27, Roland McGrath wrote:
>
> > (this change also breaks ptrace_resume() but this is fixeable).
>
> I can't tell which code you are talking about here. But the only
> ptrace_resume I care about is the new one true ptrace based from
> scratch on cleaned-up utrace, which you haven't written yet.
I meant ptrace_resume()->task_is_stopped() check in ptrace_utrace.patch.
> > But personally I never liked the fact that ptrace_check_attach() does
> > s/TASK_STOPPED/TASK_TRACED/. Because I think this creates the unfixeable
> > ptrace && jctl-group-stop problems. The bookkeeping of ->group_stop_count
> > in utrace_stop() (and in ptrace_stop()) is just wrong and imho should be
> > removed. And do_signal_stop() should not take TASK_TRACED threads into
> > account.
>
> Why do you think they are unfixable? The reason that switching into
> TASK_TRACED is good is that it (and only it) ensures that SIGCONT
> won't wake the thread up. As long as it's in TASK_STOPPED, any
> examination loop (prepare/finish_examine, or task_current_syscall,
> i.e. the "wait_task_inactive before and after" check for no csw)
> can experience a hiccup if a normal SIGCONT comes along. It seems far
> better that it just not wake up (i.e. any csw) while UTRACE_STOP,
Yes, this is true.
> For do_signal_stop it's right to consider TASK_TRACED threads as
> stopped. The purpose of the group-stop bookkeeping is to make sure
> that when we declare the whole process stopped (SIGNAL_STOP_STOPPED,
> send SIGCHLD to real parent, wait_task_stopped reports, etc.) then no
> more user instructions will run in any thread, nor any user syscall in
> progress complete. If some threads are in TASK_TRACED, then they
> won't run any such things (if we get the bookkeeping right). It would
> be wrong to make the whole-process stop fail to finish while the
> debugger holds one thread in TASK_TRACED.
Yes, the whole-process stop will be delayed until the debugger
wakes up the tracee and it sees signal_pending().
Not nice, agreed, but probably not too bad. The debugger doesn't
really care. As for real parent, it will be confused in any case.
It can't assume the whole process is really stopped, debugger
can wake up a thread at any moment.
> The bookkeeping should ensure that when a TASK_TRACED thread was
> counted as stopped,
But we can't know if it was already counted or not. And note that
utrace_stop() doesn't set SIGNAL_STOP_STOPPED and doesn't notify
if ->group_stop_count becomes 0.
> then utrace never wakes it up but instead puts it
> in TASK_STOPPED
only if group-stop is already completed.
> I'm open to all manner of reorganization of the bookkeeping.
> But please explain how what you propose addresses these ideas.
I meant
--- kernel/signal.c
+++ kernel/signal.c
@@ -1571,7 +1571,7 @@ static int do_signal_stop(int signr)
* so this check has no races.
*/
if (!(t->flags & PF_EXITING) &&
- !task_is_stopped_or_traced(t)) {
+ !task_is_stopped(t)) {
stop_count++;
signal_wake_up(t, 0);
}
--- kernel/utrace.c
+++ kernel/utrace.c
@@ -409,13 +409,6 @@ static bool utrace_stop(struct task_stru
utrace->stopped = 1;
__set_current_state(TASK_TRACED);
- /*
- * If there is a group stop in progress,
- * we must participate in the bookkeeping.
- */
- if (task->signal->group_stop_count > 0)
- --task->signal->group_stop_count;
-
spin_unlock_irq(&task->sighand->siglock);
spin_unlock(&utrace->lock);
for the start.
Or we can ignore all these problems, there are not new.
So. Let's change utrace_do_stop() to set TASK_TRACED,
--- kernel/utrace.c
+++ kernel/utrace.c
@@ -797,8 +797,10 @@ static bool utrace_do_stop(struct task_s
} else if (task_is_stopped(target)) {
/*
* Stopped is considered quiescent; when it wakes up,
it will
- * go through utrace_get_signal() before doing anything
else.
+ * go through utrace_report_jctl() or
utrace_get_signal()
+ * before doing anything else.
*/
+ __set_task_state(target, TASK_TRACED);
utrace->stopped = stopped = true;
} else if (!utrace->report && !utrace->interrupt) {
utrace->report = 1;
@@ -1913,6 +1915,15 @@ int utrace_get_signal(struct task_struct
int signr;
utrace = &task->utrace;
+
+ if (unlikely(utrace->stopped)) {
+ spin_unlock_irq(&task->sighand->siglock);
+ spin_lock(&utrace->lock);
+ utrace->stopped = 0;
+ spin_unlock(&utrace->lock);
+ spin_lock_irq(&task->sighand->siglock);
+ }
+
if (utrace->interrupt || utrace->report ||
utrace->signal_handler) {
/*
* We've been asked for an explicit report before we
@@ -1937,7 +1948,6 @@ int utrace_get_signal(struct task_struct
* interrupt path, so clear the flags asking for those.
*/
utrace->interrupt = utrace->report =
utrace->signal_handler = 0;
- utrace->stopped = 0;
/*
* Make sure signal_pending() only returns true
@@ -1966,22 +1976,13 @@ int utrace_get_signal(struct task_struct
event = 0;
ka = NULL;
memset(return_ka, 0, sizeof *return_ka);
- } else if ((task->utrace_flags & UTRACE_EVENT_SIGNAL_ALL) == 0
&&
- !utrace->stopped) {
+ } else if ((task->utrace_flags & UTRACE_EVENT_SIGNAL_ALL) == 0)
{
/*
* If no engine is interested in intercepting signals,
* let the caller just dequeue them normally.
*/
return 0;
} else {
- if (unlikely(utrace->stopped)) {
- spin_unlock_irq(&task->sighand->siglock);
- spin_lock(&utrace->lock);
- utrace->stopped = 0;
- spin_unlock(&utrace->lock);
- spin_lock_irq(&task->sighand->siglock);
- }
-
/*
* Steal the next signal so we can let tracing engines
* examine it. From the signal number and sigaction,
Or do you think it is better to add tracehook_finish_stop() helper which is
called by do_signal_stop() to clear ->stopped ?
As you pointed out we can remove ->stopped, but I am not sure we should do
this now.
BTW, can't finish_utrace_stop() check utrace->stopped lockless?
Oleg.
