On Fri, Aug 7, 2020 at 12:01 PM Steven Rostedt <rost...@goodmis.org> wrote:
>
> On Fri, 7 Aug 2020 10:59:16 +0800
> Guo Ren <guo...@kernel.org> 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.
The solution is ok for me, but I want to get riscv maintainer's
opinion before the next patch.
@Paul Walmsley
@Palmer Dabbelt
>
> > + 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?
Ah.., no :P
>
> 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.
Yes, currently it seemed only to be used by kernel/kprobes.c.
--
Best Regards
Guo Ren
ML: https://lore.kernel.org/linux-csky/
