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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 18.08.2020 11:12, Julien Grall wrote:
> Hi Jan,
> 
> On 18/08/2020 09:57, Jan Beulich wrote:
>> On 18.08.2020 10:53, Julien Grall wrote:
>>> 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.
>>
>> The port of Linux'es {READ,WRITE}_ONCE() is our ACCESS_ONCE().
> 
> Not really... Our ACCESS_ONCE() only supports scalar type. {READ, 
> WRITE}_ONCE() are able to support non-scalar type as well.

I guess that's merely because ours was derived from an earlier version of
Linux.

>> As pointed
>> out before, ACCESS_ONCE() and {read,write}_atomic() serve slightly
>> different purposes, and so far it looks like all of us are lacking ideas
>> on how to construct something that catches all cases by one single approach.
> 
> I am guessing you are referring to [1], right?
> 
> If you read the documentation of READ_ONCE()/WRITE_ONCE(), they are meant to 
> be atomic in some cases. The cases are exactly the same as {read, 
> write}_atomic().
> 
> I will ask the same thing as I asked to Roger. If Linux can rely on it, why 
> can't we?

That's not the way I'd like to see arguments go here: If Linux has
something suitable, I'm fine with us using it. But we ought to be
permitted to question whether what we inherit is indeed fit for
purpose, and afaict there is at least one gap to be filled. In no
case should we blindly pull in things from Linux and then assume
that simply by doing so all is well.

> Although, I agree that the implementation is not portable to another 
> compiler. But that's why they are implemented in compiler.h.

Aiui items in compiler.h are meant to be suitable for all compilers,
whereas truly gcc-specific things (for example) live in
compiler-gcc.h (where Linux has two further ones they support).

Jan