On Mon, Mar 20, 2006 at 12:40:49PM -0800, Andrew Morton wrote:
> Prasanna S Panchamukhi <[email protected]> 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.
>
Yes, I will use inc_preempt_count() instead of preempt_disable().
> > > > + */
> > > > +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.
Agreed. I will look at this issue and remove the __lock_page().
>
> > > > + 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?
There is a small window in which other processor will not be able to see
the breakpoint if we are single step inline. But since single stepping inline
is a bad idea, we will disarm the probe forever (replace with original instrcution) if we cannot single step out-of-line.
Any suggestions?
Thanks
Prasanna
--
Prasanna S Panchamukhi
Linux Technology Center
India Software Labs, IBM Bangalore
Email: [email protected]
Ph: 91-80-51776329
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
