linux-kernel - Re: [PATCH 2/4] fprobe: make fprobe_kprobe

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]

Date:   Tue, 16 May 2023 11:02:27 +0800
From:   Ze Gao <zegao2021@...il.com>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     Masami Hiramatsu <mhiramat@...nel.org>, Ze Gao <zegao@...cent.com>,
        linux-kernel@...r.kernel.org, linux-trace-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] fprobe: make fprobe_kprobe_handler recursion free

Hi Steven,

On Tue, May 16, 2023 at 9:25 AM Steven Rostedt <rostedt@...dmis.org> wrote:
>
> On Mon, 15 May 2023 11:26:39 +0800
> Ze Gao <zegao2021@...il.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@...cent.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 *fregs)
> > +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) += $(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 "notrace".

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