This is the mail archive of the
systemtap@sourceware.org
mailing list for the systemtap project.
RE: Review patches of user space kprobe
- From: "Zhang, Yanmin" <yanmin dot zhang at intel dot com>
- To: <prasanna at in dot ibm dot com>
- Cc: <systemtap at sources dot redhat dot com>, "Keshavamurthy, Anil S" <anil dot s dot keshavamurthy at intel dot com>, "Mao, Bibo" <bibo dot mao at intel dot com>
- Date: Thu, 5 Jan 2006 15:09:28 +0800
- Subject: RE: Review patches of user space kprobe
The 4th patch is to avoid probes misses in smp environment. I just have
some initial comments because the patch is not complete.
See the inline comments below.
Yanmin
>>---
>>
>> linux-2.6.13-prasanna/arch/i386/kernel/kprobes.c | 52
++++++++++++++++++++---
>> 1 files changed, 46 insertions(+), 6 deletions(-)
>>
>>diff -puN arch/i386/kernel/kprobes.c~kprobes_avoid_smp_missprobes
arch/i386/kernel/kprobes.c
>>---
linux-2.6.13/arch/i386/kernel/kprobes.c~kprobes_avoid_smp_missprobes
2005-09-15 13:56:24.105192248 +0530
>>+++ linux-2.6.13-prasanna/arch/i386/kernel/kprobes.c 2005-09-15
13:56:24.137187384 +0530
>>+/*
>>+ * This routine provides the functionality of single stepping out of
line by
>>+ * copying the original instruction in the user process free stack
space.
>>+ */
>>+static inline int uprobe_single_step(struct uprobe *p, struct pt_regs
*regs)
>>+{
>>+ unsigned long page_addr, stack_addr = regs->esp;
>>+ struct vm_area_struct *vma = NULL;
>>+ int size = MAX_INSN_SIZE * sizeof(kprobe_opcode_t);
>>+
>>+ if (!(vma = find_vma(current->mm, PAGE_ALIGN(stack_addr)))) {
>>+ printk("No vma found\n");
>>+ return -ENOENT;
>>+ }
>>+
>>+ if (vma->vm_flags & VM_GROWSDOWN) {
>>+ page_addr = PAGE_ALIGN(stack_addr);
It's wrong to use PAGE_ALIGN here. I think mostly page_addr is bigger
than stack_addr. Pls. use stack_addr&PAGE_MASK.
>>+ if (((stack_addr - sizeof(long long)) - page_addr) >
size) {
Pay attention to the type of stack_add and page_addr. They are unsigned
long. So it's better to change it to:
if ((stack_addr - sizeof(long long)) > (size + page_addr)) {
And why consider a sizeof(long long) here?
>>+ if (copy_to_user((unsigned long *)(page_addr +
size),
>>+ (unsigned long
*)&p->kp.ainsn.insn,
>>+ size))
>>+ return -EFAULT;
>>+ regs->eip = page_addr + size;
>>+ }
>>+ } else {
>>+ page_addr = PAGE_ALIGN(stack_addr) + PAGE_SIZE;
Similar issue.
>>+ if ((page_addr - (stack_addr + sizeof(long long))) >
size) {
Similar issue.
>>+ if (copy_to_user((unsigned long *)page_addr,
>>+ (unsigned long
*)&p->kp.ainsn.insn,
>>+\ size))
>>+ return -EFAULT;
>>+ regs->eip = page_addr;
>>+ }
>>+ }
>>+ singlestep_addr = (kprobe_opcode_t *)regs->eip;
>>+ return 0;
>>+}
>>
>> static inline void prepare_singlestep(struct kprobe *p, struct
pt_regs *regs)
>> {
>>@@ -140,10 +178,13 @@ static inline void prepare_singlestep(st
>> if (p->opcode == BREAKPOINT_INSTRUCTION)
>> regs->eip = (unsigned long)p->addr;
>> else {
>>- if (!kernel_text_address((unsigned long)p->addr)) {
>>- arch_disarm_uprobe(current_uprobe);
>>- regs->eip = (unsigned long)uprobe_addr;
>>- } else
>>+ if (!kernel_text_address((unsigned long)p->addr))
>>+ uprobe_single_step(current_uprobe, regs);
1) If uprobe_single_step returns -EFAULT, how does the thread go ahead?
Note the first byte of the original instruction is break now, so the
instruction from the second byte is wrong.
2) If prepare_singlestep succeeds, but later the real instruction of
single step might cause a fatal error, for example, to access illegal
address, then the process will be killed. Would the current
kprobe(uprobe) environment be restored clearly? Such like kprobe_cpu,
kprobe_status and so on.