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

[Oleg Nesterov]

On 11/03, Roland McGrath wrote:
>
> > 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))) {

Afaics this fixes nothing.

Yes, this avoids the unnecessary utrace_reset() in utrace_get_signal(),
but sometimes we need utrace_reset() here, now it is never called.

Suppose a tracee is stopped (TASK_TRACED). The tracer does
utrace_control(UTRACE_SINGLESTEP). This sets ->resume = UTRACE_SINGLESTEP
and wakes up the tracee.

Right after that, the tracer does utrace_control(UTRACE_DETACH). This
calls mark_engine_detached(), but utrace_do_stop() do not change ->resume,
it is still UTRACE_SINGLESTEP.

Now, the tracee enters utrace_get_signal(), notices ->resume < UTRACE_RESUME,
sets utrace->resume = UTRACE_RESUME and does

		if (resume > UTRACE_REPORT) {
			/*
			 * We only got here to process utrace->resume.
			 */
			report.action = resume;
			finish_resume_report(&report, task, utrace);
			return -1;
		}

Now that finish_resume_report() does not do finish_report_reset(),
we call user_enable_single_step() and return. Unless we have another
engine which triggers utrace_resume() eventually, UTRACE_DETACH can't
be completed, utrace_detached_ops engine is "lost".

UTRACE_STOP can be lost in the same way, with or without this change.
mark_engine_wants_stop() sets ENGINE_STOP, but since we reset ->resume
it can't be noticed until some engine sets ->resume = REPORT/INTERRUPT.

> > 5. utrace_resume() has the same problems.

Including the possibility to miss UTRACE_STOP or UTRACE_DETACH.

And,

> > 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?

I think no. finish_report_reset() will always call utrace_reset()
because start_callback(event => 0) can never set takers.

If report.action != UTRACE_REPORT, then we don't call utrace_reset().
But, like above, sometime it is needed to complete UTRACE_DETACH.

Oleg.