Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jiri Olsa <olsajiri@gmail.com>
Date:   Tue, 17 May 2022 09:49:33 +0200
To:     Yonghong Song <yhs@fb.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        "Paul E. McKenney" <paulmck@kernel.org>, netdev@vger.kernel.org,
        bpf@vger.kernel.org, lkml <linux-kernel@vger.kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [PATCH bpf-next 1/2] cpuidle/rcu: Making arch_cpu_idle and
 rcu_idle_exit noinstr
Message-ID: <YoNTjXBDLQe9xj27@krava>
References: <20220515203653.4039075-1-jolsa@kernel.org>
 <5b4bd044-ba88-649b-9b85-e08e175691f9@fb.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <5b4bd044-ba88-649b-9b85-e08e175691f9@fb.com>
Precedence: bulk

On Mon, May 16, 2022 at 07:54:37PM -0700, Yonghong Song wrote:
> 
> 
> On 5/15/22 1:36 PM, Jiri Olsa wrote:
> > Making arch_cpu_idle and rcu_idle_exit noinstr. Both functions run
> > in rcu 'not watching' context and if there's tracer attached to
> > them, which uses rcu (e.g. kprobe multi interface) it will hit RCU
> > warning like:
> > 
> >    [    3.017540] WARNING: suspicious RCU usage
> >    ...
> >    [    3.018363]  kprobe_multi_link_handler+0x68/0x1c0
> >    [    3.018364]  ? kprobe_multi_link_handler+0x3e/0x1c0
> >    [    3.018366]  ? arch_cpu_idle_dead+0x10/0x10
> >    [    3.018367]  ? arch_cpu_idle_dead+0x10/0x10
> >    [    3.018371]  fprobe_handler.part.0+0xab/0x150
> >    [    3.018374]  0xffffffffa00080c8
> >    [    3.018393]  ? arch_cpu_idle+0x5/0x10
> >    [    3.018398]  arch_cpu_idle+0x5/0x10
> >    [    3.018399]  default_idle_call+0x59/0x90
> >    [    3.018401]  do_idle+0x1c3/0x1d0
> > 
> > The call path is following:
> > 
> > default_idle_call
> >    rcu_idle_enter
> >    arch_cpu_idle
> >    rcu_idle_exit
> > 
> > The arch_cpu_idle and rcu_idle_exit are the only ones from above
> > path that are traceble and cause this problem on my setup.
> > 
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >   arch/x86/kernel/process.c | 2 +-
> >   kernel/rcu/tree.c         | 2 +-
> >   2 files changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index b370767f5b19..1345cb0124a6 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -720,7 +720,7 @@ void arch_cpu_idle_dead(void)
> >   /*
> >    * Called from the generic idle code.
> >    */
> > -void arch_cpu_idle(void)
> > +void noinstr arch_cpu_idle(void)
> 
> noinstr includes a lot of attributes:
> 
> #define noinstr                                                         \
>         noinline notrace __attribute((__section__(".noinstr.text")))    \
>         __no_kcsan __no_sanitize_address __no_profile __no_sanitize_coverage
> 
> should we use notrace here?

hm right, so notrace should be enough for our case (kprobe_multi)
which is based on ftrace/fprobe jump

noinstr (among other things) adds the function also the kprobes
blacklist, which will prevent standard kprobes to attach

ASAICS standard kprobes use rcu in probe path as well, like in
opt_pre_handler function

so I think we should go with noinstr

jirka

> 
> >   {
> >   	x86_idle();
> >   }
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index a4b8189455d5..20d529722f51 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -896,7 +896,7 @@ static void noinstr rcu_eqs_exit(bool user)
> >    * If you add or remove a call to rcu_idle_exit(), be sure to test with
> >    * CONFIG_RCU_EQS_DEBUG=y.
> >    */
> > -void rcu_idle_exit(void)
> > +void noinstr rcu_idle_exit(void)
> >   {
> >   	unsigned long flags;