[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v1.1 2/3] xen/sched: fix theoretical races accessing vcpu->dirty_cpu


On 02/05/2020 12:45, Jürgen Groß wrote:
On 02.05.20 13:36, Julien Grall wrote:
Hi Juergen,

They are less theoritical than we would want. :/ There was a great series of article on lwn [1] about compiler optimization last year.

There is at least a few TOCTOU in the code where you could end up with cpumask_of(VCPU_CPU_CLEAN).

It is theoretical in the sense that I don't know of any failure
resulting due to this.

How about "latent" instead of "theoritical"?

On 30/04/2020 16:28, Juergen Gross wrote:
The dirty_cpu field of struct vcpu denotes which cpu still holds data
of a vcpu. All accesses to this field should be atomic in case the
vcpu could just be running, as it is accessed without any lock held
in most cases.

Looking at the patch below, I am not sure why the issue is happening only when running. For instance, in the case of context_switch(), 'next' should not be running.

Instead, I think, the race would happen if the vCPU state is synchronized (__sync_local_execstate()) at the same time as time context_switch(). Am I correct?


Would you mind adding this context in the commit message?


diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 195e7ee583..81628e2d98 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -844,7 +844,7 @@ static inline bool is_vcpu_dirty_cpu(unsigned int cpu)
  static inline bool vcpu_cpu_dirty(const struct vcpu *v)
-    return is_vcpu_dirty_cpu(v->dirty_cpu);
+    return is_vcpu_dirty_cpu(read_atomic((unsigned int *)&v->dirty_cpu));

Is the cast necessary?

Yes, that was the problem when building for ARM I encountered.

read_atomic() on ARM has a local variable of the same type as the
read_atomic() parameter for storing the result. Due to the const
attribute of v this results in assignment to a read-only variable.

Doh, we should be able to read from a const value without. So I would argue this is a bug in the read_atomic() implementation on Arm. I will try to come up with a patch.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.