lkml.org 
[lkml]   [2009]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH 2/6] jump label v3 - x86: Introduce generic jump patching without stop_machine
    On Wed, Nov 18, 2009 at 08:57:56PM -0500, Mathieu Desnoyers wrote:
    > * Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
    > > On Wed, Nov 18, 2009 at 07:28:26PM -0500, Mathieu Desnoyers wrote:
    > > > * Jason Baron (jbaron@redhat.com) wrote:
    > > > > Add text_poke_fixup() which takes a fixup address to where a processor
    > > > > jumps if it hits the modifying address while code modifying.
    > > > > text_poke_fixup() does following steps for this purpose.
    > > > >
    > > > > 1. Setup int3 handler for fixup.
    > > > > 2. Put a breakpoint (int3) on the first byte of modifying region,
    > > > > and synchronize code on all CPUs.
    > > > > 3. Modify other bytes of modifying region, and synchronize code on all CPUs.
    > > > > 4. Modify the first byte of modifying region, and synchronize code
    > > > > on all CPUs.
    > > > > 5. Clear int3 handler.
    > > > >
    > > >
    > > > Hi Masami,
    > > >
    > > > I like the approach and the API is clean. I have intersped comments
    > > > below.
    > > >
    > > > Ingo: I raise a question about text_mutex vs on_each_cpu hangs I
    > > > experienced recently in the message below. Might be worth having a look,
    > > > I suspect this might have caused the hangs Paul McKenney had with his
    > > > past TREE RCU callback migration. I think he did take a mutex in the cpu
    > > > hotplug callbacks and might have used IPIs within that same lock.
    > >
    > > Hello, Mathieu,
    > >
    > > By "mutex", do you mean "mutex_lock()"? If so, I don't do that from
    > > within the CPU hotplug notifiers. I do spin_lock_irqsave().
    > >
    > > People have been known to invoke synchronize_rcu() from CPU hotplug
    > > notifiers, however -- and this does block. Is that what you are
    > > getting at?
    >
    > Hi Paul,
    >
    > What I had in mind is more like a N-way deadlock involving the cpu
    > hotplug mutex, on_each_cpu, and possibly stop_machine interaction.
    > However I did not push the lockup analysis far enough to know for sure,
    > but I feel like it's good to let others know. I am not sure if blocking
    > primitives could be affected by this.

    Then you might be interested to hear that my attempts to move
    rcu_offline_cpu() to the CPU_DYING and rcu_online_cpu() to CPU_STARTING
    did result in howls of pain from lockdep as well as deadlocks.
    I abandoned such attempts. ;-)

    (The reason behind my attempts was to reduce the number and complexity
    of race conditions in the RCU implementations -- but no big deal, as I
    have found other ways.)

    > > I do invoke smp_send_reschedule() with irqs disabled, which did arouse my
    > > suspicions of late. But this seems to bypass the smp_call_function()
    > > code that is most definitely illegal to invoke with irqs disabled,
    > > so no smoking gun. All that aside, if invoking smp_send_reschedule()
    > > with irqs disabled is in any way a bad idea, please let me know so I
    > > can rearrange the code appropriately.
    > >
    > > RCU is currently running reasonably well with the set of patches I have
    > > submitted. It the kernel is now withstanding a level of punishment that
    > > would have reduced 2.6.28 (for example) to a steaming pile of bits, with
    > > or without the help of rcutorture. And I am now hitting the occasional
    > > non-RCU bug, so I am starting to feel like RCU is approaching stability.
    > > Approaching, but not there yet -- a few suspicious areas remain. Time
    > > to crank the testing up another notch or two. ;-)
    >
    > Nice :) I've been going through the same "stabilization" phase in the
    > past weeks for LTTng. It's good to see things stabilize even under heavy
    > cpu hotplug stress-tests.

    Cool!!! ;-)

    Thanx, Paul

    > Thanks,
    >
    > Mathieu
    >
    > >
    > > Thanx, Paul
    > >
    > > > > Thus, if some other processor execute modifying address when step2 to step4,
    > > > > it will be jumped to fixup code.
    > > > >
    > > > > This still has many limitations for modifying multi-instructions at once.
    > > > > However, it is enough for 'a 5 bytes nop replacing with a jump' patching,
    > > > > because;
    > > > > - Replaced instruction is just one instruction, which is executed atomically.
    > > > > - Replacing instruction is a jump, so we can set fixup address where the jump
    > > > > goes to.
    > > > >
    > > > > Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
    > > > > ---
    > > > > arch/x86/include/asm/alternative.h | 12 ++++
    > > > > arch/x86/include/asm/kprobes.h | 1 +
    > > > > arch/x86/kernel/alternative.c | 120 ++++++++++++++++++++++++++++++++++++
    > > > > kernel/kprobes.c | 2 +-
    > > > > 4 files changed, 134 insertions(+), 1 deletions(-)
    > > > >
    > > >
    > > > [snip snip]
    > > >
    > > > > index de7353c..af47f12 100644
    > > > > --- a/arch/x86/kernel/alternative.c
    > > > > +++ b/arch/x86/kernel/alternative.c
    > > > > @@ -4,6 +4,7 @@
    > > > > #include <linux/list.h>
    > > > > #include <linux/stringify.h>
    > > > > #include <linux/kprobes.h>
    > > > > +#include <linux/kdebug.h>
    > > > > #include <linux/mm.h>
    > > > > #include <linux/vmalloc.h>
    > > > > #include <linux/memory.h>
    > > > > @@ -552,3 +553,122 @@ void *__kprobes text_poke(void *addr, const void *opcode, size_t len)
    > > > > local_irq_restore(flags);
    > > > > return addr;
    > > > > }
    > > > > +
    > > > > +/*
    > > > > + * On pentium series, Unsynchronized cross-modifying code
    > > > > + * operations can cause unexpected instruction execution results.
    > > > > + * So after code modified, we should synchronize it on each processor.
    > > > > + */
    > > > > +static void __kprobes __local_sync_core(void *info)
    > > > > +{
    > > > > + sync_core();
    > > > > +}
    > > > > +
    > > > > +void __kprobes sync_core_all(void)
    > > > > +{
    > > > > + on_each_cpu(__local_sync_core, NULL, 1);
    > > >
    > > > OK, so you rely on the fact that on_each_cpu has memory barriers and
    > > > executes the remote "__local_sync_core" with the appropriate memory
    > > > barriers underneath, am I correct ?
    > > >
    > > > > +}
    > > > > +
    > > > > +/* Safely cross-code modifying with fixup address */
    > > > > +static void *patch_fixup_from;
    > > > > +static void *patch_fixup_addr;
    > > > > +
    > > > > +static int __kprobes patch_exceptions_notify(struct notifier_block *self,
    > > > > + unsigned long val, void *data)
    > > > > +{
    > > > > + struct die_args *args = data;
    > > > > + struct pt_regs *regs = args->regs;
    > > > > +
    > > > > + if (likely(!patch_fixup_from))
    > > > > + return NOTIFY_DONE;
    > > > > +
    > > > > + if (val != DIE_INT3 || !regs || user_mode_vm(regs) ||
    > > > > + (unsigned long)patch_fixup_from != regs->ip)
    > > > > + return NOTIFY_DONE;
    > > > > +
    > > > > + args->regs->ip = (unsigned long)patch_fixup_addr;
    > > > > + return NOTIFY_STOP;
    > > > > +}
    > > > > +
    > > > > +/**
    > > > > + * text_poke_fixup() -- cross-modifying kernel text with fixup address.
    > > > > + * @addr: Modifying address.
    > > > > + * @opcode: New instruction.
    > > > > + * @len: length of modifying bytes.
    > > > > + * @fixup: Fixup address.
    > > > > + *
    > > > > + * Note: You must backup replaced instructions before calling this,
    > > > > + * if you need to recover it.
    > > > > + * Note: Must be called under text_mutex.
    > > > > + */
    > > > > +void *__kprobes text_poke_fixup(void *addr, const void *opcode, size_t len,
    > > > > + void *fixup)
    > > > > +{
    > > > > + static const unsigned char int3_insn = BREAKPOINT_INSTRUCTION;
    > > > > + static const int int3_size = sizeof(int3_insn);
    > > > > +
    > > > > + /* Replacing 1 byte can be done atomically. */
    > > >
    > > > I'm sure it can be done atomically, but can it be done safely though ?
    > > >
    > > > (disclaimer: we're still waiting for the official answer on this
    > > > statement): Assuming instruction trace cache effects of SMP cross-code
    > > > modification, and that only int3 would be safe to patch, then even
    > > > changing 1 single byte could only be done by going to an intermediate
    > > > int3 and synchronizing all other cores.
    > > >
    > > > > + if (unlikely(len <= 1))
    > > > > + return text_poke(addr, opcode, len);
    > > > > +
    > > > > + /* Preparing */
    > > > > + patch_fixup_addr = fixup;
    > > > > + wmb();
    > > >
    > > > hrm, missing comment ?
    > > >
    > > > > + patch_fixup_from = (u8 *)addr + int3_size; /* IP address after int3 */
    > > > > +
    > > > > + /* Cap by an int3 */
    > > > > + text_poke(addr, &int3_insn, int3_size);
    > > > > + sync_core_all();
    > > > > +
    > > > > + /* Replace tail bytes */
    > > > > + text_poke((char *)addr + int3_size, (const char *)opcode + int3_size,
    > > > > + len - int3_size);
    > > > > + sync_core_all();
    > > > > +
    > > > > + /* Replace int3 with head byte */
    > > > > + text_poke(addr, opcode, int3_size);
    > > > > + sync_core_all();
    > > > > +
    > > > > + /* Cleanup */
    > > > > + patch_fixup_from = NULL;
    > > > > + wmb();
    > > >
    > > > missing comment here too.
    > > >
    > > > > + return addr;
    > > >
    > > > Little quiz question:
    > > >
    > > > When patch_fixup_from is set to NULL, what ensures that the int3
    > > > handlers have completed their execution ?
    > > >
    > > > I think it's probably OK, because the int3 is an interrupt gate, which
    > > > therefore disables interrupts as soon as it runs, and executes the
    > > > notifier while irqs are off. When we run sync_core_all() after replacing
    > > > the int3 by the new 1st byte, we only return when all other cores have
    > > > executed an interrupt, which implies that all int3 handlers previously
    > > > running should have ended. Is it right ? It looks to me as if this 3rd
    > > > sync_core_all() is only needed because of that. Probably that adding a
    > > > comment would be good.
    > > >
    > > >
    > > > Another thing: I've recently noticed that the following locking seems to
    > > > hang the system with doing stress-testing concurrently with cpu
    > > > hotplug/hotunplug:
    > > >
    > > > mutex_lock(&text_mutex);
    > > > on_each_cpu(something, NULL, 1);
    > > >
    > > > The hang seems to be caused by the fact that alternative.c has:
    > > >
    > > > within cpu hotplug (cpu hotplug lock held)
    > > > mutex_lock(&text_mutex);
    > > >
    > > > It might also be caused by the interaction with the stop_machine()
    > > > performed within the cpu hotplug lock. I did not find the root cause of
    > > > the problem, but this probably calls for lockdep improvements.
    > > >
    > > > In any case, given you are dealing with the exact same locking scenario
    > > > here, I would recomment the following stress-test:
    > > >
    > > > in a loop, use text_poke_jump()
    > > > in a loop, hotunplug all cpus, then hotplug all cpus
    > > >
    > > > I had to fix this temporarily by taking get/put_online_cpus() about the
    > > > text_mutex.
    > > >
    > > > [snip snip]
    > > >
    > > > > +static struct notifier_block patch_exceptions_nb = {
    > > > > + .notifier_call = patch_exceptions_notify,
    > > > > + .priority = 0x7fffffff /* we need to be notified first */
    > > > > +};
    > > > > +
    > > > > +static int __init patch_init(void)
    > > > > +{
    > > > > + return register_die_notifier(&patch_exceptions_nb);
    > > > > +}
    > > > > +
    > > > > +arch_initcall(patch_init);
    > > > > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
    > > > > index e5342a3..43a30d8 100644
    > > > > --- a/kernel/kprobes.c
    > > > > +++ b/kernel/kprobes.c
    > > > > @@ -898,7 +898,7 @@ EXPORT_SYMBOL_GPL(unregister_kprobes);
    > > > >
    > > > > static struct notifier_block kprobe_exceptions_nb = {
    > > > > .notifier_call = kprobe_exceptions_notify,
    > > > > - .priority = 0x7fffffff /* we need to be notified first */
    > > > > + .priority = 0x7ffffff0 /* High priority, but not first. */
    > > >
    > > > It would be good to keep all these priorities in a centralized header.
    > > >
    > > > Thanks,
    > > >
    > > > Mathieu
    > > >
    > > > > };
    > > > >
    > > > > unsigned long __weak arch_deref_entry_point(void *entry)
    > > > > --
    > > > > 1.6.5.1
    > > > >
    > > >
    > > > --
    > > > Mathieu Desnoyers
    > > > OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
    >
    > --
    > Mathieu Desnoyers
    > OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F BA06 3F25 A8FE 3BAE 9A68
    > --
    > 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: 2009-11-19 05:19    [W:0.067 / U:0.596 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site