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

[Julien Grall <julien@xxxxxxx>]

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).
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. So maybe we want to import 
READ_ONCE() and WRITE_ONCE() from Linux?
As a side note, I have seen suggestion only (see [1]) which suggest that 
they those helpers wouldn't be portable:
"One relatively unimportant misunderstanding is due to the fact that the 
standard only talks about accesses to volatile objects. It does not talk 
about accesses via volatile qualified pointers. Some programmers believe 
that using a pointer-to-volatile should be handled as though it pointed 
to a volatile object. That is not guaranteed by the standard and is 
therefore not portable. However, this is relatively unimportant because 
gcc does in fact treat a pointer-to-volatile as though it pointed to a 
volatile object."
I would assume that the use is OK on CLang and GCC given that Linux has 
been using it.

> And this is of course besides the fact that I think we have many
> more instances where guaranteeing a single access would be
> needed, if we're afraid of the described permitted compiler
> behavior. Which then makes me wonder if this is really something
> we should fix one by one, rather than by at least larger scope
> audits (in order to not suggest "throughout the code base").
It depends how much the contributor can invest on chasing the rest of 
the issues. The larger the scope is, the less likely you will find 
someone that has bandwith to allocate for fixing it completely.
If the scope is "a field", then I think it is a reasonable suggesting.

In this case, I had a look at arch.irq and wasn't able to spot other 
potential issue.
> As a minor remark, unless you've observed problematic behavior,
> would you mind adding "potential" or "theoretical" to the title?
I am not aware of any issues with compiler so far. So I can add 
"potential" in the title.
Cheers,

[1] https://www.airs.com/blog/archives/154

--
Julien Grall