From: Muchun Song <[email protected]>
commit 0cb2f1372baa60af8456388a574af6133edd7d80 upstream.
We found a case of kernel panic on our server. The stack trace is as
follows(omit some irrelevant information):
BUG: kernel NULL pointer dereference, address: 0000000000000080
RIP: 0010:kprobe_ftrace_handler+0x5e/0xe0
RSP: 0018:ffffb512c6550998 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff8e9d16eea018 RCX: 0000000000000000
RDX: ffffffffbe1179c0 RSI: ffffffffc0535564 RDI: ffffffffc0534ec0
RBP: ffffffffc0534ec1 R08: ffff8e9d1bbb0f00 R09: 0000000000000004
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: ffff8e9d1f797060 R14: 000000000000bacc R15: ffff8e9ce13eca00
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000080 CR3: 00000008453d0005 CR4: 00000000003606e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<IRQ>
ftrace_ops_assist_func+0x56/0xe0
ftrace_call+0x5/0x34
tcpa_statistic_send+0x5/0x130 [ttcp_engine]
The tcpa_statistic_send is the function being kprobed. After analysis,
the root cause is that the fourth parameter regs of kprobe_ftrace_handler
is NULL. Why regs is NULL? We use the crash tool to analyze the kdump.
crash> dis tcpa_statistic_send -r
<tcpa_statistic_send>: callq 0xffffffffbd8018c0 <ftrace_caller>
The tcpa_statistic_send calls ftrace_caller instead of ftrace_regs_caller.
So it is reasonable that the fourth parameter regs of kprobe_ftrace_handler
is NULL. In theory, we should call the ftrace_regs_caller instead of the
ftrace_caller. After in-depth analysis, we found a reproducible path.
Writing a simple kernel module which starts a periodic timer. The
timer's handler is named 'kprobe_test_timer_handler'. The module
name is kprobe_test.ko.
1) insmod kprobe_test.ko
2) bpftrace -e 'kretprobe:kprobe_test_timer_handler {}'
3) echo 0 > /proc/sys/kernel/ftrace_enabled
4) rmmod kprobe_test
5) stop step 2) kprobe
6) insmod kprobe_test.ko
7) bpftrace -e 'kretprobe:kprobe_test_timer_handler {}'
We mark the kprobe as GONE but not disarm the kprobe in the step 4).
The step 5) also do not disarm the kprobe when unregister kprobe. So
we do not remove the ip from the filter. In this case, when the module
loads again in the step 6), we will replace the code to ftrace_caller
via the ftrace_module_enable(). When we register kprobe again, we will
not replace ftrace_caller to ftrace_regs_caller because the ftrace is
disabled in the step 3). So the step 7) will trigger kernel panic. Fix
this problem by disarming the kprobe when the module is going away.
Link: https://lkml.kernel.org/r/[email protected]
Cc: [email protected]
Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization")
Acked-by: Masami Hiramatsu <[email protected]>
Signed-off-by: Muchun Song <[email protected]>
Co-developed-by: Chengming Zhou <[email protected]>
Signed-off-by: Chengming Zhou <[email protected]>
Signed-off-by: Steven Rostedt (VMware) <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
kernel/kprobes.c | 7 +++++++
1 file changed, 7 insertions(+)
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -2077,6 +2077,13 @@ static void kill_kprobe(struct kprobe *p
* the original probed function (which will be freed soon) any more.
*/
arch_remove_kprobe(p);
+
+ /*
+ * The module is going away. We should disarm the kprobe which
+ * is using ftrace.
+ */
+ if (kprobe_ftrace(p))
+ disarm_kprobe_ftrace(p);
}
/* Disable one kprobe */
On Thu, 20 Aug 2020 at 15:23, Greg Kroah-Hartman
<[email protected]> wrote:
>
> From: Muchun Song <[email protected]>
>
> commit 0cb2f1372baa60af8456388a574af6133edd7d80 upstream.
>
> We found a case of kernel panic on our server. The stack trace is as
> follows(omit some irrelevant information):
>
> BUG: kernel NULL pointer dereference, address: 0000000000000080
> RIP: 0010:kprobe_ftrace_handler+0x5e/0xe0
> RSP: 0018:ffffb512c6550998 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: ffff8e9d16eea018 RCX: 0000000000000000
> RDX: ffffffffbe1179c0 RSI: ffffffffc0535564 RDI: ffffffffc0534ec0
> RBP: ffffffffc0534ec1 R08: ffff8e9d1bbb0f00 R09: 0000000000000004
> R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
> R13: ffff8e9d1f797060 R14: 000000000000bacc R15: ffff8e9ce13eca00
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000080 CR3: 00000008453d0005 CR4: 00000000003606e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> <IRQ>
> ftrace_ops_assist_func+0x56/0xe0
> ftrace_call+0x5/0x34
> tcpa_statistic_send+0x5/0x130 [ttcp_engine]
>
> The tcpa_statistic_send is the function being kprobed. After analysis,
> the root cause is that the fourth parameter regs of kprobe_ftrace_handler
> is NULL. Why regs is NULL? We use the crash tool to analyze the kdump.
>
> crash> dis tcpa_statistic_send -r
> <tcpa_statistic_send>: callq 0xffffffffbd8018c0 <ftrace_caller>
>
> The tcpa_statistic_send calls ftrace_caller instead of ftrace_regs_caller.
> So it is reasonable that the fourth parameter regs of kprobe_ftrace_handler
> is NULL. In theory, we should call the ftrace_regs_caller instead of the
> ftrace_caller. After in-depth analysis, we found a reproducible path.
>
> Writing a simple kernel module which starts a periodic timer. The
> timer's handler is named 'kprobe_test_timer_handler'. The module
> name is kprobe_test.ko.
>
> 1) insmod kprobe_test.ko
> 2) bpftrace -e 'kretprobe:kprobe_test_timer_handler {}'
> 3) echo 0 > /proc/sys/kernel/ftrace_enabled
> 4) rmmod kprobe_test
> 5) stop step 2) kprobe
> 6) insmod kprobe_test.ko
> 7) bpftrace -e 'kretprobe:kprobe_test_timer_handler {}'
>
> We mark the kprobe as GONE but not disarm the kprobe in the step 4).
> The step 5) also do not disarm the kprobe when unregister kprobe. So
> we do not remove the ip from the filter. In this case, when the module
> loads again in the step 6), we will replace the code to ftrace_caller
> via the ftrace_module_enable(). When we register kprobe again, we will
> not replace ftrace_caller to ftrace_regs_caller because the ftrace is
> disabled in the step 3). So the step 7) will trigger kernel panic. Fix
> this problem by disarming the kprobe when the module is going away.
>
> Link: https://lkml.kernel.org/r/[email protected]
>
> Cc: [email protected]
> Fixes: ae6aa16fdc16 ("kprobes: introduce ftrace based optimization")
> Acked-by: Masami Hiramatsu <[email protected]>
> Signed-off-by: Muchun Song <[email protected]>
> Co-developed-by: Chengming Zhou <[email protected]>
> Signed-off-by: Chengming Zhou <[email protected]>
> Signed-off-by: Steven Rostedt (VMware) <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
stable rc branch 4.19 build warning on arm64.
../kernel/kprobes.c: In function ‘kill_kprobe’:
../kernel/kprobes.c:1070:33: warning: statement with no effect [-Wunused-value]
1070 | #define disarm_kprobe_ftrace(p) (-ENODEV)
| ^
../kernel/kprobes.c:2090:3: note: in expansion of macro ‘disarm_kprobe_ftrace’
2090 | disarm_kprobe_ftrace(p);
| ^~~~~~~~~~~~~~~~~~~~
>
> ---
> kernel/kprobes.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -2077,6 +2077,13 @@ static void kill_kprobe(struct kprobe *p
> * the original probed function (which will be freed soon) any more.
> */
> arch_remove_kprobe(p);
> +
> + /*
> + * The module is going away. We should disarm the kprobe which
> + * is using ftrace.
> + */
> + if (kprobe_ftrace(p))
> + disarm_kprobe_ftrace(p);
> }
>
> /* Disable one kprobe */
>
--
Linaro LKFT
https://lkft.linaro.org
On Tue, 29 Sep 2020 01:32:59 +0530
Naresh Kamboju <[email protected]> wrote:
> stable rc branch 4.19 build warning on arm64.
>
> ../kernel/kprobes.c: In function ‘kill_kprobe’:
> ../kernel/kprobes.c:1070:33: warning: statement with no effect [-Wunused-value]
> 1070 | #define disarm_kprobe_ftrace(p) (-ENODEV)
> | ^
> ../kernel/kprobes.c:2090:3: note: in expansion of macro ‘disarm_kprobe_ftrace’
> 2090 | disarm_kprobe_ftrace(p);
> | ^~~~~~~~~~~~~~~~~~~~
Seems to affect upstream as well.
-- Steve
On Mon, 28 Sep 2020 18:09:42 -0400
Steven Rostedt <[email protected]> wrote:
> On Tue, 29 Sep 2020 01:32:59 +0530
> Naresh Kamboju <[email protected]> wrote:
>
> > stable rc branch 4.19 build warning on arm64.
> >
> > ../kernel/kprobes.c: In function ‘kill_kprobe’:
> > ../kernel/kprobes.c:1070:33: warning: statement with no effect [-Wunused-value]
> > 1070 | #define disarm_kprobe_ftrace(p) (-ENODEV)
> > | ^
> > ../kernel/kprobes.c:2090:3: note: in expansion of macro ‘disarm_kprobe_ftrace’
> > 2090 | disarm_kprobe_ftrace(p);
> > | ^~~~~~~~~~~~~~~~~~~~
>
> Seems to affect upstream as well.
>
Bah, no (tested the wrong kernel).
You want this commit too:
10de795a5addd ("kprobes: Fix compiler warning for !CONFIG_KPROBES_ON_FTRACE")
-- Steve
Hi,
On Mon, 28 Sep 2020 18:15:35 -0400
Steven Rostedt <[email protected]> wrote:
> On Mon, 28 Sep 2020 18:09:42 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Tue, 29 Sep 2020 01:32:59 +0530
> > Naresh Kamboju <[email protected]> wrote:
> >
> > > stable rc branch 4.19 build warning on arm64.
> > >
> > > ../kernel/kprobes.c: In function ‘kill_kprobe’:
> > > ../kernel/kprobes.c:1070:33: warning: statement with no effect [-Wunused-value]
> > > 1070 | #define disarm_kprobe_ftrace(p) (-ENODEV)
> > > | ^
> > > ../kernel/kprobes.c:2090:3: note: in expansion of macro ‘disarm_kprobe_ftrace’
> > > 2090 | disarm_kprobe_ftrace(p);
> > > | ^~~~~~~~~~~~~~~~~~~~
> >
> > Seems to affect upstream as well.
> >
>
> Bah, no (tested the wrong kernel).
>
> You want this commit too:
>
> 10de795a5addd ("kprobes: Fix compiler warning for !CONFIG_KPROBES_ON_FTRACE")
It seems that this commit's Fixes tag is wrong.
ae6aa16fdc163 (Masami Hiramatsu 2012-06-05 19:28:32 +0900 1079) #define prepare_kprobe(p) arch_prepare_kprobe(p)
12310e3437554 (Jessica Yu 2018-01-10 00:51:23 +0100 1080) #define arm_kprobe_ftrace(p) (-ENODEV)
297f9233b53a0 (Jessica Yu 2018-01-10 00:51:24 +0100 1081) #define disarm_kprobe_ftrace(p) (-ENODEV)
Thus, it should have "Fixes: 297f9233b53a ("kprobes: Propagate error from disarm_kprobe_ftrace()")"
$ git tag -l --contains 297f9233b53a | grep "^v[[:digit:].]*$" | cut -f1-2 -d. | uniq
v4.16
v4.17
v4.18
v4.19
v4.20
v5.0
v5.1
v5.2
v5.3
v5.4
v5.5
v5.6
v5.7
v5.8
So the commit 10de795a5addd must be backported to 4.19.y and 5.4.y.
Thank you,
--
Masami Hiramatsu <[email protected]>
On Tue, Sep 29, 2020 at 02:49:54PM +0900, Masami Hiramatsu wrote:
> Hi,
>
> On Mon, 28 Sep 2020 18:15:35 -0400
> Steven Rostedt <[email protected]> wrote:
>
> > On Mon, 28 Sep 2020 18:09:42 -0400
> > Steven Rostedt <[email protected]> wrote:
> >
> > > On Tue, 29 Sep 2020 01:32:59 +0530
> > > Naresh Kamboju <[email protected]> wrote:
> > >
> > > > stable rc branch 4.19 build warning on arm64.
> > > >
> > > > ../kernel/kprobes.c: In function ‘kill_kprobe’:
> > > > ../kernel/kprobes.c:1070:33: warning: statement with no effect [-Wunused-value]
> > > > 1070 | #define disarm_kprobe_ftrace(p) (-ENODEV)
> > > > | ^
> > > > ../kernel/kprobes.c:2090:3: note: in expansion of macro ‘disarm_kprobe_ftrace’
> > > > 2090 | disarm_kprobe_ftrace(p);
> > > > | ^~~~~~~~~~~~~~~~~~~~
> > >
> > > Seems to affect upstream as well.
> > >
> >
> > Bah, no (tested the wrong kernel).
> >
> > You want this commit too:
> >
> > 10de795a5addd ("kprobes: Fix compiler warning for !CONFIG_KPROBES_ON_FTRACE")
>
> It seems that this commit's Fixes tag is wrong.
>
> ae6aa16fdc163 (Masami Hiramatsu 2012-06-05 19:28:32 +0900 1079) #define prepare_kprobe(p) arch_prepare_kprobe(p)
> 12310e3437554 (Jessica Yu 2018-01-10 00:51:23 +0100 1080) #define arm_kprobe_ftrace(p) (-ENODEV)
> 297f9233b53a0 (Jessica Yu 2018-01-10 00:51:24 +0100 1081) #define disarm_kprobe_ftrace(p) (-ENODEV)
>
> Thus, it should have "Fixes: 297f9233b53a ("kprobes: Propagate error from disarm_kprobe_ftrace()")"
>
> $ git tag -l --contains 297f9233b53a | grep "^v[[:digit:].]*$" | cut -f1-2 -d. | uniq
> v4.16
> v4.17
> v4.18
> v4.19
> v4.20
> v5.0
> v5.1
> v5.2
> v5.3
> v5.4
> v5.5
> v5.6
> v5.7
> v5.8
>
> So the commit 10de795a5addd must be backported to 4.19.y and 5.4.y.
Now queued up, thanks.
greg k-h