From: Yafang Shao <laoar.shao@gmail.com> To: Ze Gao <zegao2021@gmail.com> Cc: Steven Rostedt <rostedt@goodmis.org>, Masami Hiramatsu <mhiramat@kernel.org>, Albert Ou <aou@eecs.berkeley.edu>, Alexander Gordeev <agordeev@linux.ibm.com>, Alexei Starovoitov <ast@kernel.org>, Borislav Petkov <bp@alien8.de>, Christian Borntraeger <borntraeger@linux.ibm.com>, Dave Hansen <dave.hansen@linux.intel.com>, Heiko Carstens <hca@linux.ibm.com>, "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>, Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Sven Schnelle <svens@linux.ibm.com>, Thomas Gleixner <tglx@linutronix.de>, Vasily Gorbik <gor@linux.ibm.com>, x86@kernel.org, bpf@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, linux-s390@vger.kernel.org, linux-trace-kernel@vger.kernel.org, Conor Dooley <conor@kernel.org>, Jiri Olsa <jolsa@kernel.org>, Yonghong Song <yhs@fb.com>, Ze Gao <zegao@tencent.com> Subject: Re: [PATCH v3 2/4] fprobe: make fprobe_kprobe_handler recursion free Date: Wed, 28 Jun 2023 15:16:47 +0800 [thread overview] Message-ID: <CALOAHbC6UpfFOOibdDiC7xFc5YFUgZnk3MZ=3Ny6we=AcrNbew@mail.gmail.com> (raw) In-Reply-To: <20230517034510.15639-3-zegao@tencent.com> On Wed, May 17, 2023 at 11:45 AM 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 in kprobe_busy_{begin, end}. > > Things goes like this without this patch quoted from Steven: > " > fprobe_kprobe_handler() { > kprobe_busy_begin() { > preempt_disable() { > preempt_count_add() { <-- trace > fprobe_kprobe_handler() { > [ wash, rinse, repeat, CRASH!!! ] > " > > By refactoring the common part out of fprobe_kprobe_handler and > fprobe_handler and call ftrace recursion detection at the very beginning, > the whole fprobe_kprobe_handler is free from recursion. > > Signed-off-by: Ze Gao <zegao@tencent.com> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Link: https://lore.kernel.org/linux-trace-kernel/20230516071830.8190-3-zegao@tencent.com > --- > kernel/trace/fprobe.c | 59 ++++++++++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 15 deletions(-) > > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c > index 9abb3905bc8e..097c740799ba 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 void __fprobe_handler(unsigned long ip, unsigned long > + parent_ip, struct ftrace_ops *ops, struct ftrace_regs *fregs) > { > struct fprobe_rethook_node *fpr; > struct rethook_node *rh = NULL; > struct fprobe *fp; > void *entry_data = NULL; > - int bit, ret; > + int ret; > > fp = container_of(ops, struct fprobe, ops); > - if (fprobe_disabled(fp)) > - return; > - > - bit = ftrace_test_recursion_trylock(ip, parent_ip); > - if (bit < 0) { > - fp->nmissed++; > - return; > - } > > if (fp->exit_handler) { > rh = rethook_try_get(fp->rethook); > if (!rh) { > fp->nmissed++; > - goto out; > + return; > } > fpr = container_of(rh, struct fprobe_rethook_node, node); > fpr->entry_ip = ip; > @@ -61,23 +53,60 @@ static void fprobe_handler(unsigned long ip, unsigned long parent_ip, > else > rethook_hook(rh, ftrace_get_regs(fregs), true); > } > -out: > +} > + > +static void fprobe_handler(unsigned long ip, unsigned long parent_ip, > + struct ftrace_ops *ops, struct ftrace_regs *fregs) > +{ > + struct fprobe *fp; > + int bit; > + > + fp = container_of(ops, struct fprobe, ops); > + if (fprobe_disabled(fp)) > + return; > + > + /* recursion detection has to go before any traceable function and > + * all functions before this point should be marked as notrace > + */ > + bit = ftrace_test_recursion_trylock(ip, parent_ip); > + if (bit < 0) { > + fp->nmissed++; > + return; > + } > + __fprobe_handler(ip, parent_ip, ops, fregs); > ftrace_test_recursion_unlock(bit); > + > } > NOKPROBE_SYMBOL(fprobe_handler); > > static void fprobe_kprobe_handler(unsigned long ip, unsigned long parent_ip, > struct ftrace_ops *ops, struct ftrace_regs *fregs) > { > - struct fprobe *fp = container_of(ops, struct fprobe, ops); > + struct fprobe *fp; > + int bit; > + > + fp = container_of(ops, struct fprobe, ops); > + if (fprobe_disabled(fp)) > + return; > + > + /* recursion detection has to go before any traceable function and > + * all functions called before this point should be marked as notrace > + */ > + bit = ftrace_test_recursion_trylock(ip, parent_ip); > + if (bit < 0) { > + fp->nmissed++; > + return; > + } > > if (unlikely(kprobe_running())) { > fp->nmissed++; I have just looked through this patchset, just out of curiosity, shouldn't we call ftrace_test_recursion_unlock(bit) here ? We have already locked it successfully, so why should we not unlock it? > return; > } > + > kprobe_busy_begin(); > - fprobe_handler(ip, parent_ip, ops, fregs); > + __fprobe_handler(ip, parent_ip, ops, fregs); > kprobe_busy_end(); > + ftrace_test_recursion_unlock(bit); > } > > static void fprobe_exit_handler(struct rethook_node *rh, void *data, > -- > 2.40.1 > > -- Regards Yafang _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
WARNING: multiple messages have this Message-ID (diff)
From: Yafang Shao <laoar.shao@gmail.com> To: Ze Gao <zegao2021@gmail.com> Cc: Steven Rostedt <rostedt@goodmis.org>, Masami Hiramatsu <mhiramat@kernel.org>, Albert Ou <aou@eecs.berkeley.edu>, Alexander Gordeev <agordeev@linux.ibm.com>, Alexei Starovoitov <ast@kernel.org>, Borislav Petkov <bp@alien8.de>, Christian Borntraeger <borntraeger@linux.ibm.com>, Dave Hansen <dave.hansen@linux.intel.com>, Heiko Carstens <hca@linux.ibm.com>, "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>, Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, Sven Schnelle <svens@linux.ibm.com>, Thomas Gleixner <tglx@linutronix.de>, Vasily Gorbik <gor@linux.ibm.com>, x86@kernel.org, bpf@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org, linux-s390@vger.kernel.org, linux-trace-kernel@vger.kernel.org, Conor Dooley <conor@kernel.org>, Jiri Olsa <jolsa@kernel.org>, Yonghong Song <yhs@fb.com>, Ze Gao <zegao@tencent.com> Subject: Re: [PATCH v3 2/4] fprobe: make fprobe_kprobe_handler recursion free Date: Wed, 28 Jun 2023 15:16:47 +0800 [thread overview] Message-ID: <CALOAHbC6UpfFOOibdDiC7xFc5YFUgZnk3MZ=3Ny6we=AcrNbew@mail.gmail.com> (raw) In-Reply-To: <20230517034510.15639-3-zegao@tencent.com> On Wed, May 17, 2023 at 11:45 AM 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 in kprobe_busy_{begin, end}. > > Things goes like this without this patch quoted from Steven: > " > fprobe_kprobe_handler() { > kprobe_busy_begin() { > preempt_disable() { > preempt_count_add() { <-- trace > fprobe_kprobe_handler() { > [ wash, rinse, repeat, CRASH!!! ] > " > > By refactoring the common part out of fprobe_kprobe_handler and > fprobe_handler and call ftrace recursion detection at the very beginning, > the whole fprobe_kprobe_handler is free from recursion. > > Signed-off-by: Ze Gao <zegao@tencent.com> > Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org> > Link: https://lore.kernel.org/linux-trace-kernel/20230516071830.8190-3-zegao@tencent.com > --- > kernel/trace/fprobe.c | 59 ++++++++++++++++++++++++++++++++----------- > 1 file changed, 44 insertions(+), 15 deletions(-) > > diff --git a/kernel/trace/fprobe.c b/kernel/trace/fprobe.c > index 9abb3905bc8e..097c740799ba 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 void __fprobe_handler(unsigned long ip, unsigned long > + parent_ip, struct ftrace_ops *ops, struct ftrace_regs *fregs) > { > struct fprobe_rethook_node *fpr; > struct rethook_node *rh = NULL; > struct fprobe *fp; > void *entry_data = NULL; > - int bit, ret; > + int ret; > > fp = container_of(ops, struct fprobe, ops); > - if (fprobe_disabled(fp)) > - return; > - > - bit = ftrace_test_recursion_trylock(ip, parent_ip); > - if (bit < 0) { > - fp->nmissed++; > - return; > - } > > if (fp->exit_handler) { > rh = rethook_try_get(fp->rethook); > if (!rh) { > fp->nmissed++; > - goto out; > + return; > } > fpr = container_of(rh, struct fprobe_rethook_node, node); > fpr->entry_ip = ip; > @@ -61,23 +53,60 @@ static void fprobe_handler(unsigned long ip, unsigned long parent_ip, > else > rethook_hook(rh, ftrace_get_regs(fregs), true); > } > -out: > +} > + > +static void fprobe_handler(unsigned long ip, unsigned long parent_ip, > + struct ftrace_ops *ops, struct ftrace_regs *fregs) > +{ > + struct fprobe *fp; > + int bit; > + > + fp = container_of(ops, struct fprobe, ops); > + if (fprobe_disabled(fp)) > + return; > + > + /* recursion detection has to go before any traceable function and > + * all functions before this point should be marked as notrace > + */ > + bit = ftrace_test_recursion_trylock(ip, parent_ip); > + if (bit < 0) { > + fp->nmissed++; > + return; > + } > + __fprobe_handler(ip, parent_ip, ops, fregs); > ftrace_test_recursion_unlock(bit); > + > } > NOKPROBE_SYMBOL(fprobe_handler); > > static void fprobe_kprobe_handler(unsigned long ip, unsigned long parent_ip, > struct ftrace_ops *ops, struct ftrace_regs *fregs) > { > - struct fprobe *fp = container_of(ops, struct fprobe, ops); > + struct fprobe *fp; > + int bit; > + > + fp = container_of(ops, struct fprobe, ops); > + if (fprobe_disabled(fp)) > + return; > + > + /* recursion detection has to go before any traceable function and > + * all functions called before this point should be marked as notrace > + */ > + bit = ftrace_test_recursion_trylock(ip, parent_ip); > + if (bit < 0) { > + fp->nmissed++; > + return; > + } > > if (unlikely(kprobe_running())) { > fp->nmissed++; I have just looked through this patchset, just out of curiosity, shouldn't we call ftrace_test_recursion_unlock(bit) here ? We have already locked it successfully, so why should we not unlock it? > return; > } > + > kprobe_busy_begin(); > - fprobe_handler(ip, parent_ip, ops, fregs); > + __fprobe_handler(ip, parent_ip, ops, fregs); > kprobe_busy_end(); > + ftrace_test_recursion_unlock(bit); > } > > static void fprobe_exit_handler(struct rethook_node *rh, void *data, > -- > 2.40.1 > > -- Regards Yafang
next prev parent reply other threads:[~2023-06-28 7:17 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-05-17 3:45 [PATCH v3 0/3] Make fprobe + rethook immune to recursion Ze Gao 2023-05-17 3:45 ` Ze Gao 2023-05-17 3:45 ` [PATCH v3 1/4] rethook: use preempt_{disable, enable}_notrace in rethook_trampoline_handler Ze Gao 2023-05-17 3:45 ` Ze Gao 2023-05-17 11:59 ` Masami Hiramatsu 2023-05-17 11:59 ` Masami Hiramatsu 2023-05-18 2:40 ` Ze Gao 2023-05-18 2:40 ` Ze Gao 2023-05-17 3:45 ` [PATCH v3 2/4] fprobe: make fprobe_kprobe_handler recursion free Ze Gao 2023-05-17 3:45 ` Ze Gao 2023-05-17 10:47 ` Jiri Olsa 2023-05-17 10:47 ` Jiri Olsa 2023-05-17 11:42 ` Masami Hiramatsu 2023-05-17 11:42 ` Masami Hiramatsu 2023-05-17 12:30 ` Jiri Olsa 2023-05-17 12:30 ` Jiri Olsa 2023-05-17 14:27 ` Masami Hiramatsu 2023-05-17 14:27 ` Masami Hiramatsu 2023-05-18 0:16 ` Andrii Nakryiko 2023-05-18 0:16 ` Andrii Nakryiko 2023-05-18 2:49 ` Ze Gao 2023-05-18 2:49 ` Ze Gao 2023-06-28 7:16 ` Yafang Shao [this message] 2023-06-28 7:16 ` Yafang Shao 2023-07-03 6:52 ` Ze Gao 2023-07-03 6:52 ` Ze Gao 2023-05-17 3:45 ` [PATCH v3 3/4] fprobe: add recursion detection in fprobe_exit_handler Ze Gao 2023-05-17 3:45 ` Ze Gao 2023-05-17 3:45 ` [PATCH v3 4/4] rethook, fprobe: do not trace rethook related functions Ze Gao 2023-05-17 3:45 ` Ze Gao
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CALOAHbC6UpfFOOibdDiC7xFc5YFUgZnk3MZ=3Ny6we=AcrNbew@mail.gmail.com' \ --to=laoar.shao@gmail.com \ --cc=agordeev@linux.ibm.com \ --cc=aou@eecs.berkeley.edu \ --cc=ast@kernel.org \ --cc=borntraeger@linux.ibm.com \ --cc=bp@alien8.de \ --cc=bpf@vger.kernel.org \ --cc=conor@kernel.org \ --cc=dave.hansen@linux.intel.com \ --cc=gor@linux.ibm.com \ --cc=hca@linux.ibm.com \ --cc=hpa@zytor.com \ --cc=jolsa@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=linux-s390@vger.kernel.org \ --cc=linux-trace-kernel@vger.kernel.org \ --cc=mhiramat@kernel.org \ --cc=mingo@redhat.com \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ --cc=rostedt@goodmis.org \ --cc=svens@linux.ibm.com \ --cc=tglx@linutronix.de \ --cc=x86@kernel.org \ --cc=yhs@fb.com \ --cc=zegao2021@gmail.com \ --cc=zegao@tencent.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.