[PATCH 0/1] Was: utrace-cleanup branch

[Roland McGrath]

> 4. One of the changes in utrace_get_signal() doesn't look exactly right.
> 
> 	if (utrace->resume < UTRACE_RESUME || utrace->signal_handler) {
> 		...
> 		if (resume > UTRACE_REPORT) {
> 			report.action = resume;
> 			finish_resume_report(&report);
> 		}
> 
>    Yes, we need finish_resume_report() here, but since report->takers
>    is not set, finish_report_reset() will always call utrace_reset().
> 
>    OTOH, we can't set ->takers before finish_resume_report(), we can
>    miss UTRACE_DETACH request. utrace_control(DETACH)->utrace_do_stop()
>    does not change utrace->resume != UTRACE_RESUME.
> 
>    And since utrace_do_stop() never "upgrades" utrace->resume, we have
>    another problem: UTRACE_STOP request can be lost here.

Hmm.  Maybe this?

diff --git a/kernel/utrace.c b/kernel/utrace.c
index f46854b..0000000 100644  
--- a/kernel/utrace.c
+++ b/kernel/utrace.c
@@ -1740,8 +1740,6 @@ static void finish_resume_report(struct 
 				 struct task_struct *task,
 				 struct utrace *utrace)
 {
-	finish_report_reset(task, utrace, report);
-
 	switch (report->action) {
 	case UTRACE_STOP:
 		utrace_stop(task, utrace, report->resume_action);
@@ -1829,6 +1827,8 @@ void utrace_resume(struct task_struct *t
 		report.action = UTRACE_RESUME;
 		list_for_each_entry(engine, &utrace->attached, entry)
 			start_callback(utrace, &report, engine, task, 0);
+
+		finish_report_reset(task, utrace, report);
 	}
 
 	/*
@@ -2147,6 +2147,7 @@ int utrace_get_signal(struct task_struct
 		 * as in utrace_resume(), above.  After we've dealt with that,
 		 * our caller will relock and come back through here.
 		 */
+		finish_report_reset(task, utrace, &report);
 		finish_resume_report(&report, task, utrace);
 
 		if (unlikely(fatal_signal_pending(task))) {


> 5. utrace_resume() has the same problems. If report->action != REPORT
>    we do not call callbacks and finish_resume_report() is called with
>    ->takers == F.
>
> 6. But! I think utrace_resume() was always wrong here. Because it
>    calls start_callback(events => 0) and thus we nevet set ->takers.

I think that change covers these too.  What do you think?


Thanks,
Roland