[PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)

[PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)

[Davidwang]

From: David Wang <davidwang@zhaoxin.com>

For the ch->cpumask be cleared by other cpu, cpumask_first() called by
hpet_detach_channel() return nr_cpu_ids. That lead an assertion in
set_channel_irq_affinity() when cpumask_of() check cpu.
Fix this by using a local variable.

Signed-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..6b3587a 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)
 {
+    cpumask_t cpumask;
+
     spin_lock_irq(&ch->lock);
 
     ASSERT(ch == per_cpu(cpu_bc_channel, cpu));
 
     per_cpu(cpu_bc_channel, cpu) = NULL;
+    cpumask_copy(&cpumask, ch->cpumask);
 
     if ( cpu != ch->cpu )
         spin_unlock_irq(&ch->lock);
-    else if ( cpumask_empty(ch->cpumask) )
+    else if ( cpumask_empty(&cpumask) )
     {
         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 = cpumask_first(&cpumask);
         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 v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)

[Jan Beulich]

>>> On 18.04.18 at 11:25, <Davidwang@zhaoxin.com> wrote:
> From: David Wang <davidwang@zhaoxin.com>
> 
> For the ch->cpumask be cleared by other cpu, cpumask_first() called by
> hpet_detach_channel() return nr_cpu_ids. That lead an assertion in
> set_channel_irq_affinity() when cpumask_of() check cpu.
> Fix this by using a local variable.

The fix isn't to use a local variable, introducing a local variable is only a
vehicle for addressing the bug. Also I'm afraid I still can't make much
sense of the first sentence; it only is that now I know what you want
to fix.

> --- 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)
>  {
> +    cpumask_t cpumask;

No, certainly not. We don't want variables of that type on the stack.
Recall that in v1 review I wrote "how about eliminating the
cpumask_empty() call in favor of just the cpumask_first()". The local
variable to introduce is to hold the result of cpumask_first().

Jan



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

答复: [PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)

[David Wang]

Hi Jan,
Thank you for reply. Maybe My description is not clear. Please allow me to explain it again.
Multiple CPU may be share an channel(according to  hpet_get_channel(), it is possible ). When  one of  them get the lock of channel(hpet_broadcast_exit()->hpet_detach_channel()->spin_lock_irq(&ch->lock)) , others shouldn't rewrite/clear the ch->cpumask in hpet_broadcast_exit(). This lead to errors.
For example:
CPU zero and CPU one share an channel by executing hpet_get_channel() respectively and  ch->cpumask of channel be set to 0x3.
Next, CPU zero  execute hpet_broadcast_exit()->cpumask_clear_cpu() and the ch->cpumask is 0x2.  
CPU zero execute hpet_broadcast_exit()->hpet_detach_channel()->cpumask_empty() and it get a false.  
After that the next moment, CPU one  execute hpet_broadcast_exit()->cpumask_clear_cpu(). That set the ch->cpumask to 0.
When CPU zero execute hpet_detach_channel()->cpumask_first(), ch->cpu would be set to nr_cpu_ids for ch->cpumask being 0.
An assertion would happen through hpet_detach_channel() ->set_channel_irq_affinity() -> cpumask_of() ->cpumask_check();
 
I think the cause leading to assertion is that  cpu rewrite  shared zone when other is reading. I've tried two ways.  The CPU must get the lock of channel before  executing cpumask_clear_cpu() as patch v1.  Another way of resolving it is "a variable hold the value of  ch->cpumask at the beginning of hpet_detach_channel() as patch v2" .

"how about eliminating the cpumask_empty() call in favor of just the cpumask_first()"  
Do you mean to delete  the cpumask_empty() and leave the cpumask_first()? 

I don't know if  i express clearly.   Best wish and look forward to your reply !

.
发件人: Jan Beulich <JBeulich@suse.com>
发送时间: 2018年4月18日 23:49
收件人: David Wang
抄送: xen-devel; Fiona Li(BJ-RD)
主题: Re: [PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)
    
>>> On 18.04.18 at 11:25, <Davidwang@zhaoxin.com> wrote:
> From: David Wang <davidwang@zhaoxin.com>
> 
> For the ch->cpumask be cleared by other cpu, cpumask_first() called by
> hpet_detach_channel() return nr_cpu_ids. That lead an assertion in
> set_channel_irq_affinity() when cpumask_of() check cpu.
> Fix this by using a local variable.

The fix isn't to use a local variable, introducing a local variable is only a
vehicle for addressing the bug. Also I'm afraid I still can't make much
sense of the first sentence; it only is that now I know what you want
to fix.

> --- 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)
>  {
> +    cpumask_t cpumask;

No, certainly not. We don't want variables of that type on the stack.
Recall that in v1 review I wrote "how about eliminating the
cpumask_empty() call in favor of just the cpumask_first()". The local
variable to introduce is to hold the result of cpumask_first().

Jan


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

Re: 答复: [PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)

[Jan Beulich]

>>> On 19.04.18 at 06:50, <DavidWang@zhaoxin.com> wrote:
> Thank you for reply. Maybe My description is not clear. Please allow me to 
> explain it again.
> Multiple CPU may be share an channel(according to  hpet_get_channel(), it is 
> possible ). When  one of  them get the lock of 
> channel(hpet_broadcast_exit()->hpet_detach_channel()->spin_lock_irq(&ch->lock)) 
> , others shouldn't rewrite/clear the ch->cpumask in hpet_broadcast_exit(). 
> This lead to errors.
> For example:
> CPU zero and CPU one share an channel by executing hpet_get_channel() 
> respectively and  ch->cpumask of channel be set to 0x3.
> Next, CPU zero  execute hpet_broadcast_exit()->cpumask_clear_cpu() and the 
> ch->cpumask is 0x2.  
> CPU zero execute hpet_broadcast_exit()->hpet_detach_channel()->cpumask_empty() 
> and it get a false.  
> After that the next moment, CPU one  execute 
> hpet_broadcast_exit()->cpumask_clear_cpu(). That set the ch->cpumask to 0.
> When CPU zero execute hpet_detach_channel()->cpumask_first(), ch->cpu would be 
> set to nr_cpu_ids for ch->cpumask being 0.
> An assertion would happen through hpet_detach_channel() 
> ->set_channel_irq_affinity() -> cpumask_of() ->cpumask_check();
>  
> I think the cause leading to assertion is that  cpu rewrite  shared zone 
> when other is reading. I've tried two ways.  The CPU must get the lock of 
> channel before  executing cpumask_clear_cpu() as patch v1.  Another way of 
> resolving it is "a variable hold the value of  ch->cpumask at the beginning of 
> hpet_detach_channel() as patch v2" .

Following your earlier description I had been able to work out what you
describe above. But that doesn't mean the description of the patch now
can remain unclear: You need to describe the issue in an understandable
way. This doesn't, however, necessarily mean to make the description
much longer (i.e. I don't think the above would be a suitable replacement).
See below.

> "how about eliminating the cpumask_empty() call in favor of just the cpumask_first()"  
> Do you mean to delete  the cpumask_empty() and leave the cpumask_first()? 

FAOD "leave" isn't the right term: I did suggest to _move_ cpumask_first(),
such that you can use its result both in place of the current cpumask_empty()
one _and_ where it is being used currently.

>>>> On 18.04.18 at 11:25, <Davidwang@zhaoxin.com> wrote:
>> From: David Wang <davidwang@zhaoxin.com>
>> 
>> For the ch->cpumask be cleared by other cpu, cpumask_first() called by
>> hpet_detach_channel() return nr_cpu_ids. That lead an assertion in
>> set_channel_irq_affinity() when cpumask_of() check cpu.
>> Fix this by using a local variable.

I think the description in v1 came closer to something understandable.
Taking that, how about:

"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()."

?

Jan



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

答复: 答复: [PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)

[David Wang]

Jan , thank you for your explanation. 
Your mean : To avoid  evaluating the cpumask twice in hpet_detach_channel(), use a local variable  to hold the result of cpumask_first(). Then  cpumask_empty() could be  replaced  by comparing the value of variable and nr_cpu_ids.

I understand it right?



发件人: Jan Beulich <JBeulich@suse.com>
发送时间: 2018年4月19日 15:30
收件人: David Wang
抄送: xen-devel; Fiona Li(BJ-RD)
主题: Re: 答复: [PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)
    
>>> On 19.04.18 at 06:50, <DavidWang@zhaoxin.com> wrote:
> Thank you for reply. Maybe My description is not clear. Please allow me to 
> explain it again.
> Multiple CPU may be share an channel(according to  hpet_get_channel(), it is 
> possible ). When  one of  them get the lock of 
> channel(hpet_broadcast_exit()->hpet_detach_channel()->spin_lock_irq(&ch->lock))
> , others shouldn't rewrite/clear the ch->cpumask in hpet_broadcast_exit(). 
> This lead to errors.
> For example:
> CPU zero and CPU one share an channel by executing hpet_get_channel() 
> respectively and  ch->cpumask of channel be set to 0x3.
> Next, CPU zero  execute hpet_broadcast_exit()->cpumask_clear_cpu() and the 
> ch->cpumask is 0x2.  
> CPU zero execute hpet_broadcast_exit()->hpet_detach_channel()->cpumask_empty() 
> and it get a false.  
> After that the next moment, CPU one  execute 
> hpet_broadcast_exit()->cpumask_clear_cpu(). That set the ch->cpumask to 0.
> When CPU zero execute hpet_detach_channel()->cpumask_first(), ch->cpu would be 
> set to nr_cpu_ids for ch->cpumask being 0.
> An assertion would happen through hpet_detach_channel() 
> ->set_channel_irq_affinity() -> cpumask_of() ->cpumask_check();
>  
> I think the cause leading to assertion is that  cpu rewrite  shared zone 
> when other is reading. I've tried two ways.  The CPU must get the lock of 
> channel before  executing cpumask_clear_cpu() as patch v1.  Another way of 
> resolving it is "a variable hold the value of  ch->cpumask at the beginning of 
> hpet_detach_channel() as patch v2" .

Following your earlier description I had been able to work out what you
describe above. But that doesn't mean the description of the patch now
can remain unclear: You need to describe the issue in an understandable
way. This doesn't, however, necessarily mean to make the description
much longer (i.e. I don't think the above would be a suitable replacement).
See below.

> "how about eliminating the cpumask_empty() call in favor of just the cpumask_first()" 
> Do you mean to delete  the cpumask_empty() and leave the cpumask_first()? 

FAOD "leave" isn't the right term: I did suggest to _move_ cpumask_first(),
such that you can use its result both in place of the current cpumask_empty()
one _and_ where it is being used currently.

>>>> On 18.04.18 at 11:25, <Davidwang@zhaoxin.com> wrote:
>> From: David Wang <davidwang@zhaoxin.com>
>> 
>> For the ch->cpumask be cleared by other cpu, cpumask_first() called by
>> hpet_detach_channel() return nr_cpu_ids. That lead an assertion in
>> set_channel_irq_affinity() when cpumask_of() check cpu.
>> Fix this by using a local variable.

I think the description in v1 came closer to something understandable.
Taking that, how about:

"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()."

?

Jan


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

Re: 答复: 答复: [PATCH v2] x86/hpet: Fix possible ASSERT(cpu < nr_cpu_ids)

[Jan Beulich]

>>> On 19.04.18 at 10:56, <DavidWang@zhaoxin.com> wrote:
> Jan , thank you for your explanation. 
> Your mean : To avoid  evaluating the cpumask twice in hpet_detach_channel(), 
> use a local variable  to hold the result of cpumask_first(). Then  
> cpumask_empty() could be  replaced  by comparing the value of variable and 
> nr_cpu_ids.
> 
> I understand it right?

Yes.

But please stop top-posting.

Jan



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