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
