Re: Review patches of user space kprobe

[Prasanna S Panchamukhi <prasanna at in dot ibm dot com>]

> The 4th patch is to avoid probes misses in smp environment. I just have
> some initial comments because the patch is not complete.
> 

> >>+
> >>+	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.

This will be fixed in the next release.
> 
> 
> >>+		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? 
> 

For the safety reasons, some space is left below the stack pointer, so that the probed 
instruction may use this space while single stepping.

> >> {
> >>@@ -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.

As of now it is not be handled but, it can be handled by allowing the page fault to
succeed or expanding the stack space or  replacing the original instruction if nothing
 can be done.

> 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.
> 

If its an illegal instruction, kprobe_fault_handler() is given control,
so that it can take care of restoring it cleanly.

Thanks
Prasanna
-- 
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Email: prasanna@in.ibm.com
Ph: 91-80-25044636