[PATCH] arch: x86: init hpet event_handler to noop

[PATCH] arch: x86: init hpet event_handler to noop

[Vladimir Davydov]

If hpet is enabled by hpet_late_init() - this usually occurs on systems
with buggy BIOS, which does not report about hpet presence through ACPI,
hpet_clockevent's event_handler can be left uninitialized by
clockevents_register_device() because of hpet_clockevent low rating (by
the time hpet_late_init() is called, high prio apic timers have already
been setup). The event_handler is then initialized a bit later by the
clocksource_done_booting() procedure.

Normally, timer interrupts should not be delivered between these two
calls, but if e.g. the kernel is booted using kexec, there might be some
pending interrupts from the previous kernel's context, which can lead to
a NULL pointer dereference in timer_interrupt().

Avoid this by initializing hpet's event_handler to noop in its definition.

Signed-off-by: Vladimir Davydov <vdavydov@parallels.com>
---
 arch/x86/kernel/hpet.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c
index ad0de0c..a736a6e 100644
--- a/arch/x86/kernel/hpet.c
+++ b/arch/x86/kernel/hpet.c
@@ -238,6 +238,7 @@ static struct clock_event_device hpet_clockevent = {
 	.set_next_event = hpet_legacy_next_event,
 	.irq		= 0,
 	.rating		= 50,
+	.event_handler	= clockevents_handle_noop,
 };
 
 static void hpet_stop_counter(void)
-- 
1.7.1

Re: [PATCH] arch: x86: init hpet event_handler to noop

[Thomas Gleixner]

On Tue, 24 Apr 2012, Vladimir Davydov wrote:

> If hpet is enabled by hpet_late_init() - this usually occurs on systems
> with buggy BIOS, which does not report about hpet presence through ACPI,
> hpet_clockevent's event_handler can be left uninitialized by
> clockevents_register_device() because of hpet_clockevent low rating (by
> the time hpet_late_init() is called, high prio apic timers have already
> been setup). The event_handler is then initialized a bit later by the
> clocksource_done_booting() procedure.
> 

This explanation is worse than an oracle and aside of that, it's
patently wrong.

How the hell is clocksource_done_booting() related to the HPET
clockevent mechanism?

> Normally, timer interrupts should not be delivered between these two
> calls, but if e.g. the kernel is booted using kexec, there might be some
> pending interrupts from the previous kernel's context, which can lead to
> a NULL pointer dereference in timer_interrupt().

How is kexec related to this?

And how should pending interrupts be not handled by the always first
initialized PIT ?

> Avoid this by initializing hpet's event_handler to noop in its definition.

"Avoid" is the correct term: You're avoiding to track down the root
cause of the problem. 

This is fairy tale mode. I really love fairy tales, just not in the
context of kernel code.

Please provide proper proof why this can happen instead of some
handwavy explanations.

Thanks,

	tglx

Re: [PATCH] arch: x86: init hpet event_handler to noop

[Vladimir Davydov]

On 05/08/2012 12:26 AM, Thomas Gleixner wrote:
> On Tue, 24 Apr 2012, Vladimir Davydov wrote:
>
>> If hpet is enabled by hpet_late_init() - this usually occurs on systems
>> with buggy BIOS, which does not report about hpet presence through ACPI,
>> hpet_clockevent's event_handler can be left uninitialized by
>> clockevents_register_device() because of hpet_clockevent low rating (by
>> the time hpet_late_init() is called, high prio apic timers have already
>> been setup). The event_handler is then initialized a bit later by the
>> clocksource_done_booting() procedure.
>>
> This explanation is worse than an oracle and aside of that, it's
> patently wrong.
>
> How the hell is clocksource_done_booting() related to the HPET
> clockevent mechanism?
>
>> Normally, timer interrupts should not be delivered between these two
>> calls, but if e.g. the kernel is booted using kexec, there might be some
>> pending interrupts from the previous kernel's context, which can lead to
>> a NULL pointer dereference in timer_interrupt().
> How is kexec related to this?
>
> And how should pending interrupts be not handled by the always first
> initialized PIT ?

 From timer_interrupt() global_clock_event->event_handler is called.

global_clock_event is first initialized to i8253_clockevent (PIT), but 
from hpet_late_init() it is reinitialized as follows (see 
hpet_legacy_clockevent_register()):

clockevents_config_and_register(&hpet_clockevent, ...);
global_clock_event = &hpet_clockevent;

It turns out that clockevents_config_and_register() lefts 
global_clock_event->event_handler unintialized (NULL).

After digging deeper into clockevent_config_and_register(), I've found 
that the event_handler should be set inside tick_check_new_device(), but 
by the time we call it, high prio apic timers have already been setup 
(see setup_APIC_timer()) so that hpet is not initialized as a oneshot 
device. The attempt to set it as a broadcast device also fails since the 
broadcast cpumask is empty (see tick_check_broadcast_device()).

Thus, for some time we have event_handler=NULL, which can lead to a NULL 
ptr deref in timer_interrupt() if a pending interrupt is handled.

>
>> Avoid this by initializing hpet's event_handler to noop in its definition.
> "Avoid" is the correct term: You're avoiding to track down the root
> cause of the problem.
>
> This is fairy tale mode. I really love fairy tales, just not in the
> context of kernel code.
>
> Please provide proper proof why this can happen instead of some
> handwavy explanations.
>
> Thanks,
>
> 	tglx