lkml.org 
[lkml]   [2006]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2006-03-20 21:48    [W:0.028 / U:0.260 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site