Messages in this thread | | | Date | Mon, 20 Mar 2006 12:40:49 -0800 | From | Andrew Morton <> | Subject | Re: [3/3 PATCH] Kprobes: User space probes support- single stepping out-of-line |
| |
Prasanna S Panchamukhi <prasanna@in.ibm.com> wrote: > > > > + > > > + if (__copy_to_user_inatomic((unsigned long *)addr, > > > + (unsigned long *)uprobe->kp.ainsn.insn, size)) > > > + return -EFAULT; > > > + > > > + regs->eip = addr; > > > + > > > + return 0; > > > +} > > > > If we're going to use __copy_to_user_inatomic() then we'll need some nice > > comments explaining why this is happening. > > > > And we'll need to actually *be* in-atomic. That means we need an > > open-coded inc_preempt_count() and dec_preempt_count() in there and I don't > > see them. > > > > We come here, after probe is hit, through uporbe_handler() with > interrupts disabled (since it is a interrupt gate). In uprobe_handler() > preemption is disabled and remains disabled until original instruction > is single stepped. > > I will add proper comments in next iteration.
preempt_disable() is insufficient - it is a no-op on !CONFIG_PREEMPT.
You _must_ run inc_preempt_count(). See how kmap_atomic() and kunmap_atomic() work.
> > > + */ > > > +void __kprobes replace_original_insn(struct uprobe *uprobe, > > > + struct pt_regs *regs, kprobe_opcode_t opcode) > > > +{ > > > + kprobe_opcode_t *addr; > > > + struct page *page; > > > + > > > + page = find_get_page(uprobe->inode->i_mapping, > > > + uprobe->offset >> PAGE_CACHE_SHIFT); > > > + BUG_ON(!page); > > > + > > > + __lock_page(page); > > > > Whoa. Why is __lock_page() being used here? It looks like a bug is being > > covered up. > > > > we come here with a spinlock held. I will add the comment.
Then the code is buggy. __lock_page() can schedule away, causing this CPU to recur onto the same lock and deadlock.
> > > + addr = (kprobe_opcode_t *)kmap_atomic(page, KM_USER1); > > > + addr = (kprobe_opcode_t *)((unsigned long)addr + > > > + (unsigned long)(uprobe->offset & ~PAGE_MASK)); > > > + *addr = opcode; > > > + /*TODO: flush vma ? */ > > > > flush_dcache_page() would be needed. > > > > But then, what happens if the page is shared by other processes? Do they > > all start taking debug traps? > > Yes, you are right. I think single stepping inline was a bad idea, disarming > the probe looks to be a better option >
You skipped my second question? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |