This patch fixes a race condition where in system used to hang
or sometime crash within minutes when kprobes are inserted on
ISR routine and a task routine.
The fix has been stress tested on i386, ia64, pp64 and on x86_64.
To reproduce the problem insert kprobes on schedule() and do_IRQ() functions and
you should see hang or system crash.
This patch should apply cleanly on 2.6.13-mm1 kernel.
Please apply.
Signed-off-by: Anil S Keshavamurthy <[email protected]>
Signed-off-by: Ananth N Mavinakayanahalli <[email protected]>
Acked-by: Prasanna S Panchamukhi <[email protected]>
=====================================================
arch/i386/kernel/kprobes.c | 3 ++-
arch/ia64/kernel/kprobes.c | 22 +++++++++++++++++++---
arch/ppc64/kernel/kprobes.c | 11 ++++++-----
arch/x86_64/kernel/kprobes.c | 3 ++-
include/asm-ia64/kprobes.h | 1 +
include/asm-ppc64/kprobes.h | 3 +++
kernel/kprobes.c | 8 ++++++++
7 files changed, 41 insertions(+), 10 deletions(-)
Index: linux-2.6.13-mm1/arch/i386/kernel/kprobes.c
===================================================================
--- linux-2.6.13-mm1.orig/arch/i386/kernel/kprobes.c
+++ linux-2.6.13-mm1/arch/i386/kernel/kprobes.c
@@ -190,7 +190,8 @@ static int __kprobes kprobe_handler(stru
Disarm the probe we just hit, and ignore it. */
p = get_kprobe(addr);
if (p) {
- if (kprobe_status == KPROBE_HIT_SS) {
+ if (kprobe_status == KPROBE_HIT_SS &&
+ *p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
regs->eflags &= ~TF_MASK;
regs->eflags |= kprobe_saved_eflags;
unlock_kprobes();
Index: linux-2.6.13-mm1/arch/ia64/kernel/kprobes.c
===================================================================
--- linux-2.6.13-mm1.orig/arch/ia64/kernel/kprobes.c
+++ linux-2.6.13-mm1/arch/ia64/kernel/kprobes.c
@@ -95,6 +95,17 @@ static void __kprobes update_kprobe_inst
p->ainsn.inst_flag = 0;
p->ainsn.target_br_reg = 0;
+ /* Check for Break instruction
+ * Bits 37:40 Major opcode to be zero
+ * Bits 27:32 X6 to be zero
+ * Bits 32:35 X3 to be zero
+ */
+ if ((!major_opcode) && (!((kprobe_inst >> 27) & 0x1FF)) ) {
+ /* is a break instruction */
+ p->ainsn.inst_flag |= INST_FLAG_BREAK_INST;
+ return;
+ }
+
if (bundle_encoding[template][slot] == B) {
switch (major_opcode) {
case INDIRECT_CALL_OPCODE:
@@ -542,8 +553,11 @@ static void __kprobes prepare_ss(struct
unsigned long bundle_addr = (unsigned long) &p->opcode.bundle;
unsigned long slot = (unsigned long)p->addr & 0xf;
- /* Update instruction pointer (IIP) and slot number (IPSR.ri) */
- regs->cr_iip = bundle_addr & ~0xFULL;
+ /* single step inline if break instruction */
+ if (p->ainsn.inst_flag == INST_FLAG_BREAK_INST)
+ regs->cr_iip = (unsigned long)p->addr & ~0xFULL;
+ else
+ regs->cr_iip = bundle_addr & ~0xFULL;
if (slot > 2)
slot = 0;
@@ -599,7 +613,9 @@ static int __kprobes pre_kprobes_handler
if (kprobe_running()) {
p = get_kprobe(addr);
if (p) {
- if (kprobe_status == KPROBE_HIT_SS) {
+ if ( (kprobe_status == KPROBE_HIT_SS) &&
+ (p->ainsn.inst_flag == INST_FLAG_BREAK_INST)) {
+ ia64_psr(regs)->ss = 0;
unlock_kprobes();
goto no_kprobe;
}
Index: linux-2.6.13-mm1/arch/ppc64/kernel/kprobes.c
===================================================================
--- linux-2.6.13-mm1.orig/arch/ppc64/kernel/kprobes.c
+++ linux-2.6.13-mm1/arch/ppc64/kernel/kprobes.c
@@ -102,7 +102,7 @@ static inline void prepare_singlestep(st
regs->msr |= MSR_SE;
/* single step inline if it is a trap variant */
- if (IS_TW(insn) || IS_TD(insn) || IS_TWI(insn) || IS_TDI(insn))
+ if (is_trap(insn))
regs->nip = (unsigned long)p->addr;
else
regs->nip = (unsigned long)p->ainsn.insn;
@@ -152,7 +152,9 @@ static inline int kprobe_handler(struct
Disarm the probe we just hit, and ignore it. */
p = get_kprobe(addr);
if (p) {
- if (kprobe_status == KPROBE_HIT_SS) {
+ kprobe_opcode_t insn = *p->ainsn.insn;
+ if (kprobe_status == KPROBE_HIT_SS &&
+ is_trap(insn)) {
regs->msr &= ~MSR_SE;
regs->msr |= kprobe_saved_msr;
unlock_kprobes();
@@ -192,8 +194,7 @@ static inline int kprobe_handler(struct
* trap variant, it could belong to someone else
*/
kprobe_opcode_t cur_insn = *addr;
- if (IS_TW(cur_insn) || IS_TD(cur_insn) ||
- IS_TWI(cur_insn) || IS_TDI(cur_insn))
+ if (is_trap(cur_insn))
goto no_kprobe;
/*
* The breakpoint instruction was removed right
@@ -403,7 +404,7 @@ int __kprobes kprobe_exceptions_notify(s
default:
break;
}
- preempt_enable();
+ preempt_enable_no_resched();
return ret;
}
Index: linux-2.6.13-mm1/arch/x86_64/kernel/kprobes.c
===================================================================
--- linux-2.6.13-mm1.orig/arch/x86_64/kernel/kprobes.c
+++ linux-2.6.13-mm1/arch/x86_64/kernel/kprobes.c
@@ -311,7 +311,8 @@ int __kprobes kprobe_handler(struct pt_r
Disarm the probe we just hit, and ignore it. */
p = get_kprobe(addr);
if (p) {
- if (kprobe_status == KPROBE_HIT_SS) {
+ if (kprobe_status == KPROBE_HIT_SS &&
+ *p->ainsn.insn == BREAKPOINT_INSTRUCTION) {
regs->eflags &= ~TF_MASK;
regs->eflags |= kprobe_saved_rflags;
unlock_kprobes();
Index: linux-2.6.13-mm1/include/asm-ia64/kprobes.h
===================================================================
--- linux-2.6.13-mm1.orig/include/asm-ia64/kprobes.h
+++ linux-2.6.13-mm1/include/asm-ia64/kprobes.h
@@ -92,6 +92,7 @@ struct arch_specific_insn {
kprobe_opcode_t insn;
#define INST_FLAG_FIX_RELATIVE_IP_ADDR 1
#define INST_FLAG_FIX_BRANCH_REG 2
+ #define INST_FLAG_BREAK_INST 4
unsigned long inst_flag;
unsigned short target_br_reg;
};
Index: linux-2.6.13-mm1/include/asm-ppc64/kprobes.h
===================================================================
--- linux-2.6.13-mm1.orig/include/asm-ppc64/kprobes.h
+++ linux-2.6.13-mm1/include/asm-ppc64/kprobes.h
@@ -42,6 +42,9 @@ typedef unsigned int kprobe_opcode_t;
#define JPROBE_ENTRY(pentry) (kprobe_opcode_t *)((func_descr_t *)pentry)
+#define is_trap(instr) (IS_TW(instr) || IS_TD(instr) || \
+ IS_TWI(instr) || IS_TDI(instr))
+
#define ARCH_SUPPORTS_KRETPROBES
void kretprobe_trampoline(void);
Index: linux-2.6.13-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.13-mm1.orig/kernel/kprobes.c
+++ linux-2.6.13-mm1/kernel/kprobes.c
@@ -155,14 +155,22 @@ void __kprobes free_insn_slot(kprobe_opc
/* Locks kprobe: irqs must be disabled */
void __kprobes lock_kprobes(void)
{
+ unsigned long flags = 0;
+
+ local_irq_save(flags);
spin_lock(&kprobe_lock);
kprobe_cpu = smp_processor_id();
+ local_irq_restore(flags);
}
void __kprobes unlock_kprobes(void)
{
+ unsigned long flags = 0;
+
+ local_irq_save(flags);
kprobe_cpu = NR_CPUS;
spin_unlock(&kprobe_lock);
+ local_irq_restore(flags);
}
/* You have to be holding the kprobe_lock */
Keshavamurthy Anil S <[email protected]> wrote:
>
> This patch fixes a race condition where in system used to hang
> or sometime crash within minutes when kprobes are inserted on
> ISR routine and a task routine.
It's desirable that the patch descriptions tell us _how_ a bug was fixed,
as well as what the bug was. It means that people don't have to ask
questions like:
> void __kprobes lock_kprobes(void)
> {
> + unsigned long flags = 0;
> +
> + local_irq_save(flags);
> spin_lock(&kprobe_lock);
> kprobe_cpu = smp_processor_id();
> + local_irq_restore(flags);
> }
what is this change trying to do? If a lock is taken from both process and
irq contexts then local IRQs must be disabled for the entire period when the
lock is held, not just for a little blip like this. If IRQ-context code is
running this function then the code is deadlockable.
Now, probably there's deep magic happening here and I'm wrong. If so then
please explain the code's magic via a comment patch so the question doesn't
arise again, thanks.
On Thu, Sep 01, 2005 at 02:09:38PM -0700, Andrew Morton wrote:
> Keshavamurthy Anil S <[email protected]> wrote:
> >
> > This patch fixes a race condition where in system used to hang
> > or sometime crash within minutes when kprobes are inserted on
> > ISR routine and a task routine.
>
> It's desirable that the patch descriptions tell us _how_ a bug was fixed,
> as well as what the bug was. It means that people don't have to ask
> questions like:
Sure, our current kprobes model is very serialized where in we serve only
one kprobe in the exception handler by holding this lock_kprobes() and
release this lock i.e unlock_kprobes() when we are done with single stepping
>
> > void __kprobes lock_kprobes(void)
> > {
> > + unsigned long flags = 0;
> > +
> > + local_irq_save(flags);
> > spin_lock(&kprobe_lock);
> > kprobe_cpu = smp_processor_id();
> > + local_irq_restore(flags);
> > }
>
> what is this change trying to do? If a lock is taken from both process and
> irq contexts then local IRQs must be disabled for the entire period when the
> lock is held, not just for a little blip like this. If IRQ-context code is
> running this function then the code is deadlockable.
In the kprobe exception handling we relay on kprobe_cpu = smp_processor_id() to determine
whether we are inside the kprobe or not. It was so happeing that when we
take the lock and before kprobe_cpu gets updated if an H/W interrupt happens
and if kprobe is enabled on ISR routine, then in the kprobe execption handler
for isr, we miss the indication that we are already in kprobes(since interrupt
happened before we get to update kprobe_cpu) and we were trying to
take the lock again and there by causing the deadlock. This deadlock is avoided
by disabling the ISR for a short period while we take the spin_lock() and update
the kprobe_cpu.
>
> Now, probably there's deep magic happening here and I'm wrong. If so then
> please explain the code's magic via a comment patch so the question doesn't
> arise again, thanks.
>
This whole serialization will go away when we introduce the scalability patch.
-Anil
Keshavamurthy Anil S <[email protected]> wrote:
>
> > > void __kprobes lock_kprobes(void)
> > > {
> > > + unsigned long flags = 0;
> > > +
> > > + local_irq_save(flags);
> > > spin_lock(&kprobe_lock);
> > > kprobe_cpu = smp_processor_id();
> > > + local_irq_restore(flags);
> > > }
> >
> > what is this change trying to do? If a lock is taken from both process and
> > irq contexts then local IRQs must be disabled for the entire period when the
> > lock is held, not just for a little blip like this. If IRQ-context code is
> > running this function then the code is deadlockable.
>
> In the kprobe exception handling we relay on kprobe_cpu = smp_processor_id() to determine
> whether we are inside the kprobe or not. It was so happeing that when we
> take the lock and before kprobe_cpu gets updated if an H/W interrupt happens
> and if kprobe is enabled on ISR routine, then in the kprobe execption handler
> for isr, we miss the indication that we are already in kprobes(since interrupt
> happened before we get to update kprobe_cpu) and we were trying to
> take the lock again and there by causing the deadlock. This deadlock is avoided
> by disabling the ISR for a short period while we take the spin_lock() and update
> the kprobe_cpu.
OK.
Are you sure that other CPUs can safely read kprobe_cpu without taking the
lock? I don't see any memory barriers in there...
> >
> > Now, probably there's deep magic happening here and I'm wrong. If so then
> > please explain the code's magic via a comment patch so the question doesn't
> > arise again, thanks.
> >
>
> This whole serialization will go away when we introduce the scalability patch.
Yes, it does look rather unscalable.
On Thu, Sep 01, 2005 at 02:42:11PM -0700, Andrew Morton wrote:
> Are you sure that other CPUs can safely read kprobe_cpu without taking the
> lock? I don't see any memory barriers in there...
I see your point, in the current code we read kprobe_cpu and compare it with
ones own smp_processor_id() and if it *does* not match, then this cpu is allowed to
take the kprobe_lock(). So in this context, if one CPU is trying to read(kprobe_cpu)
the value while other is updating its own smp_processor_id() value,
then this cpu can either get NR_CPU(stale data) or the correct CPU_ID
of the other processor, which in any case does not match with its
own smp_processor_id() and we are allowed to take the lock(where in we might have to
wait spinning since we are any way serializing handling of probes).
So to answer your question, for our current desing, we are safe to read
kprobe_cpu outside of the lock.
Here is the link which gives the status of testing this patch on
various platforms.
http://sourceware.org/bugzilla/show_bug.cgi?id=1229
On Thu, Sep 01, 2005 at 02:09:38PM -0700, Andrew Morton wrote:
> Now, probably there's deep magic happening here and I'm wrong. If so then
> please explain the code's magic via a comment patch so the question doesn't
> arise again, thanks.
>
This is a comment patch around lock_kprobes() and unlock_kprobes() functions.
Signed-off-by: Anil S Keshavamurthy <[email protected]>
===================================================================
kernel/kprobes.c | 14 ++++++++++++++
1 files changed, 14 insertions(+)
Index: linux-2.6.13-mm1/kernel/kprobes.c
===================================================================
--- linux-2.6.13-mm1.orig/kernel/kprobes.c
+++ linux-2.6.13-mm1/kernel/kprobes.c
@@ -157,9 +157,16 @@ void __kprobes lock_kprobes(void)
{
unsigned long flags = 0;
+ /* Avoiding local interrupts to happen right after we take the kprobe_lock
+ * and before we get a chance to update kprobe_cpu, this to prevent
+ * deadlock when we have a kprobe on ISR routine and a kprobe on task
+ * routine
+ */
local_irq_save(flags);
+
spin_lock(&kprobe_lock);
kprobe_cpu = smp_processor_id();
+
local_irq_restore(flags);
}
@@ -167,9 +174,16 @@ void __kprobes unlock_kprobes(void)
{
unsigned long flags = 0;
+ /* Avoiding local interrupts to happen right after we update
+ * kprobe_cpu and before we get a a chance to release kprobe_lock,
+ * this to prevent deadlock when we have a kprobe on ISR routine and
+ * a kprobe on task routine
+ */
local_irq_save(flags);
+
kprobe_cpu = NR_CPUS;
spin_unlock(&kprobe_lock);
+
local_irq_restore(flags);
}