Re: [PATCH] xen/x86: irq: Avoid a TOCTOU race in pirq_spin_lock_irq_desc()

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 18/08/2020 09:39, Jan Beulich wrote:
> On 14.08.2020 21:25, Julien Grall wrote:
> > Hi Andrew,
> >
> > Sorry for the late answer.
> >
> > On 23/07/2020 14:59, Andrew Cooper wrote:
> > > On 23/07/2020 14:22, Julien Grall wrote:
> > > > Hi Jan,
> > > >
> > > > On 23/07/2020 12:23, Jan Beulich wrote:
> > > > > On 22.07.2020 18:53, Julien Grall wrote:
> > > > > > --- a/xen/arch/x86/irq.c
> > > > > > +++ b/xen/arch/x86/irq.c
> > > > > > @@ -1187,7 +1187,7 @@ struct irq_desc *pirq_spin_lock_irq_desc(
> > > > > >           for ( ; ; )
> > > > > >         {
> > > > > > -        int irq = pirq->arch.irq;
> > > > > > +        int irq = read_atomic(&pirq->arch.irq);
> > > > > There we go - I'd be fine this way, but I'm pretty sure Andrew
> > > > > would want this to be ACCESS_ONCE(). So I guess now is the time
> > > > > to settle which one to prefer in new code (or which criteria
> > > > > there are to prefer one over the other).
> > > > I would prefer if we have a single way to force the compiler to do a
> > > > single access (read/write).
> > > Unlikely to happen, I'd expect.
> > >
> > > But I would really like to get rid of (or at least rename)
> > > read_atomic()/write_atomic() specifically because they've got nothing to
> > > do with atomic_t's and the set of functionality who's namespace they share.
> > Would you be happy if I rename both to READ_ONCE() and WRITE_ONCE()?
> Wouldn't this lead to confusion with Linux'es macros of the same names?
From my understanding, the purpose of READ_ONCE()/WRITE_ONCE() in Linux 
is the same as our read_atomic()/write_atomic().
So I think it would be fine to rename them. An alternative would be port 
the Linux version in Xen and drop ours.
Cheers,

--
Julien Grall