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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

>
> The existing implementation of ACCESS_ONCE() can only work on scalar
> type. The implementation is based on a Linux, although we have an
> extra check. Looking through the Linux history, it looks like it is
> not possible to make ACCESS_ONCE() work with non-scalar types:
>
>     ACCESS_ONCE does not work reliably on non-scalar types. For
>     example gcc 4.6 and 4.7 might remove the volatile tag for such
>     accesses during the SRA (scalar replacement of aggregates) step
>     https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145)
>
> I understand that our implementation of read_atomic(), write_atomic()
> would lead to less optimized code.

There are cases where read_atomic()/write_atomic() prevent optimisations
which ACCESS_ONCE() would allow, but it is only for code of the form:

ACCESS_ONCE(ptr) |= val;

Which a sufficiently clever compiler could convert to a single `or $val,
ptr` instruction on x86, while read_atomic()/write_atomic() would force
it to be `mov ptr, %reg; or $val, %reg; mov %reg, ptr`.

That said - your note about GCC treating the pointed-to object as
volatile probably means it won't make the above optimisation, even
though it would be appropriate to do so.

> So maybe we want to import READ_ONCE() and WRITE_ONCE() from Linux?

There is no point.  Linux has taken a massive detour through wildly
different READ/WRITE_ONCE() functions (to fix the above GCC bugs), and
are now back to something very similar to ACCESS_ONCE().

~Andrew