* [PATCH v2 0/2] ptrace: DEBUGCTLMSR_BTF fixes
@ 2012-08-13 13:35 Oleg Nesterov
2012-08-13 13:35 ` [PATCH v2 1/2] ptrace: introduce set_task_blockstep() helper Oleg Nesterov
2012-08-13 13:35 ` [PATCH v2 2/2] [PATCH] ptrace: partly fix set_task_blockstep()->update_debugctlmsr() logic Oleg Nesterov
0 siblings, 2 replies; 3+ messages in thread
From: Oleg Nesterov @ 2012-08-13 13:35 UTC (permalink / raw)
To: Al Viro, Ingo Molnar, Linus Torvalds
Cc: Ananth N Mavinakayanahalli, Anton Arapov, H. Peter Anvin,
Peter Zijlstra, Roland McGrath, Sebastian Andrzej Siewior,
Srikar Dronamraju, linux-kernel
Hello.
The subject says "ptrace:", but we need these changes for uprobes.
Although I think this fix makes sense by itself. I am really
surprised how many problems this simple code has, or I am
totally confused (this is very possible).
Could someone review 2/2 ?
The next step will change uprobes to use the new helper and avoid
user_enable_single_step().
Oleg.
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v2 1/2] ptrace: introduce set_task_blockstep() helper
2012-08-13 13:35 [PATCH v2 0/2] ptrace: DEBUGCTLMSR_BTF fixes Oleg Nesterov
@ 2012-08-13 13:35 ` Oleg Nesterov
2012-08-13 13:35 ` [PATCH v2 2/2] [PATCH] ptrace: partly fix set_task_blockstep()->update_debugctlmsr() logic Oleg Nesterov
1 sibling, 0 replies; 3+ messages in thread
From: Oleg Nesterov @ 2012-08-13 13:35 UTC (permalink / raw)
To: Al Viro, Ingo Molnar, Linus Torvalds
Cc: Ananth N Mavinakayanahalli, Anton Arapov, H. Peter Anvin,
Peter Zijlstra, Roland McGrath, Sebastian Andrzej Siewior,
Srikar Dronamraju, linux-kernel
No functional changes, preparation for the next fix and for uprobes
single-step fixes.
Move the code playing with TIF_BLOCKSTEP/DEBUGCTLMSR_BTF into the
new helper, set_task_blockstep().
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
arch/x86/kernel/step.c | 41 +++++++++++++++++++++--------------------
1 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index c346d11..7a51498 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -157,6 +157,21 @@ static int enable_single_step(struct task_struct *child)
return 1;
}
+static void set_task_blockstep(struct task_struct *task, bool on)
+{
+ unsigned long debugctl;
+
+ debugctl = get_debugctlmsr();
+ if (on) {
+ debugctl |= DEBUGCTLMSR_BTF;
+ set_tsk_thread_flag(task, TIF_BLOCKSTEP);
+ } else {
+ debugctl &= ~DEBUGCTLMSR_BTF;
+ clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
+ }
+ update_debugctlmsr(debugctl);
+}
+
/*
* Enable single or block step.
*/
@@ -169,19 +184,10 @@ static void enable_step(struct task_struct *child, bool block)
* So no one should try to use debugger block stepping in a program
* that uses user-mode single stepping itself.
*/
- if (enable_single_step(child) && block) {
- unsigned long debugctl = get_debugctlmsr();
-
- debugctl |= DEBUGCTLMSR_BTF;
- update_debugctlmsr(debugctl);
- set_tsk_thread_flag(child, TIF_BLOCKSTEP);
- } else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
- unsigned long debugctl = get_debugctlmsr();
-
- debugctl &= ~DEBUGCTLMSR_BTF;
- update_debugctlmsr(debugctl);
- clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
- }
+ if (enable_single_step(child) && block)
+ set_task_blockstep(child, true);
+ else if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+ set_task_blockstep(child, false);
}
void user_enable_single_step(struct task_struct *child)
@@ -199,13 +205,8 @@ void user_disable_single_step(struct task_struct *child)
/*
* Make sure block stepping (BTF) is disabled.
*/
- if (test_tsk_thread_flag(child, TIF_BLOCKSTEP)) {
- unsigned long debugctl = get_debugctlmsr();
-
- debugctl &= ~DEBUGCTLMSR_BTF;
- update_debugctlmsr(debugctl);
- clear_tsk_thread_flag(child, TIF_BLOCKSTEP);
- }
+ if (test_tsk_thread_flag(child, TIF_BLOCKSTEP))
+ set_task_blockstep(child, false);
/* Always clear TIF_SINGLESTEP... */
clear_tsk_thread_flag(child, TIF_SINGLESTEP);
--
1.5.5.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v2 2/2] [PATCH] ptrace: partly fix set_task_blockstep()->update_debugctlmsr() logic
2012-08-13 13:35 [PATCH v2 0/2] ptrace: DEBUGCTLMSR_BTF fixes Oleg Nesterov
2012-08-13 13:35 ` [PATCH v2 1/2] ptrace: introduce set_task_blockstep() helper Oleg Nesterov
@ 2012-08-13 13:35 ` Oleg Nesterov
1 sibling, 0 replies; 3+ messages in thread
From: Oleg Nesterov @ 2012-08-13 13:35 UTC (permalink / raw)
To: Al Viro, Ingo Molnar, Linus Torvalds
Cc: Ananth N Mavinakayanahalli, Anton Arapov, H. Peter Anvin,
Peter Zijlstra, Roland McGrath, Sebastian Andrzej Siewior,
Srikar Dronamraju, linux-kernel
Afaics the usage of update_debugctlmsr() and TIF_BLOCKSTEP in
step.c was always very wrong.
1. update_debugctlmsr() was simply unneeded. The child sleeps
TASK_TRACED, __switch_to_xtra(next_p => child) should notice
TIF_BLOCKSTEP and set/clear DEBUGCTLMSR_BTF after resume if
needed.
2. It is wrong. The state of DEBUGCTLMSR_BTF bit in CPU register
should always match the state of current's TIF_BLOCKSTEP bit.
3. Even get_debugctlmsr() + update_debugctlmsr() itself does not
look right. Irq can change other bits in MSR_IA32_DEBUGCTLMSR
register or the caller can be preempted in between.
4. It is not safe to play with TIF_BLOCKSTEP if task != current.
DEBUGCTLMSR_BTF and TIF_BLOCKSTEP should always match each
other if the task is running. The tracee is stopped but it
can be SIGKILL'ed right before set/clear_tsk_thread_flag().
However, now that uprobes uses user_enable_single_step(current)
we can't simply remove update_debugctlmsr(). So this patch adds
the additional "task == current" check and disables irqs to avoid
the race with interrupts/preemption.
Unfortunately this patch doesn't solve the last problem, we need
another fix. Probably we should teach ptrace_stop() to set/clear
single/block stepping after resume.
And afaics there is yet another problem: perf can play with
MSR_IA32_DEBUGCTLMSR from nmi, this obviously means that even
__switch_to_xtra() has problems.
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
arch/x86/kernel/step.c | 14 +++++++++++++-
1 files changed, 13 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kernel/step.c b/arch/x86/kernel/step.c
index 7a51498..f89cdc6 100644
--- a/arch/x86/kernel/step.c
+++ b/arch/x86/kernel/step.c
@@ -161,6 +161,16 @@ static void set_task_blockstep(struct task_struct *task, bool on)
{
unsigned long debugctl;
+ /*
+ * Ensure irq/preemption can't change debugctl in between.
+ * Note also that both TIF_BLOCKSTEP and debugctl should
+ * be changed atomically wrt preemption.
+ * FIXME: this means that set/clear TIF_BLOCKSTEP is simply
+ * wrong if task != current, SIGKILL can wakeup the stopped
+ * tracee and set/clear can play with the running task, this
+ * can confuse the next __switch_to_xtra().
+ */
+ local_irq_disable();
debugctl = get_debugctlmsr();
if (on) {
debugctl |= DEBUGCTLMSR_BTF;
@@ -169,7 +179,9 @@ static void set_task_blockstep(struct task_struct *task, bool on)
debugctl &= ~DEBUGCTLMSR_BTF;
clear_tsk_thread_flag(task, TIF_BLOCKSTEP);
}
- update_debugctlmsr(debugctl);
+ if (task == current)
+ update_debugctlmsr(debugctl);
+ local_irq_enable();
}
/*
--
1.5.5.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-08-13 13:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-13 13:35 [PATCH v2 0/2] ptrace: DEBUGCTLMSR_BTF fixes Oleg Nesterov
2012-08-13 13:35 ` [PATCH v2 1/2] ptrace: introduce set_task_blockstep() helper Oleg Nesterov
2012-08-13 13:35 ` [PATCH v2 2/2] [PATCH] ptrace: partly fix set_task_blockstep()->update_debugctlmsr() logic Oleg Nesterov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).