Hello.
With kprobes enabled, vm86 doesn't feel
good. The problem is that kprobes steal
the interrupts (mainly int3 I think) from
it for no good reason.
The attached patch fixes the problem by
checking the VM flag where it makes sense.
Andrew, can this please be applied?
Signed-off-by: Stas Sergeev <[email protected]>
> Hello.
>
Hi,
> With kprobes enabled, vm86 doesn't feel
> good. The problem is that kprobes steal
> the interrupts (mainly int3 I think) from
> it for no good reason.
If the int3 is not registered through kprobes,
kprobes handler does not handle it and it falls through the
normal int3 handler AFAIK.
Could you please provide a test case to show that kprobes
steals the interrupts.
Thanks
Prasanna
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<[email protected]>
Hi.
Prasanna S Panchamukhi wrote:
>> With kprobes enabled, vm86 doesn't feel
>> good. The problem is that kprobes steal
>> the interrupts (mainly int3 I think) from
>> it for no good reason.
> If the int3 is not registered through kprobes,
> kprobes handler does not handle it and it falls through the
> normal int3 handler AFAIK.
I was considering this, but I convinced
myself that checking the VM flag is good
in any case, because, as I presume, you
never need the interrupts from v86. Or do
you?
If there is a bug in kprobes, it would be
good to fix either, but I just think it
will not make my patch completely useless.
> Could you please provide a test case to show that kprobes
> steals the interrupts.
Sure, attached. But it is not perfect: on
the patched kernel it passes the test, but
on the unpatched one (2.6.9), it just Oopses
the kernel without printing any reasonable
diagnostic. Because of the Oops, I can't
demonstrate the interrupt theft right away,
but I hope the test-case for the Oops in
kprobe_exceptions_notify() may also be
interesting for you.
Hello,
>
> Prasanna S Panchamukhi wrote:
> >>With kprobes enabled, vm86 doesn't feel
> >>good. The problem is that kprobes steal
> >>the interrupts (mainly int3 I think) from
> >>it for no good reason.
> >If the int3 is not registered through kprobes,
> >kprobes handler does not handle it and it falls through the
> >normal int3 handler AFAIK.
> I was considering this, but I convinced
> myself that checking the VM flag is good
> in any case, because, as I presume, you
> never need the interrupts from v86. Or do
> you?
> If there is a bug in kprobes, it would be
> good to fix either, but I just think it
> will not make my patch completely useless.
>
Yes, there is a small bug in kprobes. Kprobes int3 handler
was returning wrong value. Please check out if the patch
attached with this mail fixes your problem.
Please let me know if you have any issues.
Thanks
Prasanna
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<[email protected]>
Hi.
Prasanna S Panchamukhi wrote:
> Yes, there is a small bug in kprobes. Kprobes int3 handler
> was returning wrong value. Please check out if the patch
> attached with this mail fixes your problem.
The patch is tested and works - thanks.
Hi Stas,
On Thu, Dec 09, 2004 at 10:28:25PM +0300, Stas Sergeev wrote:
> OK, but if you need another test-case,
> here it is. Much simpler than the vm86 one.
> It can work in 2 modes: started without args,
> it will print the diagnostic (passed or
> failed) and exit. If started with any arg,
> it will Oops the kernel.
> This happens both with your latest patch
> and without it. This doesn't happen with
> your previous patch (no Oops), but then fixing
> problems by exploiting the gcc optimization
> was not the best idea I think.
>
The patch below fixes this problem.
Please let me know your comments.
Thanks
Prasanna
The address used by the kprobes handler was not correct if the application
was using LDT entries for code segment. This patch fixes the above problem by
calculating the address using base address of the current code segment.
Also this patch removes the inline prefix of kprobe_handler() .
Signed-off-by: Prasanna S Panchamukhi <[email protected]>
---
linux-2.6.10-prasanna/arch/i386/kernel/kprobes.c | 19 ++++++++++++++++---
1 files changed, 16 insertions(+), 3 deletions(-)
diff -puN arch/i386/kernel/kprobes.c~kprobes-corrupt-eip arch/i386/kernel/kprobes.c
--- linux-2.6.10/arch/i386/kernel/kprobes.c~kprobes-corrupt-eip 2005-01-07 17:01:37.000000000 +0530
+++ linux-2.6.10-prasanna/arch/i386/kernel/kprobes.c 2005-01-07 17:01:49.000000000 +0530
@@ -86,15 +86,28 @@ static inline void prepare_singlestep(st
* Interrupts are disabled on entry as trap3 is an interrupt gate and they
* remain disabled thorough out this function.
*/
-static inline int kprobe_handler(struct pt_regs *regs)
+static int kprobe_handler(struct pt_regs *regs)
{
struct kprobe *p;
int ret = 0;
- u8 *addr = (u8 *) (regs->eip - 1);
+ kprobe_opcode_t *addr = NULL;
+ unsigned long *lp;
/* We're in an interrupt, but this is clear and BUG()-safe. */
preempt_disable();
-
+ /* Check if the application is using LDT entry for its code segment and
+ * calculate the address by reading the base address from the LDT entry.
+ */
+ if ((regs->xcs & 4) && (current->mm)) {
+ lp = (unsigned long *) ((unsigned long)((regs->xcs >> 3) * 8)
+ + (char *) current->mm->context.ldt);
+ addr = (kprobe_opcode_t *) ((((*lp) >> 16 & 0x0000ffff)
+ | (*(lp +1) & 0xff000000)
+ | ((*(lp +1) << 16) & 0x00ff0000))
+ + regs->eip - sizeof(kprobe_opcode_t));
+ } else {
+ addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));
+ }
/* Check we're not actually recursing */
if (kprobe_running()) {
/* We *are* holding lock here, so this is safe.
_
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<[email protected]>
Prasanna S Panchamukhi <[email protected]> writes:
> + /* Check if the application is using LDT entry for its code segment and
> + * calculate the address by reading the base address from the LDT entry.
> + */
> + if ((regs->xcs & 4) && (current->mm)) {
> + lp = (unsigned long *) ((unsigned long)((regs->xcs >> 3) * 8)
> + + (char *) current->mm->context.ldt);
> + addr = (kprobe_opcode_t *) ((((*lp) >> 16 & 0x0000ffff)
> + | (*(lp +1) & 0xff000000)
> + | ((*(lp +1) << 16) & 0x00ff0000))
> + + regs->eip - sizeof(kprobe_opcode_t));
> + } else {
> + addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));
> + }
With that patch we would have LDT reading code three times in the kernel
now (ptrace, prefetch workaround and now this). How about you factor
this out into a common helper function? This stuff is tricky enough
that there are likely bugs in there anyways and it would be best
to only fix them at one place then.
-Andi
Hi.
Prasanna S Panchamukhi wrote:
> The patch below fixes this problem.
> Please let me know your comments.
The patch works, thanks. I have no
complains to it, it fixes the problem
it is intended to fix.
The following is just a reminder about
the other problems I mentioned earlier.
This problem is still not addressed:
http://lkml.org/lkml/2004/12/4/48
Also VM86 check should be done before the
"addr" is used I think, is this true?
And dereferencing the pointer to user-space,
like "*addr", is this safe? I thought
get_user() is used for that, was this
intentional?
By the way, maybe it is possible to avoid
the removal race without checking an opcode
at all? Probably by marking the probes as
"removed", and checking the "addr" against
the "removed" ones instead of checking the
opcode? This way you'll avoid any interference
with the debuggers that can also remove their
breakpoints, and also the aforementioned
problem will get fixed automatically.
Hi Andi,
> > + addr = (kprobe_opcode_t *) ((((*lp) >> 16 & 0x0000ffff)
> > + | (*(lp +1) & 0xff000000)
> > + | ((*(lp +1) << 16) & 0x00ff0000))
> With that patch we would have LDT reading code three times in the kernel
> now (ptrace, prefetch workaround and now this). How about you factor
> this out into a common helper function? This stuff is tricky enough
> that there are likely bugs in there anyways and it would be best
> to only fix them at one place then.
The patch below moves this tricky code to a common place, please let
me know your comments. Ptrace uses a structure instead of unsigned long *.
Thanks
Prasanna
Calculating the base address of the segment is tricky and
is used in several places as well. This patch moves this tricky part
in a common place as suggested by Andi Kleen.
Signed-of-by: Prasanna S Panchamukhi <[email protected]>
---
linux-2.6.11-rc1-prasanna/arch/i386/kernel/kprobes.c | 7 +++----
linux-2.6.11-rc1-prasanna/arch/i386/mm/fault.c | 4 +---
linux-2.6.11-rc1-prasanna/include/asm-i386/desc.h | 9 +++++++++
3 files changed, 13 insertions(+), 7 deletions(-)
diff -puN arch/i386/mm/fault.c~kprobes-desc-common-routine arch/i386/mm/fault.c
--- linux-2.6.11-rc1/arch/i386/mm/fault.c~kprobes-desc-common-routine 2005-01-13 11:29:49.000000000 +0530
+++ linux-2.6.11-rc1-prasanna/arch/i386/mm/fault.c 2005-01-13 11:36:08.000000000 +0530
@@ -112,9 +112,7 @@ static inline unsigned long get_segment_
}
/* Decode the code segment base from the descriptor */
- base = (desc[0] >> 16) |
- ((desc[1] & 0xff) << 16) |
- (desc[1] & 0xff000000);
+ base = get_desc_base((unsigned long *)desc);
if (seg & (1<<2)) {
up(¤t->mm->context.sem);
diff -puN arch/i386/kernel/kprobes.c~kprobes-desc-common-routine arch/i386/kernel/kprobes.c
--- linux-2.6.11-rc1/arch/i386/kernel/kprobes.c~kprobes-desc-common-routine 2005-01-13 11:30:01.000000000 +0530
+++ linux-2.6.11-rc1-prasanna/arch/i386/kernel/kprobes.c 2005-01-13 11:44:43.000000000 +0530
@@ -31,6 +31,7 @@
#include <linux/spinlock.h>
#include <linux/preempt.h>
#include <asm/kdebug.h>
+#include <asm/desc.h>
/* kprobe_status settings */
#define KPROBE_HIT_ACTIVE 0x00000001
@@ -101,10 +102,8 @@ static int kprobe_handler(struct pt_regs
if ((regs->xcs & 4) && (current->mm)) {
lp = (unsigned long *) ((unsigned long)((regs->xcs >> 3) * 8)
+ (char *) current->mm->context.ldt);
- addr = (kprobe_opcode_t *) ((((*lp) >> 16 & 0x0000ffff)
- | (*(lp +1) & 0xff000000)
- | ((*(lp +1) << 16) & 0x00ff0000))
- + regs->eip - sizeof(kprobe_opcode_t));
+ addr = (kprobe_opcode_t *) (get_desc_base(lp) + regs->eip -
+ sizeof(kprobe_opcode_t));
} else {
addr = (kprobe_opcode_t *)(regs->eip - sizeof(kprobe_opcode_t));
}
diff -puN include/asm-i386/desc.h~kprobes-desc-common-routine include/asm-i386/desc.h
--- linux-2.6.11-rc1/include/asm-i386/desc.h~kprobes-desc-common-routine 2005-01-13 11:30:11.000000000 +0530
+++ linux-2.6.11-rc1-prasanna/include/asm-i386/desc.h 2005-01-13 11:31:36.000000000 +0530
@@ -126,6 +126,15 @@ static inline void load_LDT(mm_context_t
put_cpu();
}
+static inline unsigned long get_desc_base(unsigned long *desc)
+{
+ unsigned long base;
+ base = ((desc[0] >> 16) & 0x0000ffff) |
+ ((desc[1] << 16) & 0x00ff0000) |
+ (desc[1] & 0xff000000);
+ return base;
+}
+
#endif /* !__ASSEMBLY__ */
#endif
_
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Ph: 91-80-25044636
<[email protected]>