lkml.org 
[lkml]   [2007]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 4/8] Immediate Value - i386 Optimization; kprobes
    On 06/18/2007 10:57 AM, Mathieu Desnoyers wrote:
    > * Chuck Ebbert (cebbert@redhat.com) wrote:
    >>> + return NOTIFY_STOP;
    >>> + }
    >>> + return NOTIFY_DONE;
    >>> +}
    >>> +
    >>> +static struct notifier_block immediate_notify = {
    >>> + .notifier_call = immediate_notifier,
    >>> + .priority = 0x7fffffff, /* we need to be notified first */
    >>> +};
    >>> +
    >>> +/*
    >>> + * The address is not aligned. We can only change 1 byte of the value
    >>> + * atomically.
    >>> + * Must be called with immediate_mutex held.
    >>> + */
    >>> +int immediate_optimized_set_enable(void *address, char enable)
    >>> +{
    >>> + char saved_byte;
    >>> + int ret;
    >>> + char *dest = address;
    >>> +
    >>> + if (!(enable ^ dest[1])) /* Must be a state change 0<->1 to execute */
    >>> + return 0;
    >>> +
    >>> +#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_PAGEALLOC)
    >>> + /* Make sure this page is writable */
    >>> + change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_EXEC);
    >>> + global_flush_tlb();
    >>> +#endif
    >> Can't we have a macro or inline to do this, and the setting back
    >> to read-only? kprobes also has the same ugly #if blocks...
    >>
    >> Hmm, what happens if you race with kprobes setting a probe on
    >> the same page? Couldn't it unexpectedly become read-only?
    >>
    >
    > Hi Chuck,
    >
    > I am looking more closely at kprobes; a few comments while we are here:
    >
    > 1 - Why is kprobe_count an atomic_t variable instead of a simple
    > integer? It is always protected by the kprobe_mutex anyway. All this
    > synchronization seems redundant.
    >
    > 2 - I wonder where is the equivalent of my snippet in kprobes code:
    >>> +#if defined(CONFIG_DEBUG_RODATA) || defined(CONFIG_DEBUG_PAGEALLOC)
    >>> + /* Make sure this page is writable */
    >>> + change_page_attr(virt_to_page(address), 1, PAGE_KERNEL_EXEC);
    >>> + global_flush_tlb();
    >>> +#endif
    >
    > I fancy it's done by the kprobe_page_fault handler, but I do not see
    > clearly how writing the breakpoint from arch_arm_kprobe() in
    > non-writeable memory is done.

    Looks like it's not merged yet:

    http://lkml.org/lkml/2007/6/7/2

    This needs to go in before 2.6.22-final

    >
    > In any case, I would like not to use that kind of approach; I prefer to
    > set the memory page to RWX before doing the memory write so I do not
    > depend on the page fault handler (remember that I instrument the page
    > fault handler itself...).
    >
    > Maybe we could use a shared "text_mutex" between kprobes and
    > immediate values to insure mutual exclusion for .text modification.
    > However, we would still have the following coherency issue when an
    > immediate value and a kprobe share the same address:
    >
    > 1- enable immediate value
    > 2- put a kprobe at the immediate value load instruction address
    > 3- disable immediate value
    > 4- remove kprobe
    >
    > The kprobe removal would put back the load immediate instruction and
    > would not touch the loaded value (which is ok). However, the instruction
    > copy kept by kprobes would not be in sync with the immediate value
    > state:
    >
    > Scenario 1: kprobes int3 handler first:
    >
    > 1- enable immediate value
    > 2- put a kprobe at the immediate value load instruction address
    >
    > -> int3 triggered
    > kprobe handler runs. Single-steps the "enabled" state.
    >
    > 3- disable immediate value
    >
    > -> int3 triggered
    > kprobe handler runs. Single-steps the "enabled" state. This state is
    > wrong.
    >
    > 4- remove kprobe
    >
    >
    > Scenario 2: immediate value int3 handler first:
    >
    > 1- enable immediate value
    > 2- put a kprobe at the immediate value load instruction address
    >
    > -> int3 triggered
    > kprobe handler runs. Single-steps the "enabled" state.
    >
    > 3- disable immediate value
    > -> int3 triggered (while we disable)
    > While we disable, the immediate value int3 handler is executed first. It
    > would cause the kprobe handler not to be called, and no "missing"
    > counter would be incremented.
    >
    > kprobe handler runs. Single-steps the "enabled" state. This state is
    > wrong.
    >
    > 4- remove kprobe
    >
    >
    > Since I don't want to depend on kprobes to put the breakpoint, because
    > of its reentrancy limitations (see all the __probes functions), It would
    > be good to figure out a mutual exclusion mechanism between immediate
    > values and kprobes. Maybe we could forbid kprobes to insert probes on
    > addresses present in the immediate values tables ? Or better: if we
    > detect this scenario, we could put the kprobe breakpoint at the
    > instruction following the "movl".
    >

    That's up to you and the kprobes people, I guess...


    -
    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: 2007-06-18 20:47    [W:0.033 / U:188.200 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site