xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH for-4.15] xen/sched: Add missing memory barrier in vcpu_block()
@ 2021-02-20 19:47 Julien Grall
  2021-02-22 11:11 ` Ian Jackson
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Julien Grall @ 2021-02-20 19:47 UTC (permalink / raw)
  To: xen-devel
  Cc: iwj, jbeulich, sstabellini, ash.j.wilding, Julien Grall,
	George Dunlap, Dario Faggioli

From: Julien Grall <jgrall@amazon.com>

The comment in vcpu_block() states that the events should be checked
/after/ blocking to avoids wakeup waiting race. However, from a generic
perspective, set_bit() doesn't prevent re-ordering. So the following
could happen:

CPU0  (blocking vCPU A)         |   CPU1 ( unblock vCPU A)
                                |
A <- read local events          |
                                |   set local events
                                |   test_and_clear_bit(_VPF_blocked)
                                |       -> Bail out as the bit if not set
                                |
set_bit(_VFP_blocked)           |
                                |
check A                         |

The variable A will be 0 and therefore the vCPU will be blocked when it
should continue running.

vcpu_block() is now gaining an smp_mb__after_atomic() to prevent the CPU
to read any information about local events before the flag _VPF_blocked
is set.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

This is a follow-up of the discussion that started in 2019 (see [1])
regarding a possible race between do_poll()/vcpu_unblock() and the wake
up path.

I haven't yet fully thought about the potential race in do_poll(). If
there is, then this would likely want to be fixed in a separate patch.

On x86, the current code is safe because set_bit() is fully ordered. SO
the problem is Arm (and potentially any new architectures).

I couldn't convince myself whether the Arm implementation of
local_events_need_delivery() contains enough barrier to prevent the
re-ordering. However, I don't think we want to play with devil here as
the function may be optimized in the future.

This patch is candidate for 4.15. The risk is really small as the memory
ordering will be stricter on Arm and therefore less racy.

[1] <3ce4998b-a8a8-37bd-bb26-9550571709b6@suse.com>
---
 xen/common/sched/core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/common/sched/core.c b/xen/common/sched/core.c
index 9745a77eee23..2b974fd6f8ba 100644
--- a/xen/common/sched/core.c
+++ b/xen/common/sched/core.c
@@ -1418,6 +1418,8 @@ void vcpu_block(void)
 
     set_bit(_VPF_blocked, &v->pause_flags);
 
+    smp_mb__after_atomic();
+
     arch_vcpu_block(v);
 
     /* Check for events /after/ blocking: avoids wakeup waiting race. */
-- 
2.17.1



^ permalink raw reply related	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-02-25 21:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-20 19:47 [PATCH for-4.15] xen/sched: Add missing memory barrier in vcpu_block() Julien Grall
2021-02-22 11:11 ` Ian Jackson
2021-02-22 14:26 ` Jan Beulich
2021-02-22 20:09   ` Stefano Stabellini
2021-02-22 20:12     ` Julien Grall
2021-02-23  1:22       ` Stefano Stabellini
2021-02-23  7:00       ` Jan Beulich
2021-02-23  9:04         ` Julien Grall
2021-02-23  8:45     ` Dario Faggioli
2021-02-23 13:24 ` Ash Wilding
2021-02-25 14:01   ` Julien Grall
2021-02-25 21:35     ` Stefano Stabellini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).