[PATCH] utrace: utrace_reset() should clear TIF_SINGLESTEP if no more engines

[Oleg Nesterov]

On 09/23, Roland McGrath wrote:
>
> > I think we should start with changing utrace_control(DETACH) anyway,
> > then try to improve. I'll ressend the one-liner I already showed.
>
> Ok.

Yes. I think we need a simple fix first, please see another email
I am going to send.

However, I am a bit confused. I was going to send the patch which
changes utrace_control(DETACH) as you suggested, but now I am not
sure. See below.

> The original theory on this was that we'd one day stop overloading
> user signals for debugger-induced traps.
> things like hardware stepping would generate a special new flavor
> of utrace event
>
> But we don't have any of that now, and don't yet know if we will
> really pursue any big improvements at this API level.

Yes! I thought about this too many times.


So. Let's discuss the alternatives for now.

The 1st patch I sent initially:

	--- kstub/kernel/utrace.c~10_utrace_reset_should_clear_ss	2010-09-20 03:55:12.000000000 +0200
	+++ kstub/kernel/utrace.c	2010-09-20 21:33:47.000000000 +0200
	@@ -709,6 +709,7 @@ static bool utrace_reset(struct task_str
			 */
			utrace->resume = UTRACE_RESUME;
			utrace->signal_handler = 0;
	+		user_disable_single_step(task);
		}
	 
		/*

It clears TIF_SINGLESTEP when the last engine detaches.

The 2nd one which changes utrace_control(DETACH),

	@@ -1139,7 +1139,9 @@ int utrace_control(struct task_struct *t
				target->utrace_flags &= ~ENGINE_STOP;
			mark_engine_detached(engine);
			reset = reset || utrace_do_stop(target, utrace);
	-		if (!reset) {
	+		if (reset) {
	+			user_disable_single_step(target);
	+		} else {
				/*
				 * As in utrace_set_events(), this barrier ensures
				 * that our engine->flags changes have hit before we

It doesn't cover the case when the TIF_SINGLESTEP tracee detaches
itself, but a) currently the are no such engines, and b) it looks
more clean.

However, I am starting to think that if you prefer this change we
have a better option. Instead of duplicating UTRACE_RESUME logic,
perhaps the patch below makes more sense?

Please tell me which fix you prefer? Or just commit the fix you
like more.

Oleg.

--- x/kernel/utrace.c
+++ x/kernel/utrace.c
@@ -716,8 +716,15 @@ static bool utrace_reset(struct task_str
 	/*
 	 * If no more engines want it stopped, wake it up.
 	 */
-	if (task_is_traced(task) && !(flags & ENGINE_STOP))
+	if (task_is_traced(task) && !(flags & ENGINE_STOP)) {
+		/*
+		 * It just resumes, so make sure single-step
+		 * is not left set.
+		 */
+		if (utrace->resume == UTRACE_RESUME)
+			user_disable_single_step(task);
 		utrace_wakeup(task, utrace);
+	}
 
 	/*
 	 * In theory spin_lock() doesn't imply rcu_read_lock().
@@ -1157,14 +1164,7 @@ int utrace_control(struct task_struct *t
 		break;
 
 	case UTRACE_RESUME:
-		/*
-		 * This and all other cases imply resuming if stopped.
-		 * There might not be another report before it just
-		 * resumes, so make sure single-step is not left set.
-		 */
 		clear_engine_wants_stop(engine);
-		if (likely(reset))
-			user_disable_single_step(target);
 		break;
 
 	case UTRACE_BLOCKSTEP: