[PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[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 <lgoncalv@redhat.com>
Fixes: 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/ptrace.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 3075006d720e..7e004f998233 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -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);
 }
 
-- 
2.25.1.362.g51ebf55

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Oleg Nesterov]

On 11/02, Oleg Nesterov wrote:
>
>  	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.

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Sebastian Andrzej Siewior]

On 2020-11-02 17:25:14 [+0100], Oleg Nesterov wrote:
> On 11/02, Oleg Nesterov wrote:
> >
> >  	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?

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…

> Oleg.

Sebastian

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Oleg Nesterov]

On 11/02, Sebastian Andrzej Siewior wrote:
>
> On 2020-11-02 17:25:14 [+0100], Oleg Nesterov wrote:
> > On 11/02, Oleg Nesterov wrote:
> > >
> > >  	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?
>
> 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.

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Sebastian Andrzej Siewior]

On 2020-11-02 18:01:33 [+0100], Oleg Nesterov wrote:

> 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.

> Oleg.

Sebastian

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Oleg Nesterov]

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

On 11/02, Sebastian Andrzej Siewior wrote:
> On 2020-11-02 18:01:33 [+0100], Oleg Nesterov wrote:
>
> > 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.

Why?

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

Oleg.

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Sebastian Andrzej Siewior]

On 2020-11-02 16:50:01 [+0100], Oleg Nesterov wrote:
> 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.

> Reported-by: Luis Claudio R. Goncalves <lgoncalv@redhat.com>
> Fixes: 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  kernel/ptrace.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 3075006d720e..7e004f998233 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -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?

>  	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);

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.

> +	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);
>  }
>  

Sebastian

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Sebastian Andrzej Siewior]

On 2020-11-02 18:41:31 [+0100], Oleg Nesterov wrote:
> 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.

> On 11/02, Sebastian Andrzej Siewior wrote:
> > On 2020-11-02 18:01:33 [+0100], Oleg Nesterov wrote:
> >
> > > 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.
> 
> 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?

> Oleg.

Sebastian

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Oleg Nesterov]

On 11/02, Sebastian Andrzej Siewior wrote:
>
> On 2020-11-02 16:50:01 [+0100], Oleg Nesterov wrote:
> >
> >  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?

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?

> >  	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);
>
> 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.

> 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.

[PATCH v2] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[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 <lgoncalv@redhat.com>
Fixes: 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/ptrace.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 3075006d720e..3f7156f06b6c 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -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);
 }
 
-- 
2.25.1.362.g51ebf55

Re: [PATCH] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Sebastian Andrzej Siewior]

On 2020-11-03 12:37:49 [+0100], Oleg Nesterov wrote:
> On 11/02, Sebastian Andrzej Siewior wrote:
> >
> > On 2020-11-02 16:50:01 [+0100], Oleg Nesterov wrote:
> > >
> > >  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?
> 
> 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.

> OK, I'll send V2 in a minute.
> 
> > 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.

> Oleg.

Sebastian

Re: [PATCH v2] ptrace: fix ptrace_unfreeze_traced() race with rt-lock

[Sebastian Andrzej Siewior]

On 2020-11-03 12:39:01 [+0100], Oleg Nesterov wrote:
> 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 <lgoncalv@redhat.com>
> Fixes: 6362e6476cd7 ("ptrace: fix ptrace vs tasklist_lock race")
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Applied, thank you.

Sebastian