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

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

On Thu, Jul 23, 2020 at 02:59:57PM +0100, 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.

I've been thinking about this a bit, and maybe the problem is the
other way around. Having an 'atomic' type doesn't make much sense IMO,
because atomicity is not (solely) based on the type but rather on how
the accesses are performed.

Maybe it would make more sense to rename atomic_t to counter_t: a
counter that guarantees atomic accesses. Then the atomic namespace
could be used to implement the low level helpers that actually
guarantee atomicity and support a wider set of types than what
atomic_t currently is (ie: an int).

Roger.