credit2 is currently modifying the pause flags of vcpus non-atomically
via sched_set_pause_flags() and sched_clear_pause_flags(). This is
dangerous as there are cases where the paus flags are modified without
any lock held.

So drop the non-atomic pause flag modification functions and rename the
atomic ones dropping the _atomic suffix.

Fixes: a76255b4266516 ("xen/sched: make credit2 scheduler vcpu agnostic.")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
It should be noted that this issue wasn't introduced by core scheduling
as even before credit2 was using the non-atomic __set_bit() and
__clear_bit() variants.
---
 xen/common/sched/credit.c  |  4 ++--
 xen/common/sched/private.h | 22 +---------------------
 2 files changed, 3 insertions(+), 23 deletions(-)

diff --git a/xen/common/sched/credit.c b/xen/common/sched/credit.c
index 93d89da278..d0aa017c64 100644
--- a/xen/common/sched/credit.c
+++ b/xen/common/sched/credit.c
@@ -453,7 +453,7 @@ static inline void __runq_tickle(const struct csched_unit *new)
                     SCHED_UNIT_STAT_CRANK(cur, kicked_away);
                     SCHED_UNIT_STAT_CRANK(cur, migrate_r);
                     SCHED_STAT_CRANK(migrate_kicked_away);
-                    sched_set_pause_flags_atomic(cur->unit, _VPF_migrating);
+                    sched_set_pause_flags(cur->unit, _VPF_migrating);
                 }
                 /* Tickle cpu anyway, to let new preempt cur. */
                 SCHED_STAT_CRANK(tickled_busy_cpu);
@@ -973,7 +973,7 @@ csched_unit_acct(struct csched_private *prv, unsigned int cpu)
         {
             SCHED_UNIT_STAT_CRANK(svc, migrate_r);
             SCHED_STAT_CRANK(migrate_running);
-            sched_set_pause_flags_atomic(currunit, _VPF_migrating);
+            sched_set_pause_flags(currunit, _VPF_migrating);
             /*
              * As we are about to tickle cpu, we should clear its bit in
              * idlers. But, if we are here, it means there is someone running
diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index 367811a12f..b9a5b4c01c 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -172,7 +172,7 @@ static inline void sched_set_pause_flags(struct sched_unit *unit,
     struct vcpu *v;
 
     for_each_sched_unit_vcpu ( unit, v )
-        __set_bit(bit, &v->pause_flags);
+        set_bit(bit, &v->pause_flags);
 }
 
 /* Clear a bit in pause_flags of all vcpus of a unit. */
@@ -181,26 +181,6 @@ static inline void sched_clear_pause_flags(struct sched_unit *unit,
 {
     struct vcpu *v;
 
-    for_each_sched_unit_vcpu ( unit, v )
-        __clear_bit(bit, &v->pause_flags);
-}
-
-/* Set a bit in pause_flags of all vcpus of a unit via atomic updates. */
-static inline void sched_set_pause_flags_atomic(struct sched_unit *unit,
-                                                unsigned int bit)
-{
-    struct vcpu *v;
-
-    for_each_sched_unit_vcpu ( unit, v )
-        set_bit(bit, &v->pause_flags);
-}
-
-/* Clear a bit in pause_flags of all vcpus of a unit via atomic updates. */
-static inline void sched_clear_pause_flags_atomic(struct sched_unit *unit,
-                                                  unsigned int bit)
-{
-    struct vcpu *v;
-
     for_each_sched_unit_vcpu ( unit, v )
         clear_bit(bit, &v->pause_flags);
 }
-- 
2.26.1

On Wed, 2020-05-06 at 17:16 +0200, Juergen Gross wrote:
> credit2 is currently modifying the pause flags of vcpus non-
> atomically
> via sched_set_pause_flags() and sched_clear_pause_flags(). This is
> dangerous as there are cases where the paus flags are modified
> without
> any lock held.
> 
Right.

> So drop the non-atomic pause flag modification functions and rename
> the
> atomic ones dropping the _atomic suffix.
> 
> Fixes: a76255b4266516 ("xen/sched: make credit2 scheduler vcpu
> agnostic.")
> Signed-off-by: Juergen Gross <jgross@suse.com>
>
Reviewed-by: Dario Faggioli <dfaggioli@suse.com>

> ---
> It should be noted that this issue wasn't introduced by core
> scheduling
> as even before credit2 was using the non-atomic __set_bit() and
> __clear_bit() variants.
>
Yes. I can see that in 222234f2ad17185 ("xen: credit2: use non-atomic
cpumask and bit operations"), where the svc->flags are switched to non-
atomic updates (as, for them, it is true that they're always accessed
while holding locks), switching of setting the _VPF_migrating
pause->flags non atomically also slipped in, but that was clearly a
mistake. :-(

I believe (but I haven't checked this part too thoroughly) that it was
the only one back then. Afterwords, when another instance was added, in
__runq_tickle(), we found the already existing one and followed suit.

Good catch, and thanks. :-)

Regards
-- 
Dario Faggioli, Ph.D
http://about.me/dario.faggioli
Virtualization Software Engineer
SUSE Labs, SUSE https://www.suse.com/
-------------------------------------------------------------------
<<This happens because _I_ choose it to happen!>> (Raistlin Majere)