* [PATCH] ftrace: Fixup lockdep assert held of text_mutex @ 2020-08-06 14:50 guoren 2020-08-06 15:48 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: guoren @ 2020-08-06 14:50 UTC (permalink / raw) To: guoren, rostedt, mingo; +Cc: linux-kernel, linux-csky, Guo Ren From: Guo Ren <guoren@linux.alibaba.com> 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(). 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@linux.alibaba.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@redhat.com> --- 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) 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); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex 2020-08-06 14:50 [PATCH] ftrace: Fixup lockdep assert held of text_mutex guoren @ 2020-08-06 15:48 ` Steven Rostedt 2020-08-07 2:59 ` Guo Ren 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-08-06 15:48 UTC (permalink / raw) To: guoren; +Cc: mingo, linux-kernel, linux-csky, Guo Ren On Thu, 6 Aug 2020 14:50:54 +0000 guoren@kernel.org wrote: > From: Guo Ren <guoren@linux.alibaba.com> > > 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. > > 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@linux.alibaba.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ingo Molnar <mingo@redhat.com> > --- > 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. -- 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); ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex 2020-08-06 15:48 ` Steven Rostedt @ 2020-08-07 2:59 ` Guo Ren 2020-08-07 4:01 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Guo Ren @ 2020-08-07 2:59 UTC (permalink / raw) To: Steven Rostedt, Palmer Dabbelt, Paul Walmsley Cc: Ingo Molnar, Linux Kernel Mailing List, linux-csky, Guo Ren, linux-riscv On Thu, Aug 6, 2020 at 11:48 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Thu, 6 Aug 2020 14:50:54 +0000 > guoren@kernel.org wrote: > > > From: Guo Ren <guoren@linux.alibaba.com> > > > > 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@linux.alibaba.com> > > Cc: Steven Rostedt <rostedt@goodmis.org> > > Cc: Ingo Molnar <mingo@redhat.com> > > --- > > 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/ ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex 2020-08-07 2:59 ` Guo Ren @ 2020-08-07 4:01 ` Steven Rostedt 2020-08-07 5:01 ` Guo Ren 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-08-07 4:01 UTC (permalink / raw) To: Guo Ren Cc: Palmer Dabbelt, Paul Walmsley, Ingo Molnar, Linux Kernel Mailing List, linux-csky, Guo Ren, linux-riscv On Fri, 7 Aug 2020 10:59:16 +0800 Guo Ren <guoren@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. > + 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex 2020-08-07 4:01 ` Steven Rostedt @ 2020-08-07 5:01 ` Guo Ren 2020-08-13 5:13 ` Palmer Dabbelt 0 siblings, 1 reply; 9+ messages in thread From: Guo Ren @ 2020-08-07 5:01 UTC (permalink / raw) To: Steven Rostedt Cc: Palmer Dabbelt, Paul Walmsley, Ingo Molnar, Linux Kernel Mailing List, linux-csky, Guo Ren, linux-riscv On Fri, Aug 7, 2020 at 12:01 PM Steven Rostedt <rostedt@goodmis.org> wrote: > > On Fri, 7 Aug 2020 10:59:16 +0800 > Guo Ren <guoren@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/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex 2020-08-07 5:01 ` Guo Ren @ 2020-08-13 5:13 ` Palmer Dabbelt 2020-08-13 15:37 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Palmer Dabbelt @ 2020-08-13 5:13 UTC (permalink / raw) To: guoren Cc: rostedt, Paul Walmsley, mingo, linux-kernel, linux-csky, guoren, linux-riscv On Thu, 06 Aug 2020 22:01:01 PDT (-0700), guoren@kernel.org wrote: > On Fri, Aug 7, 2020 at 12:01 PM Steven Rostedt <rostedt@goodmis.org> wrote: >> >> On Fri, 7 Aug 2020 10:59:16 +0800 >> Guo Ren <guoren@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 Sorry, I'm not really sure what's going on here. I'm not really seeing code that matches this in our port right now, so maybe this is aginst some other tree? If it's the RISC-V kprobes patch set then I was hoping to take a look at that tomorrow (or I guess a bit earlier this week, but I had some surprise work stuff to do). IIRC there were a handful of races in the last patch set I saw, but it's been a while so I don't remember for sure. That said, I certainly wouldn't be surprised if there's a locking bug in our ftrace stuff. It'd be way easier for me to figure out what's going on if you have a concrete suggestion as to how to fix the issues -- even if it's just a workaround. > >> >> > + 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex 2020-08-13 5:13 ` Palmer Dabbelt @ 2020-08-13 15:37 ` Steven Rostedt 2020-08-25 0:29 ` Palmer Dabbelt 0 siblings, 1 reply; 9+ messages in thread From: Steven Rostedt @ 2020-08-13 15:37 UTC (permalink / raw) To: Palmer Dabbelt Cc: guoren, Paul Walmsley, mingo, linux-kernel, linux-csky, guoren, linux-riscv On Wed, 12 Aug 2020 22:13:19 -0700 (PDT) Palmer Dabbelt <palmer@dabbelt.com> wrote: > Sorry, I'm not really sure what's going on here. I'm not really seeing code > that matches this in our port right now, so maybe this is aginst some other > tree? If it's the RISC-V kprobes patch set then I was hoping to take a look at > that tomorrow (or I guess a bit earlier this week, but I had some surprise work > stuff to do). IIRC there were a handful of races in the last patch set I saw, > but it's been a while so I don't remember for sure. > > That said, I certainly wouldn't be surprised if there's a locking bug in our > ftrace stuff. It'd be way easier for me to figure out what's going on if you > have a concrete suggestion as to how to fix the issues -- even if it's just a > workaround. The issue is actually quite basic. ftrace_init_nop() is called quite early in boot up and never called again. It's called before SMP is set up, so it's on a single CPU, and no worries about synchronization with other CPUs is needed. On x86, it is called before text_poke() is initialized (which is used to synchronize code updates across CPUs), and thus can't be called. There's a "text_poke_early()" that is used instead, which is basically just a memcpy(). Now, if ftrace_init_nop() is not defined by the architecture, it is a simple call to ftrace_make_nop(), which is also used to disable ftrace callbacks. The issue is that we have the following path on riscv: ftrace_init_nop() ftrace_make_nop() __ftrace_modify_call() patch_text_nosync() patch_insn_write() lockdep_assert_held(&text_mutex); Boom! text_mutex is not held, and lockdep complains. The difference between ftrace_make_nop() being called by ftrace_init_nop() and being called later to disable function tracing is that the latter will have: ftrace_arch_code_modify_prepare(); [..] ftrace_make_nop(); [..] ftrace_arch_code_modify_post_process(); and the former will not have those called. On x86, we handle the two different cases with: static int ftrace_poke_late = 0; int ftrace_arch_code_modify_prepare(void) { mutex_lock(&text_mutex); ftrace_poke_late = 1; return 0; } int ftrace_arch_code_modify_post_process(void) { text_poke_finish(); ftrace_poke_late = 0; mutex_unlock(&text_mutex); } Although, the post_process() probably doesn't even need to set ftrace_poke_late back to zero. Then in ftrace_make_nop(), we have: ftrace_make_nop() ftrace_modify_code_direct() if (ftrace_poke_late) text_poke_queue(...); // this checks if text_mutex is held else text_poke_early(...); // is basically just memcpy, no test on text_mutex. The two solutions for riscv, is either to implement the same thing as above, or you can create your own ftrace_init_nop() to take the text_mutex before calling ftrace_make_nop(), and that should work too. -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex 2020-08-13 15:37 ` Steven Rostedt @ 2020-08-25 0:29 ` Palmer Dabbelt 2020-08-26 18:53 ` Steven Rostedt 0 siblings, 1 reply; 9+ messages in thread From: Palmer Dabbelt @ 2020-08-25 0:29 UTC (permalink / raw) To: rostedt Cc: guoren, Paul Walmsley, mingo, linux-kernel, linux-csky, guoren, linux-riscv On Thu, 13 Aug 2020 08:37:43 PDT (-0700), rostedt@goodmis.org wrote: > On Wed, 12 Aug 2020 22:13:19 -0700 (PDT) > Palmer Dabbelt <palmer@dabbelt.com> wrote: > >> Sorry, I'm not really sure what's going on here. I'm not really seeing code >> that matches this in our port right now, so maybe this is aginst some other >> tree? If it's the RISC-V kprobes patch set then I was hoping to take a look at >> that tomorrow (or I guess a bit earlier this week, but I had some surprise work >> stuff to do). IIRC there were a handful of races in the last patch set I saw, >> but it's been a while so I don't remember for sure. >> >> That said, I certainly wouldn't be surprised if there's a locking bug in our >> ftrace stuff. It'd be way easier for me to figure out what's going on if you >> have a concrete suggestion as to how to fix the issues -- even if it's just a >> workaround. > > The issue is actually quite basic. > > ftrace_init_nop() is called quite early in boot up and never called > again. It's called before SMP is set up, so it's on a single CPU, and > no worries about synchronization with other CPUs is needed. > > On x86, it is called before text_poke() is initialized (which is used > to synchronize code updates across CPUs), and thus can't be called. > There's a "text_poke_early()" that is used instead, which is basically > just a memcpy(). > > Now, if ftrace_init_nop() is not defined by the architecture, it is a > simple call to ftrace_make_nop(), which is also used to disable ftrace > callbacks. > > The issue is that we have the following path on riscv: > > ftrace_init_nop() > ftrace_make_nop() > __ftrace_modify_call() > patch_text_nosync() > patch_insn_write() > lockdep_assert_held(&text_mutex); > > Boom! text_mutex is not held, and lockdep complains. > > > The difference between ftrace_make_nop() being called by > ftrace_init_nop() and being called later to disable function tracing is > that the latter will have: > > > ftrace_arch_code_modify_prepare(); > [..] > ftrace_make_nop(); > [..] > ftrace_arch_code_modify_post_process(); > > and the former will not have those called. > > On x86, we handle the two different cases with: > > > static int ftrace_poke_late = 0; > > int ftrace_arch_code_modify_prepare(void) > { > mutex_lock(&text_mutex); > ftrace_poke_late = 1; > return 0; > } > > int ftrace_arch_code_modify_post_process(void) > { > text_poke_finish(); > ftrace_poke_late = 0; > mutex_unlock(&text_mutex); > } > > Although, the post_process() probably doesn't even need to set > ftrace_poke_late back to zero. > > Then in ftrace_make_nop(), we have: > > ftrace_make_nop() > ftrace_modify_code_direct() > if (ftrace_poke_late) > text_poke_queue(...); // this checks if text_mutex is held > else > text_poke_early(...); // is basically just memcpy, no test on text_mutex. > > The two solutions for riscv, is either to implement the same thing as > above, or you can create your own ftrace_init_nop() to take the > text_mutex before calling ftrace_make_nop(), and that should work too. Ya, thanks, that's a pretty straight-forward issue. I've To'd you on a patch, but it's essentially just exactly what you suggested so I doubt it's that interesting. I pointed out in the patch notes that it seems reasonable to have the generic code handle this case, would you be opposed to doing it that way? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex 2020-08-25 0:29 ` Palmer Dabbelt @ 2020-08-26 18:53 ` Steven Rostedt 0 siblings, 0 replies; 9+ messages in thread From: Steven Rostedt @ 2020-08-26 18:53 UTC (permalink / raw) To: Palmer Dabbelt Cc: guoren, Paul Walmsley, mingo, linux-kernel, linux-csky, guoren, linux-riscv On Mon, 24 Aug 2020 17:29:32 -0700 (PDT) Palmer Dabbelt <palmer@dabbelt.com> wrote: > I pointed out in the patch notes that it seems reasonable to have the generic > code handle this case, would you be opposed to doing it that way? For now, lets hold off and see if other archs start to do it the same way. Then we should look into making it more generic. It's an easy enough fix per arch, so there's no urgency to this. -- Steve ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-08-26 18:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-08-06 14:50 [PATCH] ftrace: Fixup lockdep assert held of text_mutex guoren 2020-08-06 15:48 ` Steven Rostedt 2020-08-07 2:59 ` Guo Ren 2020-08-07 4:01 ` Steven Rostedt 2020-08-07 5:01 ` Guo Ren 2020-08-13 5:13 ` Palmer Dabbelt 2020-08-13 15:37 ` Steven Rostedt 2020-08-25 0:29 ` Palmer Dabbelt 2020-08-26 18:53 ` Steven Rostedt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).