| Message ID | 20201102155001.GA18313@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | ptrace: fix ptrace_unfreeze_traced() race with rt-lock | expand |
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.
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
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.
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
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.
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
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
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.
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
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); }
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(-)