Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex

[Steven Rostedt]

On Fri, 7 Aug 2020 10:59:16 +0800
Guo Ren <guoren@xxxxxxxxxx> wrote:
> >
> > This looks like a bug in the lockdep_assert_held() in whatever arch
> > (riscv) is running.  
> Seems you think it's a bug of arch implementation with the wrong usage
> of text_mutex?
> 
> Also @riscv maintainer,
> How about modifying it in riscv's code? we still need to solve it.
> 
> ----------------
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index ace8a6e..fb266c3 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -23,6 +23,12 @@ static inline unsigned long
> ftrace_call_adjust(unsigned long addr)
> 
>  struct dyn_arch_ftrace {
>  };
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE
> +struct dyn_ftrace;
> +int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> +#define ftrace_init_nop ftrace_init_nop
> +#endif
>  #endif
> 
>  #ifdef CONFIG_DYNAMIC_FTRACE
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index 2ff63d0..9e9f7c0 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -97,6 +97,17 @@ int ftrace_make_nop(struct module *mod, struct
> dyn_ftrace *rec,
>         return __ftrace_modify_call(rec->ip, addr, false);
>  }
> 
> +int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> +       int ret;
> +
> +       mutex_lock(&text_mutex);
> +       ret = ftrace_make_nop(mod, rec, MCOUNT_ADDR);

Looking at x86, we have the following code:

static int ftrace_poke_late = 0;

int ftrace_arch_code_modify_prepare(void)
    __acquires(&text_mutex)
{
	/*
	 * Need to grab text_mutex to prevent a race from module loading
	 * and live kernel patching from changing the text permissions while
	 * ftrace has it set to "read/write".
	 */
	mutex_lock(&text_mutex);
	ftrace_poke_late = 1;
	return 0;
}

int ftrace_arch_code_modify_post_process(void)
    __releases(&text_mutex)
{
	/*
	 * ftrace_make_{call,nop}() may be called during
	 * module load, and we need to finish the text_poke_queue()
	 * that they do, here.
	 */
	text_poke_finish();
	ftrace_poke_late = 0;
	mutex_unlock(&text_mutex);
	return 0;
}

And if ftrace_poke_late is not set, then ftrace_make_nop() does direct
modification (calls text_poke_early(), which is basically a memcpy).

This path doesn't have any checks against text_mutex being held,
because it only happens at boot up.

> +       mutex_unlock(&text_mutex);
> +
> +       return ret;
> +}
> +
>  int ftrace_update_ftrace_func(ftrace_func_t func)
>  {
>         int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
> -------------------
> 
> > > --- a/kernel/trace/ftrace.c
> > > +++ b/kernel/trace/ftrace.c
> > > @@ -26,6 +26,7 @@
> > >  #include <linux/uaccess.h>
> > >  #include <linux/bsearch.h>
> > >  #include <linux/module.h>
> > > +#include <linux/memory.h>
> > >  #include <linux/ftrace.h>
> > >  #include <linux/sysctl.h>
> > >  #include <linux/slab.h>
> > > @@ -6712,9 +6713,11 @@ void __init ftrace_init(void)  
> >
> > ftrace_init() is called before SMP is initialized. Nothing else should
> > be running here. That means grabbing a mutex is useless.  
> I don't agree, ftrace_init are modifying kernel text, so we should
> give the lock of text_mutex to keep semantic consistency.


Did you test your patch on x86 with lockdep?

ftrace_process_locs() grabs the ftrace_lock, which I believe is held
when text_mutex is taken in other locations. So this will probably not
work anyway.

text_mutex isn't to be taken at the ftrace level.

-- Steve