On 09/14, Oleg Nesterov wrote:
>
> On 09/13, Roland McGrath wrote:
> >
> > Locks just between the tracer and ptrace_report_signal are not bad.
>
> OK, but let me think a bit more. I'd really like to avoid adding the
> new lock to avoid the very unlikely race with the exiting tracer.
Oh, please take a look at this (untested) patch.
But I think I should make another attempt, probably spinlock (->siglock ?)
would be cleaner.
Oleg.
--- kstub/kernel/ptrace-utrace.c~9_EXIT_PTRACE_CAN_LOSE_SIG 2010-08-24
14:31:17.000000000 +0200
+++ kstub/kernel/ptrace-utrace.c 2010-09-14 23:37:59.000000000 +0200
@@ -67,6 +67,7 @@ struct ptrace_context {
#define PT_UTRACED 0x00001000
#define PTRACE_O_SYSEMU 0x100
+#define PTRACE_O_REUSABLE 0x200
#define PTRACE_EVENT_SYSCALL (1 << 16)
#define PTRACE_EVENT_SIGTRAP (2 << 16)
@@ -115,7 +116,7 @@ ptrace_reuse_engine(struct task_struct *
return engine;
ctx = ptrace_context(engine);
- if (unlikely(ctx->resume == UTRACE_DETACH)) {
+ if (unlikely(ctx->options == PTRACE_O_REUSABLE)) {
/*
* Try to reuse this self-detaching engine.
* The only caller which can hit this case is ptrace_attach(),
@@ -246,24 +247,48 @@ static void ptrace_detach_task(struct ta
*/
bool voluntary = (sig >= 0);
struct utrace_engine *engine = ptrace_lookup_engine(tracee);
+ struct ptrace_context *ctx = ptrace_context(engine);
enum utrace_resume_action action = UTRACE_DETACH;
if (unlikely(IS_ERR(engine)))
return;
- if (sig) {
- struct ptrace_context *ctx = ptrace_context(engine);
+ if (!voluntary) {
+ int err;
+
+ ctx->resume = UTRACE_DETACH;
+ /* synchronize with ptrace_report_signal() */
+ do {
+ err = utrace_barrier(tracee, engine);
+ } while (err == -ERESTARTSYS);
+
+ if (!err) {
+ /*
+ * The tracee has already reported a signal. If we
+ * see ->siginfo != NULL it is safe to mark this ctx
+ * as reusable and resume the tracee. We can race
+ * with ptrace_report_signal(UTRACE_SIGNAL_REPORT)
+ * already in progress but this doesn't matter, it
+ * must see ->resume = UTRACE_DETACH.
+ */
+ if (ctx->siginfo) {
+ smp_wmb();
+ ctx->options = PTRACE_O_REUSABLE;
+ action = UTRACE_RESUME;
+ }
+ }
+ } else if (sig) {
switch (get_stop_event(ctx)) {
case PTRACE_EVENT_SYSCALL:
- if (voluntary)
- send_sig_info(sig, SEND_SIG_PRIV, tracee);
+ send_sig_info(sig, SEND_SIG_PRIV, tracee);
break;
case PTRACE_EVENT_SIGNAL:
- if (voluntary)
- ctx->signr = sig;
+ ctx->signr = sig;
ctx->resume = UTRACE_DETACH;
+ smp_wmb();
+ ctx->options = PTRACE_O_REUSABLE;
action = UTRACE_RESUME;
break;
}
@@ -410,6 +435,9 @@ static u32 ptrace_report_syscall_exit(u3
return UTRACE_STOP;
if (ctx->resume != UTRACE_RESUME) {
+ if (ctx->resume == UTRACE_DETACH)
+ return UTRACE_RESUME;
+
WARN_ON(ctx->resume != UTRACE_BLOCKSTEP &&
ctx->resume != UTRACE_SINGLESTEP);
ctx->resume = UTRACE_RESUME;
@@ -502,6 +530,9 @@ static u32 ptrace_report_signal(u32 acti
ctx->siginfo = NULL;
if (resume != UTRACE_RESUME) {
+ if (resume == UTRACE_DETACH)
+ return action;
+
WARN_ON(resume != UTRACE_BLOCKSTEP &&
resume != UTRACE_SINGLESTEP);
@@ -531,6 +562,11 @@ static u32 ptrace_report_signal(u32 acti
}
WARN_ON(ctx->siginfo);
+
+ /* Raced with the exiting tracer ? */
+ if (resume == UTRACE_DETACH)
+ return action;
+
ctx->siginfo = info;
/*
* ctx->siginfo points to the caller's stack.
@@ -759,7 +795,7 @@ void exit_ptrace(struct task_struct *tra
static int ptrace_set_options(struct task_struct *tracee,
struct utrace_engine *engine, long data)
{
- BUILD_BUG_ON(PTRACE_O_MASK & PTRACE_O_SYSEMU);
+ BUILD_BUG_ON(PTRACE_O_MASK & (PTRACE_O_SYSEMU | PTRACE_O_REUSABLE));
ptrace_set_events(tracee, engine, data & PTRACE_O_MASK);
return (data & ~PTRACE_O_MASK) ? -EINVAL : 0;
