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,
