Somehow I can't really understand this patch. I hope more or less
I can see what it does, but the resulting code looks even more
subtle to me.
Well, it was an untested draft and probably needed more comments.
With this patch, apply_resume_action() is always called after
utrace_stop(). Well, except for utrace_report_exit(), but I guess
it could do apply_resume_action() too.
It could, but it just never matters at all. The only thing that can
possibly be meaningful is UTRACE_INTERRUPT, and for that finish_report has
set TIF_SIGPENDING already anyway.
Now it is not clear why utrace_control(SINGLESTEP) sets
TIF_NOTIFY_RESUME, it is not needed after apply_resume_action()
processes -resume. Yes, we have jctl stops, but we can move
this code into utrace_finish_stop().
Yes, it is not really needed. But note that it is actually now possible to
use UTRACE_SINGLESTEP without UTRACE_STOP first--not that it's really
useful, but we don't really have any reason to disallow it. So in that
case it does make a difference. We could just do:
@@ -1183,7 +1183,8 @@ int utrace_control(struct task_struct *target,
clear_engine_wants_stop(engine);
if (action utrace-resume) {
utrace-resume = action;
- set_notify_resume(target);
+ if (reset)
+ set_notify_resume(target);
}
break;
Since TIF_NOTIFY_RESUME will now always be superfluous when stopped.
It is not clear to me why apply_resume_action(UTRACE_INTERRUPT) does
set_tsk_thread_flag(TIF_SIGPENDING). In fact I don't understand why
apply_resume_action() checks UTRACE_INTERRUPT at all. If it is called
after utrace_stop(), action == start_report() which never resets
UTRACE_INTERRUPT.
It's only because this code path is shared with the tail of
finish_resume_report, where it is the only thing processing
UTRACE_INTERRUPT in the real return-to-user cases. i.e., in paths that
don't call finish_report(), TIF_SIGPENDING won't ever have been set by a
callback return value. utrace_control does always set it up front, so it
is superfluous when that's how UTRACE_INTERRUPT got set.
And since we process -resume after stop, it is not clear why we
set -resume = report-resume_action before stop, apply_resume_action()
could use min(report-resume_action, utrace-resume). Yes, yes,
we have the nasty utrace-vfork_stop case, I mean it is not easy to
understand the logic behind all these -resume changes.
Ok. Feel free to clean it up if you think it makes things clearer. To me,
it's just natural to do that MIN operation as soon as you know about it.
utrace_stop() always takes the lock anyway, so it's relatively free. If
the ambient state is stored early, then e.g. utrace_control() won't bother
to set TIF_SIGPENDING or TIF_NOTIFY_RESUME.
I guess I think the logic is simple: apply a new minimum to -resume as
soon as you know about it.
And afaics, this can't help to fix the tracehook_report_syscall_exit()
TIF_SINGLESTEP problems in the multitracing case, UTRACE_INTERRUPT
destroys UTRACE_SINGLESTEP. Ptrace can reassert it later, but this
will be too late, the trace has already passed this tracehook.
See the other thread, but what I said there is let's take this case up in
its own thread later.
I don't understand this skip_notify, utrace_stop() is always called
with skip_notify == true?
Hmm. Yes, this patch was unfinished when I sent it because your patches
were crossing paths with what I was doing. We can skip TIF_NOTIFY_RESUME
when we'll always call apply_resume_action() after utrace_stop() before
user mode (i.e. report_exit doesn't matter).
Since report_exit doesn't matter one way or the other, perhaps it will be
cleaner to roll apply_resume_action() into utrace_stop() in some fashion.
I notice we're now actually using the REPORT() macro only four times, and
one of those is report_exit. So maybe that and finish_report() could
change somehow too to refactor things. I'll leave it to you to rework all
those and finish_* to the most useful organization of subroutines.
I think we're mutually clear now on the idea of when to do user_*_step()
calls (i.e. apply_resume_action).
Thanks,
Roland
