[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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(). 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.

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.