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 04:53:51PM +0100, Julien Grall wrote:
> 
> 
> On 17/08/2020 16:03, Roger Pau Monné wrote:
> > On Mon, Aug 17, 2020 at 03:39:52PM +0100, Julien Grall wrote:
> > > 
> > > 
> > > On 17/08/2020 15:01, Roger Pau Monné wrote:
> > > > 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?
> > >  From my understanding, at least on GCC/Clang, ACCESS_ONCE() should be 
> > > atomic
> > > if you are using aligned address and the size smaller than a register 
> > > size.
> > 
> > Yes, any sane compiler shouldn't split such access, but this is not
> > guaranteed by the current code in ACCESS_ONCE.
> To be sure, your concern here is not about GCC/Clang but other compilers. Am
> I correct?

Or about the existing ones switching behavior, which is again quite
unlikely I would like to assume.

> We already have a collection of compiler specific macros in compiler.h. So
> how about we classify this macro as a compiler specific one? (See more
> below).
> 
> > 
> > > > 
> > > > > 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.
> > > 
> > > Lets leave aside how ACCESS_ONCE() is implemented for a moment.
> > > 
> > > If ACCESS_ONCE() doesn't guarantee atomicy, then it means you may read a 
> > > mix
> > > of the old and new value. This would most likely break quite a few of the
> > > users because the result wouldn't be coherent.
> > > 
> > > Do you have place in mind where the non-atomicity would be useful?
> > 
> > Not that I'm aware, I think they could all be safely switched to use
> > the atomic variants
> There is concern that read_atomic(), write_atomic() prevent the compiler to
> do certain optimization. Andrew gave the example of:
> 
> ACCESS_ONCE(...) |= ...

I'm not sure how will that behave when used with a compile known
value that's smaller than the size of the destination. Could the
compiler optimize this as a partial read/write if only the lower byte
is modified for example?

> 
> > 
> > In fact I wouldn't be surprised if users of ACCESS_ONCE break if the
> > access was split into multiple instructions.
> > 
> > My comment was to notice that just renaming the atomic read/write
> > helpers to use the _ONCE prefix is IMO weird as they offer different
> > properties than ACCESS_ONCE, and hence might confuse users.Just
> > looking at READ_ONCE users could assume all _ONCE helpers would
> > guarantee atomicity, which is not the case.
> 
> Our implementation of ACCESS_ONCE() is very similar to what Linux used to
> have. There READ_ONCE()/WRITE_ONCE() are also using the same principles.
> 
> From my understanding, you can safely assume the access will be atomic if
> the following conditions are met:
>       - The address is correctly size
>       - The size is smaller than the word machine size

I guess we could go that route, and properly document what each helper
is supposed to do, and that {READ/WRITE}_ONCE guarantee atomicity
while ACCESS_ONCE requires special condition for us to guarantee the
operation will be atomic.

> I would agree this may not be correct on all the existing compilers. But
> this macro could easily be re-implemented if we add support for a compiler
> with different guarantee.
> 
> Therefore, I fail to see why we can't use the same guarantee in Xen.

I'm fine if what's expected from each helper is documented, it just
seems IMO more confusing that using more differentiated naming for the
helpers, because the fact that ACCESS_ONCE is atomic is a compiler
defined behavior, but not something that could be guaranteed from the
code itself.

Thanks, Roger.