Skip to content
  • Petr Mladek's avatar
    ftrace/x86: Remove possible deadlock between register_kprobe() and ftrace_run_update_code() · 0c0b5477
    Petr Mladek authored
    commit d5b844a2 upstream.
    
    The commit 9f255b63 ("module: Fix livepatch/ftrace module text
    permissions race") causes a possible deadlock between register_kprobe()
    and ftrace_run_update_code() when ftrace is using stop_machine().
    
    The existing dependency chain (in reverse order) is:
    
    -> #1 (text_mutex){+.+.}:
           validate_chain.isra.21+0xb32/0xd70
           __lock_acquire+0x4b8/0x928
           lock_acquire+0x102/0x230
           __mutex_lock+0x88/0x908
           mutex_lock_nested+0x32/0x40
           register_kprobe+0x254/0x658
           init_kprobes+0x11a/0x168
           do_one_initcall+0x70/0x318
           kernel_init_freeable+0x456/0x508
           kernel_init+0x22/0x150
           ret_from_fork+0x30/0x34
           kernel_thread_starter+0x0/0xc
    
    -> #0 (cpu_hotplug_lock.rw_sem){++++}:
           check_prev_add+0x90c/0xde0
           validate_chain.isra.21+0xb32/0xd70
           __lock_acquire+0x4b8/0x928
           lock_acquire+0x102/0x230
           cpus_read_lock+0x62/0xd0
           stop_machine+0x2e/0x60
           arch_ftrace_update_code+0x2e/0x40
           ftrace_run_update_code+0x40/0xa0
           ftrace_startup+0xb2/0x168
           register_ftrace_function+0x64/0x88
           klp_patch_object+0x1a2/0x290
           klp_enable_patch+0x554/0x980
           do_one_initcall+0x70/0x318
           do_init_module+0x6e/0x250
           load_module+0x1782/0x1990
           __s390x_sys_finit_module+0xaa/0xf0
           system_call+0xd8/0x2d0
    
     Possible unsafe locking scenario:
    
           CPU0                    CPU1
           ----                    ----
      lock(text_mutex);
                                   lock(cpu_hotplug_lock.rw_sem);
                                   lock(text_mutex);
      lock(cpu_hotplug_lock.rw_sem);
    
    It is similar problem that has been solved by the commit 2d1e38f5
    ("kprobes: Cure hotplug lock ordering issues"). Many locks are involved.
    To be on the safe side, text_mutex must become a low level lock taken
    after cpu_hotplug_lock.rw_sem.
    
    This can't be achieved easily with the current ftrace design.
    For example, arm calls set_all_modules_text_rw() already in
    ftrace_arch_code_modify_prepare(), see arch/arm/kernel/ftrace.c.
    This functions is called:
    
      + outside stop_machine() from ftrace_run_update_code()
      + without stop_machine() from ftrace_module_enable()
    
    Fortunately, the problematic fix is needed only on x86_64. It is
    the only architecture that calls set_all_modules_text_rw()
    in ftrace path and supports livepatching at the same time.
    
    Therefore it is enough to move text_mutex handling from the generic
    kernel/trace/ftrace.c into arch/x86/kernel/ftrace.c:
    
       ftrace_arch_code_modify_prepare()
       ftrace_arch_code_modify_post_process()
    
    This patch basically reverts the ftrace part of the problematic
    commit 9f255b63 ("module: Fix livepatch/ftrace module
    text permissions race"). And provides x86_64 specific-fix.
    
    Some refactoring of the ftrace code will be needed when livepatching
    is implemented for arm or nds32. These architectures call
    set_all_modules_text_rw() and use stop_machine() at the same time.
    
    Link: http://lkml.kernel.org/r/20190627081334.12793-1-pmladek@suse.com
    
    Fixes: 9f255b63
    
     ("module: Fix livepatch/ftrace module text permissions race")
    Acked-by: default avatarThomas Gleixner <tglx@linutronix.de>
    Reported-by: default avatarMiroslav Benes <mbenes@suse.cz>
    Reviewed-by: default avatarMiroslav Benes <mbenes@suse.cz>
    Reviewed-by: default avatarJosh Poimboeuf <jpoimboe@redhat.com>
    Signed-off-by: default avatarPetr Mladek <pmladek@suse.com>
    [
      As reviewed by Miroslav Benes <mbenes@suse.cz>, removed return value of
      ftrace_run_update_code() as it is a void function.
    ]
    Signed-off-by: default avatarSteven Rostedt (VMware) <rostedt@goodmis.org>
    Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
    0c0b5477