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

[Guo Ren]

On Thu, Aug 6, 2020 at 11:48 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> On Thu,  6 Aug 2020 14:50:54 +0000
> guoren@xxxxxxxxxx wrote:
>
> > From: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> >
> > The function ftrace_process_locs() will modify text code, so we
> > should give a text_mutex lock. Because some arch's patch code
> > will assert held of text_mutex even during start_kernel->
> > ftrace_init().
>
> NAK.
>
> 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);
+       mutex_unlock(&text_mutex);
+
+       return ret;
+}
+
 int ftrace_update_ftrace_func(ftrace_func_t func)
 {
        int ret = __ftrace_modify_call((unsigned long)&ftrace_call,
-------------------

>
> >
> > backtrace log:
> >    assert by lockdep_assert_held(&text_mutex)
> > 0  patch_insn_write (addr=0xffffffe0000010fc <set_reset_devices+10>, insn=0xffffffe001203eb8, len=8) at arch/riscv/kernel/patch.c:63
> > 1  0xffffffe0002042ec in patch_text_nosync (addr=<optimized out>, insns=<optimized out>, len=<optimized out>) at arch/riscv/kernel/patch.c:93
> > 2  0xffffffe00020628e in __ftrace_modify_call (hook_pos=<optimized out>, target=<optimized out>, enable=<optimized out>) at arch/riscv/kernel/ftrace.c:68
> > 3  0xffffffe0002063c0 in ftrace_make_nop (mod=<optimized out>, rec=0xffffffe001221c70 <text_mutex+96>, addr=18446743936272720288) at arch/riscv/kernel/ftrace.c:97
> > 4  0xffffffe0002b13f0 in ftrace_init_nop (rec=<optimized out>, mod=<optimized out>) at ./include/linux/ftrace.h:647
> > 5  ftrace_nop_initialize (rec=<optimized out>, mod=<optimized out>) at kernel/trace/ftrace.c:2619
> > 6  ftrace_update_code (new_pgs=<optimized out>, mod=<optimized out>) at kernel/trace/ftrace.c:3063
> > 7  ftrace_process_locs (mod=<optimized out>, start=<optimized out>, end=<optimized out>) at kernel/trace/ftrace.c:6154
> > 8  0xffffffe00000b6e6 in ftrace_init () at kernel/trace/ftrace.c:6715
> > 9  0xffffffe000001b48 in start_kernel () at init/main.c:888
> > 10 0xffffffe0000010a8 in _start_kernel () at arch/riscv/kernel/head.S:247
> >
> > Signed-off-by: Guo Ren <guoren@xxxxxxxxxxxxxxxxx>
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> > ---
> >  kernel/trace/ftrace.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> > index 1903b80..4b48b88 100644
> > --- 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.


>
> -- Steve
>
>
> >
> >       last_ftrace_enabled = ftrace_enabled = 1;
> >
> > +     mutex_lock(&text_mutex);
> >       ret = ftrace_process_locs(NULL,
> >                                 __start_mcount_loc,
> >                                 __stop_mcount_loc);
> > +     mutex_unlock(&text_mutex);
> >
> >       pr_info("ftrace: allocated %ld pages with %ld groups\n",
> >               ftrace_number_of_pages, ftrace_number_of_groups);
>


-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/