| Message ID | 1410360247-13791-1-git-send-email-daniel.thompson@linaro.org |
|---|---|
| State | New |
| Headers | show |
This patchset makes it possible to use kdb's ftdump command without panicing, crashing or livelocking. Changes since v1: * Fixed use-after-free problems in v1 by adding logic to NULL out buffer_iter during free operations. * Introduced a second patch to fix a live lock when trying to display an empty trace buffer. Daniel Thompson (2): trace: kdb: Fix kernel panic during ftdump trace: kdb: Fix kernel livelock with empty buffers kernel/trace/trace_kdb.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) -- 1.9.3
Is Jason Wessel still actively maintaining this? I'd like an acked-by from the KDB maintainer before pulling this. Thanks! -- Steve On Wed, 24 Sep 2014 11:32:59 +0100 Daniel Thompson <daniel.thompson@linaro.org> wrote: > This patchset makes it possible to use kdb's ftdump command without > panicing, crashing or livelocking. > > Changes since v1: > > * Fixed use-after-free problems in v1 by adding logic to NULL out > buffer_iter during free operations. > > * Introduced a second patch to fix a live lock when trying to display > an empty trace buffer. > > > Daniel Thompson (2): > trace: kdb: Fix kernel panic during ftdump > trace: kdb: Fix kernel livelock with empty buffers > > kernel/trace/trace_kdb.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > -- > 1.9.3
On 24/09/14 16:07, Steven Rostedt wrote: > Is Jason Wessel still actively maintaining this? He still maintaining kgdb and kdb although he is not in MAINTAINERS for this particular file (perhaps he should be?). > I'd like an acked-by from the KDB maintainer before pulling this. Makes sense. > > Thanks! > > -- Steve > > > On Wed, 24 Sep 2014 11:32:59 +0100 > Daniel Thompson <daniel.thompson@linaro.org> wrote: > >> This patchset makes it possible to use kdb's ftdump command without >> panicing, crashing or livelocking. >> >> Changes since v1: >> >> * Fixed use-after-free problems in v1 by adding logic to NULL out >> buffer_iter during free operations. >> >> * Introduced a second patch to fix a live lock when trying to display >> an empty trace buffer. >> >> >> Daniel Thompson (2): >> trace: kdb: Fix kernel panic during ftdump >> trace: kdb: Fix kernel livelock with empty buffers >> >> kernel/trace/trace_kdb.c | 21 +++++++++++++-------- >> 1 file changed, 13 insertions(+), 8 deletions(-) >> >> -- >> 1.9.3 >
This patchset makes it possible to use kdb's ftdump command without panicing, crashing or livelocking. The ftdump command cannot be used at all without these changes. IIRC this patches are still pending Jason's ack. Changes since v1: * Fixed use-after-free problems in v1 by adding logic to NULL out buffer_iter during free operations. * Introduced a second patch to fix a live lock when trying to display an empty trace buffer. Daniel Thompson (2): trace: kdb: Fix kernel panic during ftdump trace: kdb: Fix kernel livelock with empty buffers kernel/trace/trace_kdb.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) -- 1.9.3
On Thu, 6 Nov 2014 12:41:54 +0000 Daniel Thompson <daniel.thompson@linaro.org> wrote: > This patchset makes it possible to use kdb's ftdump command without > panicing, crashing or livelocking. The ftdump command cannot be used > at all without these changes. > > IIRC this patches are still pending Jason's ack. I haven't heard from Jason in a long time. Is he still active? -- Steve > > Changes since v1: > > * Fixed use-after-free problems in v1 by adding logic to NULL out > buffer_iter during free operations. > > * Introduced a second patch to fix a live lock when trying to display > an empty trace buffer. > > > Daniel Thompson (2): > trace: kdb: Fix kernel panic during ftdump > trace: kdb: Fix kernel livelock with empty buffers > > kernel/trace/trace_kdb.c | 21 +++++++++++++-------- > 1 file changed, 13 insertions(+), 8 deletions(-) > > -- > 1.9.3 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On 06/11/14 13:27, Steven Rostedt wrote: > On Thu, 6 Nov 2014 12:41:54 +0000 > Daniel Thompson <daniel.thompson@linaro.org> wrote: > >> This patchset makes it possible to use kdb's ftdump command without >> panicing, crashing or livelocking. The ftdump command cannot be used >> at all without these changes. >> >> IIRC this patches are still pending Jason's ack. > > I haven't heard from Jason in a long time. Is he still active? [sorry for the delay, I wanted to give Jason a chance to answer this] Very occasionally. I can't find anything on lkml in the last three months, and I have unreviewed kdb patches that stretch back well beyond that. That said he still helps people on kgdb-bugreport@ from time-to-time (and as recently as last week). I've also had a little bit of private contact although nothing very recent. On that basis I'd say you shouldn't feel guilty if you have to accept a change here without an ack. Daniel.
On Mon, 10 Nov 2014 09:41:32 +0000 Daniel Thompson <daniel.thompson@linaro.org> wrote: > On 06/11/14 13:27, Steven Rostedt wrote: > > On Thu, 6 Nov 2014 12:41:54 +0000 > > Daniel Thompson <daniel.thompson@linaro.org> wrote: > > > >> This patchset makes it possible to use kdb's ftdump command without > >> panicing, crashing or livelocking. The ftdump command cannot be used > >> at all without these changes. > >> > >> IIRC this patches are still pending Jason's ack. > > > > I haven't heard from Jason in a long time. Is he still active? > > [sorry for the delay, I wanted to give Jason a chance to answer this] > > Very occasionally. > > I can't find anything on lkml in the last three months, and I have > unreviewed kdb patches that stretch back well beyond that. > > That said he still helps people on kgdb-bugreport@ from time-to-time > (and as recently as last week). I've also had a little bit of private > contact although nothing very recent. > > On that basis I'd say you shouldn't feel guilty if you have to accept a > change here without an ack. > He had more than enough time to respond. OK, I'll take it. Looking at the first patch, I notice that there's no protection of the static buffer_iter array. I also noticed that there's no protection of the static iter itself (which was there before your patch). I take it that this code is not re-entrant. Thanks! -- Steve
On 14/11/14 02:16, Steven Rostedt wrote: > On Mon, 10 Nov 2014 09:41:32 +0000 > Daniel Thompson <daniel.thompson@linaro.org> wrote: > >> On 06/11/14 13:27, Steven Rostedt wrote: >>> On Thu, 6 Nov 2014 12:41:54 +0000 >>> Daniel Thompson <daniel.thompson@linaro.org> wrote: >>> >>>> This patchset makes it possible to use kdb's ftdump command without >>>> panicing, crashing or livelocking. The ftdump command cannot be used >>>> at all without these changes. >>>> >>>> IIRC this patches are still pending Jason's ack. >>> >>> I haven't heard from Jason in a long time. Is he still active? >> >> [sorry for the delay, I wanted to give Jason a chance to answer this] >> >> Very occasionally. >> >> I can't find anything on lkml in the last three months, and I have >> unreviewed kdb patches that stretch back well beyond that. >> >> That said he still helps people on kgdb-bugreport@ from time-to-time >> (and as recently as last week). I've also had a little bit of private >> contact although nothing very recent. >> >> On that basis I'd say you shouldn't feel guilty if you have to accept a >> change here without an ack. >> > > He had more than enough time to respond. OK, I'll take it. > > Looking at the first patch, I notice that there's no protection of the > static buffer_iter array. I also noticed that there's no protection of > the static iter itself (which was there before your patch). I take it > that this code is not re-entrant. No. k(g)db halts all other processors before entering the command dispatch loop and it forces a kernel panic if the debugger is reentered by the same CPU. Daniel.
This patchset makes it possible to use kdb's ftdump command without panicing, crashing or livelocking. The ftdump command cannot be used at all without these changes. IIRC this patches are still pending Jason's ack. v3: * Remove lines leading with --- from the patch comments, now it it -~- cut here -~- and "git am" did the right thing when I checked (Steven Rostedt) v2: * Fixed use-after-free problems in v1 by adding logic to NULL out buffer_iter during free operations. * Introduced a second patch to fix a live lock when trying to display an empty trace buffer. Daniel Thompson (2): trace: kdb: Fix kernel panic during ftdump trace: kdb: Fix kernel livelock with empty buffers kernel/trace/trace_kdb.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) -- 1.9.3
diff --git a/kernel/trace/trace_kdb.c b/kernel/trace/trace_kdb.c index bd90e1b..989288a 100644 --- a/kernel/trace/trace_kdb.c +++ b/kernel/trace/trace_kdb.c @@ -20,10 +20,12 @@ static void ftrace_dump_buf(int skip_lines, long cpu_file) { /* use static because iter can be a bit big for the stack */ static struct trace_iterator iter; + static struct ring_buffer_iter *buffer_iter[CONFIG_NR_CPUS]; unsigned int old_userobj; int cnt = 0, cpu; trace_init_global_iter(&iter); + iter.buffer_iter = buffer_iter; for_each_tracing_cpu(cpu) { atomic_inc(&per_cpu_ptr(iter.trace_buffer->data, cpu)->disabled);
Currently kdb's ftdump command unconditionally crashes due to a null pointer de-reference whenever the command is run. This in turn causes the kernel to panic. The abridged stacktrace (gathered with ARCH=arm) is: --- cut here --- [<c09535ac>] (panic) from [<c02132dc>] (die+0x264/0x440) [<c02132dc>] (die) from [<c0952eb8>] (__do_kernel_fault.part.11+0x74/0x84) [<c0952eb8>] (__do_kernel_fault.part.11) from [<c021f954>] (do_page_fault+0x1d0/0x3c4) [<c021f954>] (do_page_fault) from [<c020846c>] (do_DataAbort+0x48/0xac) [<c020846c>] (do_DataAbort) from [<c0213c58>] (__dabt_svc+0x38/0x60) Exception stack(0xc0deba88 to 0xc0debad0) ba80: e8c29180 00000001 e9854304 e9854300 c0f567d8 c0df2580 baa0: 00000000 00000000 00000000 c0f117b8 c0e3a3c0 c0debb0c 00000000 c0debad0 bac0: 0000672e c02f4d60 60000193 ffffffff [<c0213c58>] (__dabt_svc) from [<c02f4d60>] (kdb_ftdump+0x1e4/0x3d8) [<c02f4d60>] (kdb_ftdump) from [<c02ce328>] (kdb_parse+0x2b8/0x698) [<c02ce328>] (kdb_parse) from [<c02ceef0>] (kdb_main_loop+0x52c/0x784) [<c02ceef0>] (kdb_main_loop) from [<c02d1b0c>] (kdb_stub+0x238/0x490) --- cut here --- The NULL deref occurs due to the initialized use of struct trace_iter's buffer_iter member. This patch solves this by providing a collection of ring_buffer_iter(s) and using this to initialized buffer_iter. Note that static allocation is used solely because the trace_iter itself is also static allocated. Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org> Cc: Jason Wessel <jason.wessel@windriver.com> Cc: Steven Rostedt <rostedt@goodmis.org> Cc: Ingo Molnar <mingo@redhat.com> --- kernel/trace/trace_kdb.c | 2 ++ 1 file changed, 2 insertions(+) -- 1.9.3