[PATCH v3] x86: Fix possible ASSERT(cpu < nr_cpu_ids)

[PATCH v3] x86: Fix possible ASSERT(cpu < nr_cpu_ids)

[Davidwang]

From: David Wang <davidwang@zhaoxin.com>

CPUs may share an in-use channel. Hence clearing of a bit from
the cpumask (in hpet_broadcast_exit()) as well as setting one
(in hpet_broadcast_enter()) must not race evaluation of that same
cpumask. Therefore avoid evaluating the cpumask twice in
hpet_detach_channel(). Otherwise cpumask_empty() may e.g.return
false while the subsequent cpumask_first() could return nr_cpu_ids,
which then triggers the assertion in cpumask_of() reached through
set_channel_irq_affinity().

Sign-off-by: David Wang <davidwang@zhaoxin.com>
---
 xen/arch/x86/hpet.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hpet.c b/xen/arch/x86/hpet.c
index bc7a851..fda0431 100644
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -509,15 +509,18 @@ static void hpet_attach_channel(unsigned int cpu,
 static void hpet_detach_channel(unsigned int cpu,
                                 struct hpet_event_channel *ch)
 {
+    unsigned int next;
+
     spin_lock_irq(&ch->lock);
 
     ASSERT(ch == per_cpu(cpu_bc_channel, cpu));
 
     per_cpu(cpu_bc_channel, cpu) = NULL;
+    next = cpumask_first(ch->cpumask);
 
     if ( cpu != ch->cpu )
         spin_unlock_irq(&ch->lock);
-    else if ( cpumask_empty(ch->cpumask) )
+    else if ( next == nr_cpu_ids )
     {
         ch->cpu = -1;
         clear_bit(HPET_EVT_USED_BIT, &ch->flags);
@@ -525,7 +528,7 @@ static void hpet_detach_channel(unsigned int cpu,
     }
     else
     {
-        ch->cpu = cpumask_first(ch->cpumask);
+        ch->cpu = next;
         set_channel_irq_affinity(ch);
         local_irq_enable();
     }
-- 
2.7.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3] x86: Fix possible ASSERT(cpu < nr_cpu_ids)

[Jan Beulich]

>>> On 19.04.18 at 12:20, <Davidwang@zhaoxin.com> wrote:
> From: David Wang <davidwang@zhaoxin.com>
> 
> CPUs may share an in-use channel. Hence clearing of a bit from
> the cpumask (in hpet_broadcast_exit()) as well as setting one
> (in hpet_broadcast_enter()) must not race evaluation of that same
> cpumask. Therefore avoid evaluating the cpumask twice in
> hpet_detach_channel(). Otherwise cpumask_empty() may e.g.return
> false while the subsequent cpumask_first() could return nr_cpu_ids,
> which then triggers the assertion in cpumask_of() reached through
> set_channel_irq_affinity().
> 
> Sign-off-by: David Wang <davidwang@zhaoxin.com>

Signed-off-by

> --- a/xen/arch/x86/hpet.c
> +++ b/xen/arch/x86/hpet.c
> @@ -509,15 +509,18 @@ static void hpet_attach_channel(unsigned int cpu,
>  static void hpet_detach_channel(unsigned int cpu,
>                                  struct hpet_event_channel *ch)
>  {
> +    unsigned int next;
> +
>      spin_lock_irq(&ch->lock);
>  
>      ASSERT(ch == per_cpu(cpu_bc_channel, cpu));
>  
>      per_cpu(cpu_bc_channel, cpu) = NULL;
> +    next = cpumask_first(ch->cpumask);
>  
>      if ( cpu != ch->cpu )
>          spin_unlock_irq(&ch->lock);
> -    else if ( cpumask_empty(ch->cpumask) )
> +    else if ( next == nr_cpu_ids )

This should be >= .

Also I'd prefer if the cpumask_first() was avoided in the cpu != ch->cpu
case.

With these
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(and the changes are easy enough to make while committing)

Also Cc Jürgen.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH v3] x86: Fix possible ASSERT(cpu < nr_cpu_ids)

[Juergen Gross]

On 19/04/18 14:10, Jan Beulich wrote:
>>>> On 19.04.18 at 12:20, <Davidwang@zhaoxin.com> wrote:
>> From: David Wang <davidwang@zhaoxin.com>
>>
>> CPUs may share an in-use channel. Hence clearing of a bit from
>> the cpumask (in hpet_broadcast_exit()) as well as setting one
>> (in hpet_broadcast_enter()) must not race evaluation of that same
>> cpumask. Therefore avoid evaluating the cpumask twice in
>> hpet_detach_channel(). Otherwise cpumask_empty() may e.g.return
>> false while the subsequent cpumask_first() could return nr_cpu_ids,
>> which then triggers the assertion in cpumask_of() reached through
>> set_channel_irq_affinity().
>>
>> Sign-off-by: David Wang <davidwang@zhaoxin.com>
> 
> Signed-off-by

Release-acked-by: Juergen Gross <jgross@suse.com>


Juergen

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel