Skip to contentSkip to conversation

KVM: arm64: Make vcpu flag updates non-preemptible

PATCH Applied Marc Zyngier 3 participants 3 messages Apr 18, 2023 – Apr 19, 2023

Subsystems (4)
Stable branch KVM arm64 KVM arm64
lore
Share & tools
lore

Marc Zyngier

author maintainer
#0 lore raw

PATCH v2 1/1 KVM: arm64: Make vcpu flag updates non-preemptible

Per-vcpu flags are updated using a non-atomic RMW operation. Which means it is possible to get preempted between the read and write operations.

Another interesting thing to note is that preemption also updates flags, as we have some flag manipulation in both the load and put operations.

It is thus possible to lose information communicated by either load or put, as the preempted flag update will overwrite the flags when the thread is resumed. This is specially critical if either load or put has stored information which depends on the physical CPU the vcpu runs on.

This results in really elusive bugs, and kudos must be given to Mostafa for the long hours of debugging, and finally spotting the problem.

Fix it by disabling preemption during the RMW operation, which ensures that the state stays consistent. Also upgrade vcpu_get_flag path to use READ_ONCE() to make sure the field is always atomically accessed.

Fixes e87abb73e594 ("KVM: arm64: Add helpers to manipulate vcpu flags among a set")
Reported-by Mostafa Saleh <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected]

---

Notes:
v2: add READ_ONCE() on the read path, expand commit message

 arch/arm64/include/asm/kvm_host.h | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)
arch/arm64/include/asm/kvm_host.h +18 -1
index bcd774d74f34..3dd691c85ca0 100644
@@ -576,9 +576,22 @@ struct kvm_vcpu_arch {
 	({							\
 		__build_check_flag(v, flagset, f, m);		\
 								\
-		v->arch.flagset & (m);				\
+		READ_ONCE(v->arch.flagset) & (m);		\
 	})
 
+/*
+ * Note that the set/clear accessors must be preempt-safe in order to
+ * avoid nesting them with load/put which also manipulate flags...
+ */
+#ifdef __KVM_NVHE_HYPERVISOR__
+/* the nVHE hypervisor is always non-preemptible */
+#define __vcpu_flags_preempt_disable()
+#define __vcpu_flags_preempt_enable()
+#else
+#define __vcpu_flags_preempt_disable()	preempt_disable()
+#define __vcpu_flags_preempt_enable()	preempt_enable()
+#endif
+
 #define __vcpu_set_flag(v, flagset, f, m)			\
 	do {							\
 		typeof(v->arch.flagset) *fset;			\

@@ -586,9 +599,11 @@ struct kvm_vcpu_arch {
 		__build_check_flag(v, flagset, f, m);		\
 								\
 		fset = &v->arch.flagset;			\
+		__vcpu_flags_preempt_disable();			\
 		if (HWEIGHT(m) > 1)				\
 			*fset &= ~(m);				\
 		*fset |= (f);					\
+		__vcpu_flags_preempt_enable();			\
 	} while (0)
 
 #define __vcpu_clear_flag(v, flagset, f, m)			\

@@ -598,7 +613,9 @@ struct kvm_vcpu_arch {
 		__build_check_flag(v, flagset, f, m);		\
 								\
 		fset = &v->arch.flagset;			\
+		__vcpu_flags_preempt_disable();			\
 		*fset &= ~(m);					\
+		__vcpu_flags_preempt_enable();			\
 	} while (0)
 
 #define vcpu_get_flag(v, ...)	__vcpu_get_flag((v), __VA_ARGS__)
Signature · 1 line
2.34.1
Applied
#1 lore raw
Quoted 9 lines · Marc Zyngier

Per-vcpu flags are updated using a non-atomic RMW operation. Which means it is possible to get preempted between the read and write operations.

Another interesting thing to note is that preemption also updates flags, as we have some flag manipulation in both the load and put operations.

[...]

Applied to kvmarm/fixes, thanks!

[1/1] KVM: arm64: Make vcpu flag updates non-preemptible

      https://git.kernel.org/kvmarm/kvmarm/c/35dcb3ac663a

--
Best,
Oliver

maintainer Acked-by on v2 1/1
#2 lore raw
In reply to Marc Zyngier
Quoted 23 lines · Marc Zyngier

Per-vcpu flags are updated using a non-atomic RMW operation. Which means it is possible to get preempted between the read and write operations.

Another interesting thing to note is that preemption also updates flags, as we have some flag manipulation in both the load and put operations.

It is thus possible to lose information communicated by either load or put, as the preempted flag update will overwrite the flags when the thread is resumed. This is specially critical if either load or put has stored information which depends on the physical CPU the vcpu runs on.

This results in really elusive bugs, and kudos must be given to Mostafa for the long hours of debugging, and finally spotting the problem.

Fix it by disabling preemption during the RMW operation, which ensures that the state stays consistent. Also upgrade vcpu_get_flag path to use READ_ONCE() to make sure the field is always atomically accessed.

Quoted 8 lines
Fixes e87abb73e594 ("KVM: arm64: Add helpers to manipulate vcpu flags among a set")
Reported-by Mostafa Saleh <[email protected]>
Signed-off-by Marc Zyngier <[email protected]>
Cc [email protected]

---

Notes:
v2: add READ_ONCE() on the read path, expand commit message

Acked-by Will Deacon <[email protected]>

Will