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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Aug 17, 2020 at 02:14:01PM +0100, Julien Grall wrote:
> Hi,
> 
> On 17/08/2020 13:46, Roger Pau Monné wrote:
> > On Fri, Aug 14, 2020 at 08:25:28PM +0100, 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()? I 
> > > would
> > > also suggest to move them implementation in a new header asm/lib.h.
> > 
> > Maybe {READ/WRITE}_SINGLE (to note those should be implemented using a
> > single instruction)?
> 
> The asm volatile statement contains only one instruction, but this doesn't
> mean the helper will generate a single instruction.

Well, the access should be done using a single instruction, which is
what we care about when using this helpers.

> You may have other instructions to get the registers ready for the access.
> 
> > 
> > ACCESS_ONCE (which also has the _ONCE suffix) IIRC could be
> > implemented using several instructions, and hence doesn't seem right
> > that they all have the _ONCE suffix.
> 
> The goal here is the same, we want to access the variable *only* once.

Right, but this is not guaranteed by the current implementation of
ACCESS_ONCE AFAICT, as the compiler *might* split the access into two
(or more) instructions, and hence won't be an atomic access anymore?

> May I ask why we would want to expose the difference to the user?

I'm not saying we should, but naming them using the _ONCE suffix seems
misleading IMO, as they have different guarantees than what
ACCESS_ONCE currently provides.

Thanks, Roger.