ptrace: fix ptrace_unfreeze_traced() race with rt-lock

PATCH Superseded Oleg Nesterov 2 participants 12 messages Nov 2, 2020 – Nov 3, 2020 ptrace support

Share & tools

Oleg Nesterov

author maintainer

2020-11-02 15:50 UTC #0 lore raw

PATCH 1/1 ptrace: fix ptrace_unfreeze_traced() race with rt-lock

The commit 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race") changed ptrace_freeze_traced() to take task->saved_state into account, but ptrace_unfreeze_traced() has the same problem and needs a similar
fix: it should check/update both ->state and ->saved_state.

Reported-by Luis Claudio R. Goncalves <[email protected]>

Fixes 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/ptrace.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

kernel/ptrace.c +14 -8

index 3075006d720e..7e004f998233 100644
@@ -197,8 +197,7 @@ static bool ptrace_freeze_traced(struct task_struct *task)
 
 static void ptrace_unfreeze_traced(struct task_struct *task)
 {
-	if (task->state != __TASK_TRACED)
-		return;
+	bool frozen = true;
 
 	WARN_ON(!task->ptrace || task->parent != current);

@@ -207,12 +206,19 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
 	 * Recheck state under the lock to close this race.
 	 */
 	spin_lock_irq(&task->sighand->siglock);
-	if (task->state == __TASK_TRACED) {
-		if (__fatal_signal_pending(task))
-			wake_up_state(task, __TASK_TRACED);
-		else
-			task->state = TASK_TRACED;
-	}
+
+	raw_spin_lock(&task->pi_lock);
+	if (task->state == __TASK_TRACED)
+		task->state = TASK_TRACED;
+	else if (task->saved_state == __TASK_TRACED)
+		task->saved_state = TASK_TRACED;
+	else
+		frozen = false;
+	raw_spin_unlock(&task->pi_lock);
+
+	if (frozen && __fatal_signal_pending(task))
+		wake_up_state(task, __TASK_TRACED);
+
 	spin_unlock_irq(&task->sighand->siglock);
 }

Signature · 1 line

2.25.1.362.g51ebf55

Oleg Nesterov

author maintainer

2020-11-02 16:25 UTC #1 lore raw

kernel/ptrace.c static void ptrace_unfreeze_traced(struct task_struct *task) · line 210

Quoted 10 lines · Oleg Nesterov

kernel/ptrace.c +2 -6

 	spin_lock_irq(&task->sighand->siglock);
-	if (task->state == __TASK_TRACED) {
-		if (__fatal_signal_pending(task))
-			wake_up_state(task, __TASK_TRACED);
-		else
-			task->state = TASK_TRACED;
-	}
+
+	raw_spin_lock(&task->pi_lock);

but perhaps it should be raw_spin_lock_irq() ?

I know nothing about kernel-rt, however it seems that spin_lock_irq() from include/linux/spinlock_rt.h doesn't disable irqs?

Oleg.

Sebastian Andrzej Siewior

2020-11-02 16:34 UTC #2 lore raw

Quoted 14 lines · Oleg Nesterov

kernel/ptrace.c static void ptrace_unfreeze_traced(struct task_struct *task) · line 210
Quoted 10 lines · Oleg Nesterov
kernel/ptrace.c +2 -6
 	spin_lock_irq(&task->sighand->siglock);
-	if (task->state == __TASK_TRACED) {
-		if (__fatal_signal_pending(task))
-			wake_up_state(task, __TASK_TRACED);
-		else
-			task->state = TASK_TRACED;
-	}
+
+	raw_spin_lock(&task->pi_lock);
but perhaps it should be raw_spin_lock_irq() ?

Quoted 2 lines

I know nothing about kernel-rt, however it seems that spin_lock_irq() from include/linux/spinlock_rt.h doesn't disable irqs?

I don't exactly how much breaks when we turn siglock into a raw_spinlock_t but there is a memory allocation in __send_signal() which is a no no.

There is task_is_traced() which looks under the PI lock for the task state to be sure (we a few of those). I haven't looked at the patch yet…

Quoted 1 line

Oleg.

Sebastian

Oleg Nesterov

author maintainer

2020-11-02 17:01 UTC #3 lore raw

Quoted 19 lines · Sebastian Andrzej Siewior

Quoted 14 lines · Oleg Nesterov
kernel/ptrace.c static void ptrace_unfreeze_traced(struct task_struct *task) · line 210
Quoted 10 lines · Oleg Nesterov
kernel/ptrace.c +2 -6
 	spin_lock_irq(&task->sighand->siglock);
-	if (task->state == __TASK_TRACED) {
-		if (__fatal_signal_pending(task))
-			wake_up_state(task, __TASK_TRACED);
-		else
-			task->state = TASK_TRACED;
-	}
+
+	raw_spin_lock(&task->pi_lock);
but perhaps it should be raw_spin_lock_irq() ?
Quoted 2 lines
I know nothing about kernel-rt, however it seems that spin_lock_irq() from include/linux/spinlock_rt.h doesn't disable irqs?

Quoted 6 lines

I don't exactly how much breaks when we turn siglock into a raw_spinlock_t but there is a memory allocation in __send_signal() which is a no no.
There is task_is_traced() which looks under the PI lock for the task state to be sure (we a few of those). I haven't looked at the patch yet…

So it seems I should send V2 which uses raw_spin_(un)lock_irq().

Or even _irqsave() like ptrace_freeze_traced() does? Although this looks confusing, exactly because ptrace_freeze_traced() calls task_is_traced() which does raw_spin_lock_irq().

Oleg.

Sebastian Andrzej Siewior

2020-11-02 17:31 UTC #4 lore raw

Quoted 5 lines · Oleg Nesterov

So it seems I should send V2 which uses raw_spin_(un)lock_irq().
Or even _irqsave() like ptrace_freeze_traced() does? Although this looks confusing, exactly because ptrace_freeze_traced() calls task_is_traced() which does raw_spin_lock_irq().

Urgh. Judging from
release_task()
-> write_lock_irq(&tasklist_lock);

 -> ptrace_release_task();
    -> ptrace_unlink();
       -> __ptrace_unlink();
         -> task_is_traced().

it will break on !RT so irqsave is indeed needed. And yes, using task_is_traced() and then acquired the PI lock again looks like too much.

Either complain or send a patch, I will look tomorrow.

Quoted 1 line

Oleg.

Sebastian

Oleg Nesterov

author maintainer

2020-11-02 17:41 UTC #5 lore raw

Sorry, I don't understand. If it was not clear, let me repeat that I know nothing about kernel-rt.

Quoted 11 lines · Sebastian Andrzej Siewior

Quoted 5 lines · Oleg Nesterov
So it seems I should send V2 which uses raw_spin_(un)lock_irq().
Or even _irqsave() like ptrace_freeze_traced() does? Although this looks confusing, exactly because ptrace_freeze_traced() calls task_is_traced() which does raw_spin_lock_irq().
Urgh. Judging from
release_task()
-> write_lock_irq(&tasklist_lock);

Quoted 6 lines

 -> ptrace_release_task();
    -> ptrace_unlink();
       -> __ptrace_unlink();
         -> task_is_traced().
it will break on !RT so irqsave is indeed needed.

Why?

task_is_traced() does raw_spin_lock_irq() under ifdef(CONFIG_PREEMPT_RT).

Oleg.

Sebastian Andrzej Siewior

2020-11-02 18:04 UTC #6 lore raw

Quoted 2 lines · Oleg Nesterov

Sorry, I don't understand. If it was not clear, let me repeat that I know nothing about kernel-rt.

That is why I said, if you don't want to continue just complain and leave it to me. Also, I could explain things if you want me to.

Quoted 17 lines · Sebastian Andrzej Siewior

Quoted 16 lines · Oleg Nesterov
Quoted 6 lines
So it seems I should send V2 which uses raw_spin_(un)lock_irq().
Or even _irqsave() like ptrace_freeze_traced() does? Although this looks confusing, exactly because ptrace_freeze_traced() calls task_is_traced() which does raw_spin_lock_irq().
Urgh. Judging from
release_task()
-> write_lock_irq(&tasklist_lock);
 -> ptrace_release_task();
    -> ptrace_unlink();
       -> __ptrace_unlink();
         -> task_is_traced().

Quoted 5 lines

Quoted 1 line
it will break on !RT so irqsave is indeed needed.
Why?
task_is_traced() does raw_spin_lock_irq() under ifdef(CONFIG_PREEMPT_RT).

That is correct. So it is not broken after all.
So basically ptrace_freeze_traced() is okay but ptrace_unfreeze_traced() needs the same treatment.

task_is_traced() acquires/drops the PI-lock and the inner of ptrace_freeze_traced() does the same but needs to be done only for RT.

How much of that do find acceptable with your upstream hat on?

Quoted 1 line

Oleg.

Sebastian

Sebastian Andrzej Siewior

2020-11-02 17:42 UTC #7 lore raw

In reply to Oleg Nesterov

Quoted 4 lines · Oleg Nesterov

The commit 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race") changed ptrace_freeze_traced() to take task->saved_state into account, but ptrace_unfreeze_traced() has the same problem and needs a similar
fix: it should check/update both ->state and ->saved_state.

Thank you for the report.

Quoted 12 lines

Reported-by Luis Claudio R. Goncalves <[email protected]>
Fixes 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")
Signed-off-by Oleg Nesterov <[email protected]>
---
 kernel/ptrace.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)
kernel/ptrace.c +0 -0
index 3075006d720e..7e004f998233 100644

kernel/ptrace.c static bool ptrace_freeze_traced(struct task_struct *task) · line 200

Quoted 6 lines

kernel/ptrace.c +1 -2

@@ -197,8 +197,7 @@ static bool ptrace_freeze_traced(struct task_struct *task)
 
 static void ptrace_unfreeze_traced(struct task_struct *task)
 {
-	if (task->state != __TASK_TRACED)
-		return;
+	bool frozen = true;

Is this okay for !PREEMPT_RT or is this considered as an important fast-path on !PREEMPT_RT?

Quoted 4 lines

kernel/ptrace.c +0 -0

 	WARN_ON(!task->ptrace || task->parent != current);

@@ -207,12 +206,19 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
 	 * Recheck state under the lock to close this race.

kernel/ptrace.c static void ptrace_unfreeze_traced(struct task_struct *task) · line 210

Quoted 10 lines

kernel/ptrace.c +2 -6

 	 */
 	spin_lock_irq(&task->sighand->siglock);
-	if (task->state == __TASK_TRACED) {
-		if (__fatal_signal_pending(task))
-			wake_up_state(task, __TASK_TRACED);
-		else
-			task->state = TASK_TRACED;
-	}
+
+	raw_spin_lock(&task->pi_lock);

I think this got mentioned in the other emails but this requires irqsave because the above (siglock) does not disable interrupts on RT. This `saved_state' is only used for RT so it might make sense to avoid the PI-lock on non-RT.

Quoted 4 lines

kernel/ptrace.c +4 -0

+	if (task->state == __TASK_TRACED)
+		task->state = TASK_TRACED;
+	else if (task->saved_state == __TASK_TRACED)
+		task->saved_state = TASK_TRACED;

kernel/ptrace.c static void ptrace_unfreeze_traced(struct task_struct *task) · line 224

Quoted 10 lines

kernel/ptrace.c +7 -0

+	else
+		frozen = false;
+	raw_spin_unlock(&task->pi_lock);
+
+	if (frozen && __fatal_signal_pending(task))
+		wake_up_state(task, __TASK_TRACED);
+
 	spin_unlock_irq(&task->sighand->siglock);
 }

Sebastian

Oleg Nesterov

author maintainer

2020-11-03 11:38 UTC #8 lore raw

Quoted 11 lines · Sebastian Andrzej Siewior

kernel/ptrace.c static bool ptrace_freeze_traced(struct task_struct *task) · line 200
Quoted 6 lines · Oleg Nesterov
static void ptrace_unfreeze_traced(struct task_struct *task) {
kernel/ptrace.c +1 -2
-	if (task->state != __TASK_TRACED)
-		return;
+	bool frozen = true;
Is this okay for !PREEMPT_RT or is this considered as an important fast-path on !PREEMPT_RT?

I do not know how can ptrace_unfreeze_traced() check both ->state and ->saved_state lockless.

If it is possible then task_is_traced() can avoid pi_lock too?

Quoted 17 lines

Quoted 4 lines
kernel/ptrace.c +0 -0
 	WARN_ON(!task->ptrace || task->parent != current);
@@ -207,12 +206,19 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
 	 * Recheck state under the lock to close this race.
kernel/ptrace.c static void ptrace_unfreeze_traced(struct task_struct *task) · line 210
Quoted 10 lines
kernel/ptrace.c +2 -6
 	 */
 	spin_lock_irq(&task->sighand->siglock);
-	if (task->state == __TASK_TRACED) {
-		if (__fatal_signal_pending(task))
-			wake_up_state(task, __TASK_TRACED);
-		else
-			task->state = TASK_TRACED;
-	}
+
+	raw_spin_lock(&task->pi_lock);
I think this got mentioned in the other emails but this requires irqsave because the above (siglock) does not disable interrupts on RT.

OK, I'll send V2 in a minute.

Quoted 2 lines

This `saved_state' is only used for RT so it might make sense to avoid the PI-lock on non-RT.

The same is true for ptrace_freeze_traced(), we can probably add ifdef's but I am not sure this makes sense...

Oleg.

Sebastian Andrzej Siewior

2020-11-03 16:45 UTC #9 lore raw

Quoted 13 lines · Oleg Nesterov

Quoted 11 lines · Sebastian Andrzej Siewior
kernel/ptrace.c static bool ptrace_freeze_traced(struct task_struct *task) · line 200
Quoted 6 lines · Oleg Nesterov
static void ptrace_unfreeze_traced(struct task_struct *task) {
kernel/ptrace.c +1 -2
-	if (task->state != __TASK_TRACED)
-		return;
+	bool frozen = true;
Is this okay for !PREEMPT_RT or is this considered as an important fast-path on !PREEMPT_RT?

Quoted 4 lines

I do not know how can ptrace_unfreeze_traced() check both ->state and ->saved_state lockless.
If it is possible then task_is_traced() can avoid pi_lock too?

Now we can't. That lock is the only thing that guarantees consistent state.

Quoted 7 lines

OK, I'll send V2 in a minute.
Quoted 2 lines
This `saved_state' is only used for RT so it might make sense to avoid the PI-lock on non-RT.
The same is true for ptrace_freeze_traced(), we can probably add ifdef's but I am not sure this makes sense...

Okay. Then we keep it as it for now.

Quoted 1 line

Oleg.

Sebastian

Oleg Nesterov

author maintainer

2020-11-03 11:39 UTC #10 lore raw

PATCH v2 1/1 ptrace: fix ptrace_unfreeze_traced() race with rt-lock

In reply to Oleg Nesterov

Reported-by Luis Claudio R. Goncalves <[email protected]>

Fixes 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/ptrace.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

kernel/ptrace.c +15 -8

index 3075006d720e..3f7156f06b6c 100644
@@ -197,8 +197,8 @@ static bool ptrace_freeze_traced(struct task_struct *task)
 
 static void ptrace_unfreeze_traced(struct task_struct *task)
 {
-	if (task->state != __TASK_TRACED)
-		return;
+	unsigned long flags;
+	bool frozen = true;
 
 	WARN_ON(!task->ptrace || task->parent != current);

@@ -207,12 +207,19 @@ static void ptrace_unfreeze_traced(struct task_struct *task)
 	 * Recheck state under the lock to close this race.
 	 */
 	spin_lock_irq(&task->sighand->siglock);
-	if (task->state == __TASK_TRACED) {
-		if (__fatal_signal_pending(task))
-			wake_up_state(task, __TASK_TRACED);
-		else
-			task->state = TASK_TRACED;
-	}
+
+	raw_spin_lock_irqsave(&task->pi_lock, flags);
+	if (task->state == __TASK_TRACED)
+		task->state = TASK_TRACED;
+	else if (task->saved_state == __TASK_TRACED)
+		task->saved_state = TASK_TRACED;
+	else
+		frozen = false;
+	raw_spin_unlock_irqrestore(&task->pi_lock, flags);
+
+	if (frozen && __fatal_signal_pending(task))
+		wake_up_state(task, __TASK_TRACED);
+
 	spin_unlock_irq(&task->sighand->siglock);
 }

Signature · 1 line

2.25.1.362.g51ebf55

Sebastian Andrzej Siewior

Applied

2020-11-03 17:01 UTC #11 lore raw

Quoted 8 lines · Oleg Nesterov

The commit 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race") changed ptrace_freeze_traced() to take task->saved_state into account, but ptrace_unfreeze_traced() has the same problem and needs a similar
fix: it should check/update both ->state and ->saved_state.
Reported-by Luis Claudio R. Goncalves <[email protected]>
Fixes 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")
Signed-off-by Oleg Nesterov <[email protected]>

Applied, thank you.

Sebastian