Re: [PATCH] Avoid race conditions in HPET initialization

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/01/14 14:23, Jan Beulich wrote:
>>>> On 03.01.14 at 14:15, Frediano Ziglio <frediano.ziglio@citrix.com> wrote:
>> Avoid turning on legacy interrupts before hpet_event has been set up.
>> Particularly, the spinlock can be uninitialised at the point at which
>> the interrupt first arrives.
> I suppose you actually saw this issue, but I currently fail to see how
> it would occur:
>
>         spin_lock_init(&hpet_events[i].lock);
>         wmb();
>         hpet_events[i].event_handler = handle_hpet_broadcast;
>
> guarantees that the lock gets initialized before the handler gets set
> (i.e. if anything you'd do a call through a NULL pointer). And this
>
>     if ( !num_hpets_used )
>         hpet_events->flags = HPET_EVT_LEGACY;
>
> happens even later, yet hpet_legacy_irq_tick() checks that flag
> before calling the handler (and hence before taking the lock).
>
> Before applying the patch I'd like to understand what I'm
> overlooking.
>
> Jan
>

We did indeed find this issue, but I overlooked a key factor.

XenServer is running with my HPET series to fix the stack overflows
which automated testing reliably finds.

My series changes the initialisation order of this, opening up this race
condition.


Overall, turning on the HPET interrupt before initialising its structure
is somewhat poor form, but now you have pointed it out, I don't think
that current upstream in vulnerable to the uninitialised spinlock.

It might be better if I just folded this fix into my series.

~Andrew