[PATCH 0/4] utrace->resume fixes

[Roland McGrath]

> 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