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

[Julien Grall <julien@xxxxxxx>]

Hi,

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?
> Yes.
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.
Cheers,

--
Julien Grall