linux-kernel - Re: [PATCH] ftrace: Fixup lockdep assert held of text

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJF2gTQjYyNnhg8KhFEm6MwOCb=c0hNsSq=HOeuSCrOzR9Qf0Q@mail.gmail.com>
Date:   Fri, 7 Aug 2020 13:01:01 +0800
From:   Guo Ren <guoren@...nel.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Ingo Molnar <mingo@...hat.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-csky@...r.kernel.org, Guo Ren <guoren@...ux.alibaba.com>,
        linux-riscv <linux-riscv@...ts.infradead.org>
Subject: Re: [PATCH] ftrace: Fixup lockdep assert held of text_mutex

On Fri, Aug 7, 2020 at 12:01 PM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Fri, 7 Aug 2020 10:59:16 +0800
> Guo Ren <guoren@...nel.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/