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



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.

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?

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

Cheers,

[1] <d3ba0dad-63db-06ad-ff3f-f90fe8649845@xxxxxxxx>




Jan


--
Julien Grall



 


Rackspace

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