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;
MIME-Version: 1.0
References: <cover.1684120990.git.zegao@tencent.com> <6c428b9f74090b2780b67ca498c45aa5e8c38c44.1684120990.git.zegao@tencent.com>
 <20230515212546.7a467a1f@gandalf.local.home>
In-Reply-To: <20230515212546.7a467a1f@gandalf.local.home>
From:   Ze Gao <zegao2021@gmail.com>
Date:   Tue, 16 May 2023 11:02:27 +0800
Message-ID: <CAD8CoPC-q23cytkzdPwBVAXKBS-B-Dpya3dWroftvQD6iN414g@mail.gmail.com>
Subject: Re: [PATCH 2/4] fprobe: make fprobe_kprobe_handler recursion free
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     Masami Hiramatsu <mhiramat@kernel.org>, Ze Gao <zegao@tencent.com>,
        linux-kernel@vger.kernel.org, linux-trace-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Steven,

On Tue, May 16, 2023 at 9:25=E2=80=AFAM Steven Rostedt <rostedt@goodmis.org=
> wrote:
>
> On Mon, 15 May 2023 11:26:39 +0800
> Ze Gao <zegao2021@gmail.com> wrote:
>
> > Current implementation calls kprobe related functions before doing
> > ftrace recursion check in fprobe_kprobe_handler, which opens door
> > to kernel crash due to stack recursion if preempt_count_{add, sub}
> > is traceable.
> >
> > Refactor the common part out of fprobe_kprobe_handler and fprobe_
> > handler and call ftrace recursion detection at the very beginning,
> > and also mark these functions notrace so that the whole fprobe_k-
> > probe_handler is free from recusion. And
> >
> > Signed-off-by: Ze Gao <zegao@tencent.com>
> > ---
> >  kernel/trace/fprobe.c | 61 +++++++++++++++++++++++++++++++------------
> >  1 file changed, 45 insertions(+), 16 deletions(-)
> >
> > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c
> > index 9abb3905bc8e..ad9a36c87ad9 100644
> > --- a/kernel/trace/fprobe.c
> > +++ b/kernel/trace/fprobe.c
> > @@ -20,30 +20,22 @@ struct fprobe_rethook_node {
> >       char data[];
> >  };
> >
> > -static void fprobe_handler(unsigned long ip, unsigned long parent_ip,
> > -                        struct ftrace_ops *ops, struct ftrace_regs *fr=
egs)
> > +static inline notrace void __fprobe_handler(unsigned long ip, unsigned=
 long
>
>
> FYI, if you look in kernel/trace/Makefile you'll see:
>
> ccflags-remove-$(CONFIG_FUNCTION_TRACER) +=3D $(CC_FLAGS_FTRACE)
>
> Which removes the flags to add tracing. So there's no reason to add
> "notrace" here, as all functions in this directory are by default "notrac=
e".

Thanks for your valuable info, which I missed before. I'll send v2 to
remove those
unnecessary notrace annotations, and use the same trick for rethook too.

BTW,  I think we can mark rethook routines decls notrace in
include/linux/rethook.h,
which helps to remind developers of other arch(s) this important info.
What do you think of it?

Regards,
Ze