Re: [PATCH] perf: fix possible divide-by-zero in perf_swevent_overflow()

Previous thread: [PATCH v2 0/2] initramfs: Cleanup and fix initramfs size calculation by Hendrik Brueckner on Thursday, August 26, 2010 - 4:41 am. (2 messages)

Next thread: [PATCH] mips: irq: add stackoverflow detection by Adam Jiang on Thursday, August 26, 2010 - 6:19 am. (3 messages)
From: Dongdong Deng
Date: Thursday, August 26, 2010 - 5:07 am

The event->hw.last_period is possible to zero, thus it will
cause divide_by_zero later in perf_swevent_set_period().

This patch checks event->hw.last_period before invoke
perf_swevent_set_period() and replaces "event->hw" with "hwc".

Signed-off-by: Dongdong Deng <dongdong.deng@windriver.com>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Paul Mackerras <paulus@samba.org>
---
 kernel/perf_event.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/perf_event.c b/kernel/perf_event.c
index 403d180..ccba741 100644
--- a/kernel/perf_event.c
+++ b/kernel/perf_event.c
@@ -4050,8 +4050,8 @@ static void perf_swevent_overflow(struct perf_event *event, u64 overflow,
 	struct hw_perf_event *hwc = &event->hw;
 	int throttle = 0;
 
-	data->period = event->hw.last_period;
-	if (!overflow)
+	data->period = hwc->last_period;
+	if (!overflow && hwc->last_period)
 		overflow = perf_swevent_set_period(event);
 
 	if (hwc->interrupts == MAX_INTERRUPTS)
-- 
1.6.0.4

--

From: Peter Zijlstra
Date: Thursday, August 26, 2010 - 5:12 am

From: DDD
Date: Thursday, August 26, 2010 - 5:36 am

When I am running the kgdbts to test the hw_breakpoint_layer with kgdb,
I get a call trace as following and this problem is hardly to reproduce.

Maybe the root cause was from kgdb/hw_breakpoint_layer,
but add a checking is good to us and harmless. :-)

Thanks,
Dongdong



------------[ cut here ]------------
WARNING: at /buildarea/ddeng/build/linux/drivers/misc/kgdbts.c:703 
run_simple_test+0x1fc/0x2a0()
Hardware name: Moon Creek platform
Modules linked in:
Pid: 668, comm: sh Tainted: G        W  2.6.36-rc1-00133-g47ade1d #43
Call Trace:
  [<c103b1fd>] warn_slowpath_common+0x6d/0xa0
  [<c124e15c>] ? run_simple_test+0x1fc/0x2a0
  [<c124e15c>] ? run_simple_test+0x1fc/0x2a0
  [<c103b245>] warn_slowpath_null+0x15/0x20
  [<c124e15c>] run_simple_test+0x1fc/0x2a0
  [<c124daf4>] kgdbts_put_char+0x14/0x20
  [<c107bab8>] gdb_serial_stub+0x678/0xbd0
  [<c101aced>] ? default_send_IPI_allbutself+0x7d/0x90
  [<c107a487>] kgdb_cpu_enter+0x197/0x470
  [<c107a8e7>] kgdb_handle_exception+0x47/0x140
  [<c13be9f0>] ? _raw_read_unlock+0x10/0x30
  [<c101eb48>] __kgdb_notify+0x38/0x170
  [<c130f1e0>] ? sock_queue_rcv_skb+0xe0/0x130
  [<c101ec8e>] kgdb_notify+0xe/0x20
  [<c105b815>] notifier_call_chain+0x35/0x70
  [<c1003910>] ? do_divide_error+0x0/0xa0
  [<c105bce8>] __atomic_notifier_call_chain+0x28/0x50
  [<c105bd2a>] atomic_notifier_call_chain+0x1a/0x20
  [<c105bd7d>] notify_die+0x2d/0x30
  [<c1003964>] do_divide_error+0x54/0xa0
  [<c11cf3d5>] ? div64_u64+0x55/0x80
  [<c1361c22>] ? udp_rcv+0x12/0x20
  [<c133e6e7>] ? ip_local_deliver_finish+0x127/0x240
  [<c1361c22>] ? udp_rcv+0x12/0x20
  [<c133e887>] ? ip_local_deliver+0x87/0x90
  [<c109bb8e>] ? perf_output_begin+0x1fe/0x230
  [<c133e0c3>] ? ip_rcv_finish+0xc3/0x320
  [<c13bf822>] error_code+0x66/0x6c
  [<c1003910>] ? do_divide_error+0x0/0xa0
  [<c11cf3d5>] ? div64_u64+0x55/0x80
  [<c1097cac>] perf_swevent_set_period+0x7c/0x100
  [<c109c797>] perf_swevent_overflow+0x87/0x90
  [<c109c85f>] perf_swevent_add+0xbf/0xd0
  ...
From: Peter Zijlstra
Date: Thursday, August 26, 2010 - 5:58 am

Yeah, I think there's a bug in the hw_breakpoint stuff, does something

Except that code path is already too bloated and should be reduced not
added to and conditionals are expensive.


---
 kernel/hw_breakpoint.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/kernel/hw_breakpoint.c b/kernel/hw_breakpoint.c
index d71a987..f57ebee 100644
--- a/kernel/hw_breakpoint.c
+++ b/kernel/hw_breakpoint.c
@@ -600,9 +600,20 @@ static int __init init_hw_breakpoint(void)
 }
 core_initcall(init_hw_breakpoint);
 
+static int hw_breakpoint_enable(struct perf_event *event)
+{
+	struct hw_perf_event *hwc = &event->hw;
+
+	if (hwc->sample_period) {
+		hwc->last_period = hwc->sample_period;
+		perf_swevent_set_period(event);
+	}
+
+	return arch_install_hw_breakpoint(event);
+}
 
 struct pmu perf_ops_bp = {
-	.enable		= arch_install_hw_breakpoint,
+	.enable		= hw_breakpoint_enable,
 	.disable	= arch_uninstall_hw_breakpoint,
 	.read		= hw_breakpoint_pmu_read,
 };

--

From: DDD
Date: Friday, August 27, 2010 - 5:21 am

hello Peter,

Thanks for your patch, but I still could reproduce the problem with your 
patch.


Thanks,

--

From: Peter Zijlstra
Date: Friday, August 27, 2010 - 5:50 am

Frederic, any clue as to what makes hw breakpoints go funny and have
--

From: DDD
Date: Friday, August 27, 2010 - 6:19 am

Hi Peter,

Thanks for you take care of it, I have got the root cause of it now.

It is the kgdb using hw_breakpoint_layer's API problem, and I have a RFC 
patch for it.(maybe it is not correctly), I will contact with Jason to 
fix this problem.

Thank you very much,

--

From: Frederic Weisbecker
Date: Friday, August 27, 2010 - 6:37 am

Thanks!

We are waiting for your patch then.

--

From: DDD
Date: Sunday, October 24, 2010 - 10:58 pm

Hello Frederic,

Sorry for delay, the patch have been accepted by Jason.

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=c1bb9a9c19...

Dongdong
--

Previous thread: [PATCH v2 0/2] initramfs: Cleanup and fix initramfs size calculation by Hendrik Brueckner on Thursday, August 26, 2010 - 4:41 am. (2 messages)

Next thread: [PATCH] mips: irq: add stackoverflow detection by Adam Jiang on Thursday, August 26, 2010 - 6:19 am. (3 messages)