[2/4] fprobe: make fprobe_kprobe_handler recursion free
Commit Message
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(-)
Comments
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 *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".
-- Steve
> + 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);
> }
>
Hi Steven,
On Tue, May 16, 2023 at 9:25 AM 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 *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
@@ -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
+ 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 notrace 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,
+static void notrace 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++;
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,