PTRACE_EVENT_SIGTRAP

[Oleg Nesterov]

On 10/27, Roland McGrath wrote:
>
> This is used only in the UTRACE_SIGNAL_HANDLER case.  That means after
> tracehook_signal_handler(), which is where a signal handler has just
> been set up.
>
> For reference, in old ptrace, tracehook_signal_handler() is:
>
> 	if (stepping)
> 		ptrace_notify(SIGTRAP);
>
> This means that when PTRACE_SINGLE{STEP,BLOCK},signr was used to
> deliver the handled signal, we stop there in ptrace_notify.  This is
> in lieu of a proper single-step trap, with the theory that what you
> meant to "step to" next was the first instruction of the signal
> handler, rather than the second instruction of the handler.
>
> The only purpose of PTRACE_EVENT_SIGTRAP is to distinguish this case
> from PTRACE_EVENT_SIGNAL as it would be for a real SIGTRAP after a
> normal instruction step.  The only purposes of this distinction are to
> make PTRACE_SETSIGINFO have no effect, and to make resumptions ignore
> the data/signr argument.  The former makes it consistent with other
> cases in utrace-ptrace that replace ptrace_notify() calls in the old
> ptrace, but that itself is a known nit incompatibility with the old
> behavior, so it's superfluous at best.  The latter is just intended
> for pure bug-compatibility with the old behavior.  Aside from these,
> there are no other reasons not to have the UTRACE_SIGNAL_HANDLER case
> result in PTRACE_EVENT_SIGNAL, with siginfo_t filled in to match what
> the old ptrace_notify() does.
>
> Is that all correct?
>
> In short, PTRACE_EVENT_SIGTRAP provides bug compatibility for
> PTRACE_SINGLESTEP,signr silently acting like PTRACE_SINGLESTEP,0 when

(s/PTRACE_SINGLESTEP/PTRACE_ANY/)

> called at the stop just provoked by the last PTRACE_SINGLESTEP,signr.
> Is that it?

Yes.

> I think this is a piece of bug compatibility that upstream will be
> happy to have "broken".  That is, I bet most people don't realize at
> all PTRACE_SINGLESTEP,signr will sometimes swallow a signal after what
> appears to be a normal single-step stop.  In fact, I suspect many
> might say that this is worse than the previous behavior my addition of
> this ptrace_notify() call (sometime before git so <2.6.12; Sep 2004,
> not sure what version that was, maybe 2.6.9).  That behavior was that
> PTRACE_SINGLESTEP,signr would stop at the second instruction of the
> signal handler and never the first--but that would be a real step
> (like you now get after PTRACE_SINGLESTEP,0 when stopped in
> ptrace_notify() at the first instruction of the handler).  I'm quite
> sure that at the time I never contemplated the signal-swallowing
> implication of using ptrace_notify() here.
>
> So, get rid of PTRACE_EVENT_SIGTRAP.  Instead for the case of
> UTRACE_SIGNAL_HANDLER when stepping, initialize *info to look
> like a vanilla SIGTRAP and then fall into the default case for
> real signals.

Yes, I already thought about this. The patch is trivial, please see
below.

> Does that all sound right to you, or did I miss something?

Don't ask me, I never know what can be changed ;)

But yes, I agree. The current behaviour looks strange (if not wrong).
With this change ptrace(PTRACE_WHATEVER, signr) after PTRACE_SINGLESTEP
never ignores signr. Even if perhaps this is not really useful, this is
consistent. Afaics, this case was a single exception when ptrace(signr)
or PTRACE_SETSIGINFO) was ignored after PTRACE_SINGLESTEP.

The only problem with this change (if we do it now), it is always nice
to have a separate changelog to document the behaviour change. But
hopefully nobody cares, I mean, nobody will notice the difference.

Oleg.

--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -73,7 +73,6 @@ struct ptrace_context {
 
 #define PTRACE_EVENT_SYSCALL_ENTRY	(1 << 16)
 #define PTRACE_EVENT_SYSCALL_EXIT	(2 << 16)
-#define PTRACE_EVENT_SIGTRAP		(3 << 16)
 #define PTRACE_EVENT_SIGNAL		(4 << 16)
 /* events visible to user-space */
 #define PTRACE_EVENT_MASK		0xFFFF
@@ -495,8 +494,8 @@ static u32 ptrace_report_signal(u32 acti
 			context->siginfo = NULL;
 
 		if (resume != UTRACE_RESUME) {
-			set_stop_code(context, PTRACE_EVENT_SIGTRAP);
-			return UTRACE_STOP | UTRACE_SIGNAL_IGN;
+			info->si_signo = SIGTRAO;
+			break;
 		}
 
 	case UTRACE_SIGNAL_REPORT:
@@ -509,21 +508,21 @@ static u32 ptrace_report_signal(u32 acti
 
 		return resume | resume_signal(task, context->signr,
 						info, return_ka);
-	default:
-		WARN_ON(context->siginfo);
-		context->siginfo = info;
-		/*
-		 * context->siginfo points to the caller's stack.
-		 * Make sure the subsequent UTRACE_SIGNAL_REPORT clears
-		 * ->siginfo before return from get_signal_to_deliver().
-		 */
-		utrace_control(task, engine, UTRACE_INTERRUPT);
+	}
 
-		context->stop_code = (PTRACE_EVENT_SIGNAL << 8) | info->si_signo;
-		context->signr   = info->si_signo;
+	WARN_ON(context->siginfo);
+	context->siginfo = info;
+	/*
+	 * context->siginfo points to the caller's stack.
+	 * Make sure the subsequent UTRACE_SIGNAL_REPORT clears
+	 * ->siginfo before return from get_signal_to_deliver().
+	 */
+	utrace_control(task, engine, UTRACE_INTERRUPT);
 
-		return UTRACE_STOP | UTRACE_SIGNAL_IGN;
-	}
+	context->stop_code = (PTRACE_EVENT_SIGNAL << 8) | info->si_signo;
+	context->signr   = info->si_signo;
+
+	return UTRACE_STOP | UTRACE_SIGNAL_IGN;
 }
 
 static u32 ptrace_report_quiesce(u32 action,