Re: exit_ptrace() ptrace_report_signal() problems

[Roland McGrath]

 The problem is, utrace_control(UTRACE_RESUME) can't prevent the stop if
 the tracee has already returned UTRACE_STOP, but utrace_stop() didn't
 take utrace-lock yet.

So you are saying that utrace_barrier does not meet its documented API.
Right?  It says effect ... has been applied.  But that's not true if a
UTRACE_STOP return value will not be cleared by an immediate subsequent
utrace_control(,,UTRACE_RESUME).

 Basically, ptrace_detach_task(sig = -1) should do:
 
   - if we are going to do utrace_control(UTRACE_DETACH), we
 should first instruct the (running) tracee to not report
 a signal, otherwise that signal will be lost.

Right.

   - if the tracee has already reported a signal, we should
 set -resume = UTRACE_DETACH and resume the tracee, like
 explicit detach does.

Right.

 We can check ctx-siginfo != NULL to detect this case.

Ok.

  Especially if it's as I suspect, that we can do
  that without changing the utrace layer.
 
 No, this problem is orthogonal, or I missed something.
 
 Please look at this message
 
   https://www.redhat.com/archives/utrace-devel/2010-June/msg00075.html

Yes, I'd forgotten about that.  We do need to fix utrace_barrier to match
its documented guarantee, or else we cannot rely on it for ptrace.

 In particualar:
 
   Off hand I think it does matter today insofar as it violates the
   documented guarantees of the utrace_barrier API.  If utrace_barrier
   returns 0 it is said to mean that, Any effect of its return value (such
   as %UTRACE_STOP) has already been applied to @engine.  So e.g. if you
   get a wake-up sent by your callback before it returns UTRACE_STOP, and
   then call utrace_barrier followed by utrace_control(,,UTRACE_RESUME),
   then you should be guaranteed that your resumption cleared the
   callback's stop.
 
 Yes, but currently UTRACE_RESUME can't guarantee this. 

From the API perspective I had been thinking in, it's not utrace_control
that's supposed to guarantee it.  It's utrace_barrier that's not
supposed to return yet.  But, that is indeed a sort of inside-out way of
looking at it, really.  What utrace_barrier guarantees is that the
callback bookkeeping is done, and it's not supposed to wait for e.g. the
next engine's callback to run.

 utrace_control(RESUME) should remove ENGINE_STOP like UTRACE_DETACH does

I think you've now talked me into this.  There is no other way that
utrace_barrier can keep its guarantee about the return value effect
without also delaying while other engines' callbacks might run, which
seems much worse.


Thanks,
Roland

Re: exit_ptrace() ptrace_report_signal() problems

[Oleg Nesterov]

On 09/14, Oleg Nesterov wrote:

 Oh, please take a look at this (untested) patch.

I tried to test the cleanuped version, seems to work.

 But I think I should make another attempt, probably spinlock (-siglock ?)
 would be cleaner.

No. I think more locking buys nothing.

I'll split this patch and resend today.

Oleg.

Re: exit_ptrace() ptrace_report_signal() problems

[Oleg Nesterov]

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.0 +0200
+++ kstub/kernel/ptrace-utrace.c2010-09-14 23:37:59.0 +0200
@@ -67,6 +67,7 @@ struct ptrace_context {
 #define PT_UTRACED 0x1000
 
 #define PTRACE_O_SYSEMU0x100
+#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));