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

[Palmer Dabbelt <palmer@dabbelt.com>]

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.