Re: [PATCH] ptrace: fix PTRACE_LISTEN race corrupting task->state

[Oleg Nesterov]

(add akpm, we usually route ptrace fixes via -mm tree)

On 02/21, bsegall@xxxxxxxxxx wrote:
>
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -184,10 +184,14 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
>  
>         WARN_ON(!task->ptrace || task->parent != current);
>  
> +       /*
> +        * Double check __TASK_TRACED under the lock to prevent corrupting state
> +        * in case of a ptrace_trap_notify wakeup
> +        */
>         spin_lock_irq(&task->sighand->siglock);
>         if (__fatal_signal_pending(task))
>                 wake_up_state(task, __TASK_TRACED);
> -       else
> +       else if (task->state == __TASK_TRACED)
>                 task->state = TASK_TRACED;
>         spin_unlock_irq(&task->sighand->siglock);

So yes, I think your patch is fine except the comment should explain that
we need this because PTRACE_LISTEN makes ptrace_trap_notify() possible. And
perhaps it would be better to do the 2nd check before fatal_signal_pending:

	if (task->state == __TASK_TRACED) {
		if (__fatal_signal_pending(task))
			wake_up_state(task, __TASK_TRACED);
		else
			task->state = TASK_TRACED;
	}

just to make the logic more clear. wake_up_state(__TASK_TRACED) can
never hurt if the task is killed, just it doesn't look strictly correct
if the tracee was already woken. But this is minor.



You know, I'd prefer another fix, see below.

Why. ptrace_unfreeze_traced() assumes that - since ptrace_freeze_traced()
checks PTRACE_LISTEN - nobody but us can wake the tracee up. So the
__TASK_TRACED check at the start of ptrace_unfreeze_traced() means that
the tracee is still freezed, it was not woken up by (say) PTRACE_CONT.

IOW, currently we assume that only the caller of ptrace_freeze_traced()
can do the __TASK_TRACED -> WHATEVER transition.

However, as you pointed out, I forgot that JOBCTL_LISTENING set by LISTEN
breaks this assumption, and imo it would be nice to fix this.

What do you think? I won't insist too much if you prefer your simple change.

Oleg.

--- x/kernel/ptrace.c
+++ x/kernel/ptrace.c
@@ -174,6 +174,18 @@
 	return ret;
 }
 
+static bool __ptrace_unfreeze_traced(struct task_struct *task)
+{
+	bool killed = __fatal_signal_pending(task);
+
+	if (killed)
+		wake_up_state(task, __TASK_TRACED);
+	else
+		task->state = TASK_TRACED;
+
+	return !killed'
+}
+
 static void ptrace_unfreeze_traced(struct task_struct *task)
 {
 	if (task->state != __TASK_TRACED)
@@ -182,10 +194,7 @@
 	WARN_ON(!task->ptrace || task->parent != current);
 
 	spin_lock_irq(&task->sighand->siglock);
-	if (__fatal_signal_pending(task))
-		wake_up_state(task, __TASK_TRACED);
-	else
-		task->state = TASK_TRACED;
+	__ptrace_unfreeze_traced(task);
 	spin_unlock_irq(&task->sighand->siglock);
 }
 
@@ -993,7 +1002,12 @@
 			break;
 
 		si = child->last_siginfo;
-		if (likely(si && (si->si_code >> 8) == PTRACE_EVENT_STOP)) {
+		/*
+		 * Once we set JOBCTL_LISTENING we do not own child->state,
+		 * need to unfreeze first.
+		 */
+		if (__ptrace_unfreeze_traced(child) &&
+		    likely(si && (si->si_code >> 8) == PTRACE_EVENT_STOP)) {
 			child->jobctl |= JOBCTL_LISTENING;
 			/*
 			 * If NOTIFY is set, it means event happened between