[PATCH] Avoid race conditions in HPET initialization

[PATCH] Avoid race conditions in HPET initialization

[Frediano Ziglio]

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.

Also, fix a memory leak of a cpumask in the unlikely event that
hpet_assign_irq() fails.

Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>
---
 xen/arch/x86/hpet.c |   59 +++++++++++++++++++++++++++------------------------
 1 file changed, 31 insertions(+), 28 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index 3a4f7e8..0dedfb7 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -387,6 +387,26 @@ static int __init hpet_assign_irq(struct hpet_event_channel *ch)
     return 0;
 }
 
+static void __init hpet_init_channel(struct hpet_event_channel *ch)
+{
+    u64 hpet_rate = hpet_setup();
+
+    /*
+     * The period is a femto seconds value. We need to calculate the scaled
+     * math multiplication factor for nanosecond to hpet tick conversion.
+     */
+    ch->mult = div_sc((unsigned long)hpet_rate,
+                                     1000000000ul, 32);
+    ch->shift = 32;
+    ch->next_event = STIME_MAX;
+    spin_lock_init(&ch->lock);
+    ch->event_handler = handle_hpet_broadcast;
+
+    ch->msi.irq = -1;
+    ch->msi.msi_attrib.maskbit = 1;
+    ch->msi.msi_attrib.pos = MSI_TYPE_HPET;
+}
+
 static void __init hpet_fsb_cap_lookup(void)
 {
     u32 id;
@@ -423,11 +443,15 @@ static void __init hpet_fsb_cap_lookup(void)
             break;
         }
 
+        hpet_init_channel(ch);
+
         ch->flags = 0;
         ch->idx = i;
 
         if ( hpet_assign_irq(ch) == 0 )
             num_hpets_used++;
+        else
+            free_cpumask_var(ch->cpumask);
     }
 
     printk(XENLOG_INFO "HPET: %u timers usable for broadcast (%u total)\n",
@@ -553,7 +577,6 @@ void __init hpet_broadcast_init(void)
 {
     u64 hpet_rate = hpet_setup();
     u32 hpet_id, cfg;
-    unsigned int i, n;
 
     if ( hpet_rate == 0 || hpet_broadcast_is_available() )
         return;
@@ -565,7 +588,6 @@ void __init hpet_broadcast_init(void)
     {
         /* Stop HPET legacy interrupts */
         cfg &= ~HPET_CFG_LEGACY;
-        n = num_hpets_used;
     }
     else
     {
@@ -577,11 +599,10 @@ void __init hpet_broadcast_init(void)
             hpet_events = xzalloc(struct hpet_event_channel);
         if ( !hpet_events || !zalloc_cpumask_var(&hpet_events->cpumask) )
             return;
-        hpet_events->msi.irq = -1;
+        hpet_init_channel(hpet_events);
 
         /* Start HPET legacy interrupts */
         cfg |= HPET_CFG_LEGACY;
-        n = 1;
 
         if ( !force_hpet_broadcast )
             pv_rtc_handler = handle_rtc_once;
@@ -589,31 +610,13 @@ void __init hpet_broadcast_init(void)
 
     hpet_write32(cfg, HPET_CFG);
 
-    for ( i = 0; i < n; i++ )
+    if ( cfg & HPET_CFG_LEGACY )
     {
-        if ( i == 0 && (cfg & HPET_CFG_LEGACY) )
-        {
-            /* set HPET T0 as oneshot */
-            cfg = hpet_read32(HPET_Tn_CFG(0));
-            cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
-            cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
-            hpet_write32(cfg, HPET_Tn_CFG(0));
-        }
-
-        /*
-         * The period is a femto seconds value. We need to calculate the scaled
-         * math multiplication factor for nanosecond to hpet tick conversion.
-         */
-        hpet_events[i].mult = div_sc((unsigned long)hpet_rate,
-                                     1000000000ul, 32);
-        hpet_events[i].shift = 32;
-        hpet_events[i].next_event = STIME_MAX;
-        spin_lock_init(&hpet_events[i].lock);
-        wmb();
-        hpet_events[i].event_handler = handle_hpet_broadcast;
-
-        hpet_events[i].msi.msi_attrib.maskbit = 1;
-        hpet_events[i].msi.msi_attrib.pos = MSI_TYPE_HPET;
+        /* set HPET T0 as oneshot */
+        cfg = hpet_read32(HPET_Tn_CFG(0));
+        cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
+        cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
+        hpet_write32(cfg, HPET_Tn_CFG(0));
     }
 
     if ( !num_hpets_used )
-- 
1.7.10.4

Re: [PATCH] Avoid race conditions in HPET initialization

[Andrew Cooper]

On 03/01/14 13:15, Frediano Ziglio 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.
>
> Also, fix a memory leak of a cpumask in the unlikely event that
> hpet_assign_irq() fails.
>
> Signed-off-by: Frediano Ziglio <frediano.ziglio@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

For reference, this was the underlying issue behind my patch "[Patch]
xen/spinlock: Improvements to check_lock()", Message ID
"1387816747-21470-1-git-send-email-andrew.cooper3@citrix.com"

The hpet code was receiving a legacy interrupt between enabling
interrupts and initialising the spinlock, and check_lock() was mistaking
the 0 in lock->debug.irq_safe for "not IRQ-safe".

~Andrew

> ---
>  xen/arch/x86/hpet.c |   59 +++++++++++++++++++++++++++------------------------
>  1 file changed, 31 insertions(+), 28 deletions(-)
>
> diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
> index 3a4f7e8..0dedfb7 100644
> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -387,6 +387,26 @@ static int __init hpet_assign_irq(struct hpet_event_channel *ch)
>      return 0;
>  }
>  
> +static void __init hpet_init_channel(struct hpet_event_channel *ch)
> +{
> +    u64 hpet_rate = hpet_setup();
> +
> +    /*
> +     * The period is a femto seconds value. We need to calculate the scaled
> +     * math multiplication factor for nanosecond to hpet tick conversion.
> +     */
> +    ch->mult = div_sc((unsigned long)hpet_rate,
> +                                     1000000000ul, 32);
> +    ch->shift = 32;
> +    ch->next_event = STIME_MAX;
> +    spin_lock_init(&ch->lock);
> +    ch->event_handler = handle_hpet_broadcast;
> +
> +    ch->msi.irq = -1;
> +    ch->msi.msi_attrib.maskbit = 1;
> +    ch->msi.msi_attrib.pos = MSI_TYPE_HPET;
> +}
> +
>  static void __init hpet_fsb_cap_lookup(void)
>  {
>      u32 id;
> @@ -423,11 +443,15 @@ static void __init hpet_fsb_cap_lookup(void)
>              break;
>          }
>  
> +        hpet_init_channel(ch);
> +
>          ch->flags = 0;
>          ch->idx = i;
>  
>          if ( hpet_assign_irq(ch) == 0 )
>              num_hpets_used++;
> +        else
> +            free_cpumask_var(ch->cpumask);
>      }
>  
>      printk(XENLOG_INFO "HPET: %u timers usable for broadcast (%u total)\n",
> @@ -553,7 +577,6 @@ void __init hpet_broadcast_init(void)
>  {
>      u64 hpet_rate = hpet_setup();
>      u32 hpet_id, cfg;
> -    unsigned int i, n;
>  
>      if ( hpet_rate == 0 || hpet_broadcast_is_available() )
>          return;
> @@ -565,7 +588,6 @@ void __init hpet_broadcast_init(void)
>      {
>          /* Stop HPET legacy interrupts */
>          cfg &= ~HPET_CFG_LEGACY;
> -        n = num_hpets_used;
>      }
>      else
>      {
> @@ -577,11 +599,10 @@ void __init hpet_broadcast_init(void)
>              hpet_events = xzalloc(struct hpet_event_channel);
>          if ( !hpet_events || !zalloc_cpumask_var(&hpet_events->cpumask) )
>              return;
> -        hpet_events->msi.irq = -1;
> +        hpet_init_channel(hpet_events);
>  
>          /* Start HPET legacy interrupts */
>          cfg |= HPET_CFG_LEGACY;
> -        n = 1;
>  
>          if ( !force_hpet_broadcast )
>              pv_rtc_handler = handle_rtc_once;
> @@ -589,31 +610,13 @@ void __init hpet_broadcast_init(void)
>  
>      hpet_write32(cfg, HPET_CFG);
>  
> -    for ( i = 0; i < n; i++ )
> +    if ( cfg & HPET_CFG_LEGACY )
>      {
> -        if ( i == 0 && (cfg & HPET_CFG_LEGACY) )
> -        {
> -            /* set HPET T0 as oneshot */
> -            cfg = hpet_read32(HPET_Tn_CFG(0));
> -            cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
> -            cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
> -            hpet_write32(cfg, HPET_Tn_CFG(0));
> -        }
> -
> -        /*
> -         * The period is a femto seconds value. We need to calculate the scaled
> -         * math multiplication factor for nanosecond to hpet tick conversion.
> -         */
> -        hpet_events[i].mult = div_sc((unsigned long)hpet_rate,
> -                                     1000000000ul, 32);
> -        hpet_events[i].shift = 32;
> -        hpet_events[i].next_event = STIME_MAX;
> -        spin_lock_init(&hpet_events[i].lock);
> -        wmb();
> -        hpet_events[i].event_handler = handle_hpet_broadcast;
> -
> -        hpet_events[i].msi.msi_attrib.maskbit = 1;
> -        hpet_events[i].msi.msi_attrib.pos = MSI_TYPE_HPET;
> +        /* set HPET T0 as oneshot */
> +        cfg = hpet_read32(HPET_Tn_CFG(0));
> +        cfg &= ~(HPET_TN_LEVEL | HPET_TN_PERIODIC);
> +        cfg |= HPET_TN_ENABLE | HPET_TN_32BIT;
> +        hpet_write32(cfg, HPET_Tn_CFG(0));
>      }
>  
>      if ( !num_hpets_used )

Re: [PATCH] Avoid race conditions in HPET initialization

[Jan Beulich]

>>> 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

Re: [PATCH] Avoid race conditions in HPET initialization

[Andrew Cooper]

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